Fix isSalCallFunction further

...after a31267be "Fix isSalCallFunction so it
also works on Windows", so that it actually does work on Windows.

Change-Id: I0218fb41b3e1000e2325967a18dfaafaa95fe415
Reviewed-on: https://gerrit.libreoffice.org/46193Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

compilerplugins/clang/compat.hxx

@@ -206,6 +206,20 @@ inline void addPPCallbacks(
 #endif
 }
 
+inline bool isPointWithin(
+    clang::SourceManager const & SM, clang::SourceLocation Location, clang::SourceLocation Start,
+    clang::SourceLocation End)
+{
+#if CLANG_VERSION >= 60000
+    return SM.isPointWithin(Location, Start, End);
+#else
+    return
+        Location == Start || Location == End
+        || (SM.isBeforeInTranslationUnit(Start, Location)
+            && SM.isBeforeInTranslationUnit(Location, End));
+#endif
+}
+
 inline bool isMacroArgExpansion(
     clang::CompilerInstance& compiler, clang::SourceLocation location,
     clang::SourceLocation * startLocation)

compilerplugins/clang/salcall.cxx

@@ -9,8 +9,10 @@
 
 #include "plugin.hxx"
 #include "check.hxx"
+#include "compat.hxx"
 
 #include <algorithm>
+#include <cassert>
 #include <set>
 #include <utility>
 #include <vector>
@@ -333,6 +335,8 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
     return true;
 }
 
+//TODO: This doesn't handle all possible cases of macro usage (and possibly never will be able to),
+// just what is encountered in practice:
 bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation* pLoc)
 {
     assert(!functionDecl->isTemplateInstantiation());
@@ -358,18 +362,10 @@ bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation
     }
 
     SourceManager& SM = compiler.getSourceManager();
-
-    // Stop searching for "SAL_CALL" at the start of the function declaration's name (for qualified
-    // names this will point after the qualifiers, but needlessly including those in the search
-    // should be harmless):
-    SourceLocation endLoc = functionDecl->getNameInfo().getLocStart();
-    while (endLoc.isMacroID() && SM.isAtStartOfImmediateMacroExpansion(endLoc))
-    {
-        endLoc = SM.getImmediateMacroCallerLoc(endLoc);
-    }
-    endLoc = SM.getSpellingLoc(endLoc);
+    std::vector<SourceRange> ranges;
 
     SourceLocation startLoc;
+    SourceLocation endLoc;
     bool noReturnType = isa<CXXConstructorDecl>(functionDecl)
                         || isa<CXXDestructorDecl>(functionDecl)
                         || isa<CXXConversionDecl>(functionDecl);
@@ -410,16 +406,30 @@ bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation
             return false;
         }
         startLoc = FTL.getReturnLoc().getEndLoc();
-        auto const slEnd = Lexer::getLocForEndOfToken(startLoc, 0, SM, compiler.getLangOpts());
-        if (slEnd.isValid()) // i.e., startLoc either non-macro, or at end of macro
+        while (SM.isMacroArgExpansion(startLoc, &startLoc))
         {
-            startLoc = slEnd;
         }
-        else // otherwise, resolve into macro text
+
+        // Stop searching for "SAL_CALL" at the start of the function declaration's name (for
+        // qualified names this will point after the qualifiers, but needlessly including those in
+        // the search should be harmless---modulo issues with using "SAL_CALL" as the name of a
+        // function-like macro parameter as discussed below):
+        endLoc = functionDecl->getNameInfo().getLocStart();
+        while (SM.isMacroArgExpansion(endLoc, &endLoc))
         {
-            startLoc = Lexer::getLocForEndOfToken(SM.getSpellingLoc(startLoc), 0, SM,
-                                                  compiler.getLangOpts());
         }
+        while (endLoc.isMacroID() && SM.isAtStartOfImmediateMacroExpansion(endLoc))
+        {
+            endLoc = SM.getImmediateMacroCallerLoc(endLoc);
+        }
+        endLoc = SM.getSpellingLoc(endLoc);
+
+        auto const slEnd = Lexer::getLocForEndOfToken(startLoc, 0, SM, compiler.getLangOpts());
+        if (slEnd.isValid())
+        {
+            // startLoc is either non-macro, or at end of macro; one source range from startLoc to
+            // endLoc:
+            startLoc = slEnd;
             while (startLoc.isMacroID() && SM.isAtEndOfImmediateMacroExpansion(startLoc, &startLoc))
             {
             }
@@ -428,13 +438,13 @@ bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation
             if (startLoc.isValid() && endLoc.isValid() && startLoc != endLoc
                 && !SM.isBeforeInTranslationUnit(startLoc, endLoc))
             {
-            // Happens for uses of trailing return type (in which case starting instead at the start
-            // of the function declaration should be fine), but also for cases like
+                // Happens for uses of trailing return type (in which case starting instead at the
+                // start of the function declaration should be fine), but also for cases like
                 //
                 //  void (*f())();
                 //
-            // where the function name is within the function type (TODO: in which case starting at
-            // the start can erroneously pick up the "SAL_CALL" from the returned pointer-to-
+                // where the function name is within the function type (TODO: in which case starting
+                // at the start can erroneously pick up the "SAL_CALL" from the returned pointer-to-
                 // function type in cases like
                 //
                 //  void SAL_CALL (*f())();
@@ -443,6 +453,36 @@ bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation
                 startAfterReturnType = false;
             }
         }
