Fix loplugin:salcall

For one, the addressOfSet filtering was only done for member functions, not for
free functions, which caused false positives in the wild like the one discussed
in the comments at <https://gerrit.libreoffice.org/c/core/+/102024/
4#message-36ec6ee09ed5badae93c552b82a90068e65d19e2> "call xDesktop->terminate()
when catching SIGTERM/SIGINT" regarding a free function
`terminationHandlerFunction` passed to `osl_createThread`.

For another, it failed to identify some cases where the address of a function is
taken implicitly, like for `f3` in compilerplugins/clang/test/salcall.cxx.  So
make this plugin reuse the existing `loplugin::FunctionAddress` functionality
(which also meant to get rid of this plugin's two-phase design).

Change-Id: Ie290c63b03825d5288d982bc8701cfb886fc84b4
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104585
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

2 files changed, 59 insertions, 101 deletions

diff --git a/compilerplugins/clang/salcall.cxx b/compilerplugins/clang/salcall.cxx
index e70e6477f2b0..a3f5678a3918 100644
--- a/compilerplugins/clang/salcall.cxx
+++ b/compilerplugins/clang/salcall.cxx
@@ -10,6 +10,7 @@
 #include "plugin.hxx"
 #include "check.hxx"
 #include "compat.hxx"
+#include "functionaddress.hxx"
 
 #include <algorithm>
 #include <cassert>
@@ -51,113 +52,41 @@ CXXMethodDecl const* getTemplateInstantiationPattern(CXXMethodDecl const* decl)
     return p == nullptr ? decl : cast<CXXMethodDecl>(p);
 }
 
-class SalCall final : public loplugin::FilteringRewritePlugin<SalCall>
+class SalCall final : public loplugin::FunctionAddress<loplugin::FilteringRewritePlugin<SalCall>>
 {
 public:
     explicit SalCall(loplugin::InstantiationData const& data)
-        : FilteringRewritePlugin(data)
+        : FunctionAddress(data)
     {
     }
 
     virtual void run() override
     {
-        m_phase = PluginPhase::FindAddressOf;
-        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
-        m_phase = PluginPhase::Warning;
-        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+        if (TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()))
+        {
+            auto const& addressOfSet = getFunctionsWithAddressTaken();
+            for (auto const decl : m_decls)
+            {
+                if (addressOfSet.find(decl->getCanonicalDecl()) == addressOfSet.end())
+                {
+                    handleFunctionDecl(decl);
+                }
+            }
+        }
     }
 
     bool VisitFunctionDecl(FunctionDecl const*);
-    bool VisitUnaryOperator(UnaryOperator const*);
-    bool VisitInitListExpr(InitListExpr const*);
-    bool VisitCallExpr(CallExpr const*);
-    bool VisitBinaryOperator(BinaryOperator const*);
-    bool VisitCXXConstructExpr(CXXConstructExpr const*);
 
 private:
-    void checkForFunctionDecl(Expr const*, bool bCheckOnly = false);
+    void handleFunctionDecl(FunctionDecl const* decl);
     bool rewrite(SourceLocation);
     bool isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation* pLoc = nullptr);
 
-    std::set<FunctionDecl const*> m_addressOfSet;
-    enum class PluginPhase
-    {
-        FindAddressOf,
-        Warning
-    };
-    PluginPhase m_phase;
+    std::set<FunctionDecl const*> m_decls;
 };
 