+        else
+        {
+            // startLoc is within a macro body; two source ranges, first is the remainder of the
+            // corresponding macro definition's replacement text, second is from after the macro
+            // invocation to endLoc, unless endLoc is already in the first range:
+            //TODO: If the macro is a function-like macro with a parameter named "SAL_CALL", uses of
+            // that parameter in the remainder of the replacement text will be false positives.
+            assert(SM.isMacroBodyExpansion(startLoc));
+            auto const startLoc2 = SM.getImmediateExpansionRange(startLoc).second;
+            auto const MI
+                = compiler.getPreprocessor()
+                      .getMacroDefinitionAtLoc(
+                          &compiler.getASTContext().Idents.get(
+                              Lexer::getImmediateMacroName(startLoc, SM, compiler.getLangOpts())),
+                          SM.getSpellingLoc(startLoc))
+                      .getMacroInfo();
+            assert(MI != nullptr);
+            auto endLoc1 = MI->getDefinitionEndLoc();
+            assert(endLoc1.isFileID());
+            endLoc1 = Lexer::getLocForEndOfToken(endLoc1, 0, SM, compiler.getLangOpts());
+            startLoc = Lexer::getLocForEndOfToken(SM.getSpellingLoc(startLoc), 0, SM,
+                                                  compiler.getLangOpts());
+            if (!compat::isPointWithin(SM, endLoc, startLoc, endLoc1))
+            {
+                ranges.emplace_back(startLoc, endLoc1);
+                startLoc = Lexer::getLocForEndOfToken(SM.getSpellingLoc(startLoc2), 0, SM,
+                                                      compiler.getLangOpts());
+            }
+        }
+    }
     if (!startAfterReturnType)
     {
         // Ctors/dtors/conversion functions don't have a return type, start searching for "SAL_CALL"
@@ -491,9 +531,22 @@ bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation
             }
         }
 #endif
+
+        // Stop searching for "SAL_CALL" at the start of the function declaration's name (for
+        // qualified names this will point after the qualifiers, but needlessly including those in
+        // the search should be harmless):
+        endLoc = functionDecl->getNameInfo().getLocStart();
+        while (endLoc.isMacroID() && SM.isAtStartOfImmediateMacroExpansion(endLoc))
+        {
+            endLoc = SM.getImmediateMacroCallerLoc(endLoc);
         }
+        endLoc = SM.getSpellingLoc(endLoc);
+    }
+    ranges.emplace_back(startLoc, endLoc);
 
-    if (startLoc.isInvalid() || endLoc.isInvalid())
+    for (auto const range : ranges)
+    {
+        if (range.isInvalid())
         {
             if (isDebugMode())
             {
@@ -503,19 +556,18 @@ bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation
             }
             return false;
         }
-    if (isDebugMode() && startLoc != endLoc && !SM.isBeforeInTranslationUnit(startLoc, endLoc))
+        if (isDebugMode() && range.getBegin() != range.getEnd()
+            && !SM.isBeforeInTranslationUnit(range.getBegin(), range.getEnd()))
         {
             report(DiagnosticsEngine::Fatal, "TODO: unexpected failure #3, needs investigation",
                    functionDecl->getLocation())
                 << functionDecl->getSourceRange();
         }
 
-    SourceLocation found;
-
-    while (SM.isBeforeInTranslationUnit(startLoc, endLoc))
+        for (auto loc = range.getBegin(); SM.isBeforeInTranslationUnit(loc, range.getEnd());)
         {
-        unsigned n = Lexer::MeasureTokenLength(startLoc, SM, compiler.getLangOpts());
-        auto s = StringRef(compiler.getSourceManager().getCharacterData(startLoc), n);
+            unsigned n = Lexer::MeasureTokenLength(loc, SM, compiler.getLangOpts());
+            auto s = StringRef(compiler.getSourceManager().getCharacterData(loc), n);
             while (s.startswith("\\\n"))
             {
                 s = s.drop_front(2);
@@ -528,23 +580,14 @@ bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation
             }
             if (s == "SAL_CALL")
             {
-            found = startLoc;
-            break;
+                if (pLoc)
+                    *pLoc = loc;
+                return true;
             }
-        if (startLoc == endLoc)
-        {
-            break;
+            loc = loc.getLocWithOffset(std::max<unsigned>(n, 1));
         }
-        startLoc = startLoc.getLocWithOffset(std::max<unsigned>(n, 1));
     }
-
-    if (found.isInvalid())
     return false;
-
-    if (pLoc)
-        *pLoc = found;
-
-    return true;
 }
 
 bool SalCall::rewrite(SourceLocation locBegin)

compilerplugins/clang/test/salcall.cxx

@@ -117,6 +117,25 @@ class Class8_3 : public Class8_1, public Class8_2
     virtual ~Class8_3();
 };
 
+#define M1(m) void m
+class Class9
+{
+    M1(method1)(); // expected-error {{SAL_CALL inconsistency [loplugin:salcall]}}
+    void method2(); // expected-error {{SAL_CALL inconsistency [loplugin:salcall]}}
+};
+void SAL_CALL Class9::method1() {} // expected-note {{SAL_CALL inconsistency [loplugin:salcall]}}
+#define M2(T) T SAL_CALL
+M2(void) Class9::method2() {} // expected-note {{SAL_CALL inconsistency [loplugin:salcall]}}
+
+#if 0 // see TODO in SalCall::isSalCallFunction
+class Class10
+{
+    void method1();
+};
+#define M3(T, SAL_CALL) T SAL_CALL::
+M3(void, Class10) method1() {} // false "SAL_CALL inconsistency"
+#endif
+
 #if 0 //TODO
 template<typename> struct S {
     virtual ~S();