-bool SalCall::VisitUnaryOperator(UnaryOperator const* op)
-{
-    if (op->getOpcode() != UO_AddrOf)
-    {
-        return true;
-    }
-    if (m_phase != PluginPhase::FindAddressOf)
-        return true;
-    checkForFunctionDecl(op->getSubExpr());
-    return true;
-}
-
-bool SalCall::VisitBinaryOperator(BinaryOperator const* binaryOperator)
-{
-    if (binaryOperator->getOpcode() != BO_Assign)
-    {
-        return true;
-    }
-    if (m_phase != PluginPhase::FindAddressOf)
-        return true;
-    checkForFunctionDecl(binaryOperator->getRHS());
-    return true;
-}
-
-bool SalCall::VisitCallExpr(CallExpr const* callExpr)
-{
-    if (m_phase != PluginPhase::FindAddressOf)
-        return true;
-    for (auto arg : callExpr->arguments())
-        checkForFunctionDecl(arg);
-    return true;
-}
-
-bool SalCall::VisitCXXConstructExpr(CXXConstructExpr const* constructExpr)
-{
-    if (m_phase != PluginPhase::FindAddressOf)
-        return true;
-    for (auto arg : constructExpr->arguments())
-        checkForFunctionDecl(arg);
-    return true;
-}
-
-bool SalCall::VisitInitListExpr(InitListExpr const* initListExpr)
-{
-    if (m_phase != PluginPhase::FindAddressOf)
-        return true;
-    for (auto subStmt : *initListExpr)
-        checkForFunctionDecl(dyn_cast<Expr>(subStmt));
-    return true;
-}
-
-void SalCall::checkForFunctionDecl(Expr const* expr, bool bCheckOnly)
-{
-    auto e1 = expr->IgnoreParenCasts();
-    auto declRef = dyn_cast<DeclRefExpr>(e1);
-    if (!declRef)
-        return;
-    auto functionDecl = dyn_cast<FunctionDecl>(declRef->getDecl());
-    if (!functionDecl)
-        return;
-    if (bCheckOnly)
-        getParentStmt(expr)->dump();
-    else
-        m_addressOfSet.insert(functionDecl->getCanonicalDecl());
-}
-
 bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
 {
-    if (m_phase != PluginPhase::Warning)
-        return true;
     if (ignoreLocation(decl))
         return true;
 
@@ -225,16 +154,15 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
 
     if (!decl->isThisDeclarationADefinition() && !(methodDecl && methodDecl->isPure()))
         return true;
-    // can only check when we have a definition since this is the most likely time
-    // when the address of the method will be taken
-    if (methodDecl)
-    {
-        if (m_addressOfSet.find(decl->getCanonicalDecl()) != m_addressOfSet.end())
-            return true;
-    }
 
+    m_decls.insert(decl);
+    return true;
+}
+
+void SalCall::handleFunctionDecl(FunctionDecl const* decl)
+{
     // some base classes are overridden by sub-classes which override both the base-class and a UNO class
-    if (recordDecl)
+    if (auto recordDecl = dyn_cast<CXXRecordDecl>(decl->getDeclContext()))
     {
         auto dc = loplugin::DeclCheck(recordDecl);
         if (dc.Class("OProxyAggregation").Namespace("comphelper").GlobalNamespace()
@@ -266,11 +194,13 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
             || dc.Class("ScVbaControl").GlobalNamespace()
 
         )
-            return true;
+            return;
     }
 
+    auto canonicalDecl = decl->getCanonicalDecl();
+
     // if any of the overridden methods are SAL_CALL, we should be too
-    if (methodDecl)
+    if (auto methodDecl = dyn_cast<CXXMethodDecl>(canonicalDecl))
     {
         for (auto iter = methodDecl->begin_overridden_methods();
              iter != methodDecl->end_overridden_methods(); ++iter)
@@ -278,17 +208,22 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
             const CXXMethodDecl* overriddenMethod
                 = getTemplateInstantiationPattern(*iter)->getCanonicalDecl();
             if (isSalCallFunction(overriddenMethod))
-                return true;
+                return;
         }
     }
 
+    SourceLocation rewriteLoc;
+    SourceLocation rewriteCanonicalLoc;
+    bool bDeclIsSalCall = isSalCallFunction(decl, &rewriteLoc);
+    isSalCallFunction(canonicalDecl, &rewriteCanonicalLoc);
+
     bool bOK = rewrite(rewriteLoc);
     if (bOK && canonicalDecl != decl)
     {
         bOK = rewrite(rewriteCanonicalLoc);
     }
     if (bOK)
-        return true;
+        return;
 
     if (bDeclIsSalCall)
     {
@@ -307,8 +242,6 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
                 << decl->getSourceRange();
         }
     }
-
-    return true;
 }
 
 //TODO: This doesn't handle all possible cases of macro usage (and possibly never will be able to),
diff --git a/compilerplugins/clang/test/salcall.cxx b/compilerplugins/clang/test/salcall.cxx
index e424432c4036..a4464cee6064 100644
--- a/compilerplugins/clang/test/salcall.cxx
+++ b/compilerplugins/clang/test/salcall.cxx
@@ -134,6 +134,31 @@ void SAL_CALL Class9::method1() // expected-error {{SAL_CALL inconsistency [lopl
 #define M2(T) T SAL_CALL
 M2(void) Class9::method2() {} // expected-error {{SAL_CALL inconsistency [loplugin:salcall]}}
 
+void SAL_CALL f0() {} // expected-error {{SAL_CALL unnecessary here [loplugin:salcall]}}
+
+void SAL_CALL f1() {}
+
+void SAL_CALL f2() {}
+
+void SAL_CALL f3() {}
+
+void SAL_CALL f4() {}
+
+typedef void SAL_CALL (*Ptr)();
+
+void takePtr(Ptr);
+
+void usePtr()
+{
+    f0();
+    takePtr(f1);
+    takePtr(&f2);
+    Ptr p = f3;
+    takePtr(p);
+    p = f4;
+    takePtr(p);
+}
+
 #if 0 // see TODO in SalCall::isSalCallFunction
 class Class10
 {