improve loplugin:unnecessarylocking

to find more locking we can remove

Change-Id: Ief7bc5ec2a1ff31f22a0ad366910b7fcc4725818
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/148599
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 199 insertions, 49 deletions

diff --git a/compilerplugins/clang/test/unnecessarylocking.cxx b/compilerplugins/clang/test/unnecessarylocking.cxx
index 6dda5d333191..a2319623140a 100644
--- a/compilerplugins/clang/test/unnecessarylocking.cxx
+++ b/compilerplugins/clang/test/unnecessarylocking.cxx
@@ -8,14 +8,17 @@
  */
 
 #include <mutex>
+#include <osl/mutex.hxx>
 
 static std::mutex gSolarMutex;
 
-class SolarMutexGuard : public std::unique_lock<std::mutex>
+class SolarMutexGuard
 {
+    std::unique_lock<std::mutex> lock;
+
 public:
     SolarMutexGuard()
-        : std::unique_lock<std::mutex>(gSolarMutex)
+        : lock(gSolarMutex)
     {
     }
 };
@@ -25,8 +28,8 @@ namespace test1
 struct Foo
 {
     int m_foo;
-    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
     int bar1()
+    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
     {
         SolarMutexGuard guard;
         return 1;
@@ -42,19 +45,22 @@ struct Foo
 
 namespace test2
 {
+int free_function() { return 1; }
+
 struct Foo
 {
     std::mutex m_aMutex;
+    osl::Mutex m_aOslMutex;
     int m_foo;
 
-    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
     int bar1()
+    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
     {
         std::unique_lock guard(m_aMutex);
         return 1;
     }
-    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
     int bar2()
+    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
     {
         std::scoped_lock guard(m_aMutex);
         return 1;
@@ -65,7 +71,44 @@ struct Foo
         std::scoped_lock guard(m_aMutex);
         return m_foo;
     }
+    int bar4()
+    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
+    {
+        ::osl::Guard<::osl::Mutex> aGuard(m_aOslMutex);
+        return 1;
+    }
+    int bar5()
+    {
+        // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
+        {
+            std::unique_lock guard(m_aMutex);
+            return free_function();
+        }
+    }
+    osl::Mutex& getOslMutex() { return m_aOslMutex; }
+    int bar6()
+    // expected-error@+1 {{unnecessary locking [loplugin:unnecessarylocking]}}
+    {
+        ::osl::Guard<::osl::Mutex> aGuard(getOslMutex());
+        return 1;
+    }
 };
 }
 
+// Calling anything on VCLUnoHelper means we need the SolarMutex
+class VCLUnoHelper
+{
+public:
+    static int CreateToolkit();
+};
+namespace test4
+{
+// no warning expected
+void bar1()
+{
+    SolarMutexGuard guard;
+    VCLUnoHelper::CreateToolkit();
+}
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unnecessarylocking.cxx b/compilerplugins/clang/unnecessarylocking.cxx
index c00758b810dc..40b15518571d 100644
--- a/compilerplugins/clang/unnecessarylocking.cxx
+++ b/compilerplugins/clang/unnecessarylocking.cxx
@@ -54,34 +54,28 @@ public:
             TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
     }
 
-    bool TraverseCXXMethodDecl(CXXMethodDecl*);
+    bool VisitCompoundStmt(const CompoundStmt*);
     bool VisitCXXThisExpr(const CXXThisExpr*);
+    bool VisitCallExpr(const CallExpr*);
 
 private:
     bool isSolarMutexLockGuardStmt(const Stmt*);
-    const Stmt* isOtherMutexLockGuardStmt(const Stmt*);
+    const CXXThisExpr* isOtherMutexLockGuardStmt(const Stmt*);
     std::vector<bool> m_TouchesThis;
     // so we ignore the CxxThisEpxr that references the maMutex in the guard expression
-    std::vector<const Stmt*> m_IgnoreThis;
+    std::vector<const CXXThisExpr*> m_IgnoreThis;
 };
 
-bool UnnecessaryLocking::TraverseCXXMethodDecl(CXXMethodDecl* cxxMethodDecl)
+bool UnnecessaryLocking::VisitCompoundStmt(const CompoundStmt* compoundStmt)
 {
-    if (ignoreLocation(cxxMethodDecl))
+    if (ignoreLocation(compoundStmt))
         return true;
-
-    if (!cxxMethodDecl->isInstance())
-        return true;
-    if (!cxxMethodDecl->isThisDeclarationADefinition())
-        return true;
-
-    auto compoundStmt = dyn_cast_or_null<CompoundStmt>(cxxMethodDecl->getBody());
-    if (!compoundStmt || compoundStmt->size() < 1)
+    if (compoundStmt->size() < 1)
         return true;
 
     const Stmt* firstStmt = *compoundStmt->body_begin();
     bool solarMutex = isSolarMutexLockGuardStmt(firstStmt);
-    const Stmt* ignoreThisStmt = nullptr;
+    const CXXThisExpr* ignoreThisStmt = nullptr;
     if (!solarMutex)
         ignoreThisStmt = isOtherMutexLockGuardStmt(firstStmt);
     if (!solarMutex && ignoreThisStmt == nullptr)
@@ -90,48 +84,133 @@ bool UnnecessaryLocking::TraverseCXXMethodDecl(CXXMethodDecl* cxxMethodDecl)
     m_TouchesThis.push_back(false);
     m_IgnoreThis.push_back(ignoreThisStmt);
 
-    bool rv = FilteringPlugin::TraverseCXXMethodDecl(cxxMethodDecl);
+    for (const Stmt* stmt : compoundStmt->body())
+        FilteringPlugin::TraverseStmt(const_cast<Stmt*>(stmt));
 
     if (!m_TouchesThis.back())
     {
         StringRef fn = getFilenameOfLocation(
-            compiler.getSourceManager().getSpellingLoc(cxxMethodDecl->getBeginLoc()));
+            compiler.getSourceManager().getSpellingLoc(compoundStmt->getBeginLoc()));
         if (
             // template magic
             !loplugin::isSamePathname(fn, SRCDIR "/include/comphelper/unique_disposing_ptr.hxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/inc/unobaseclass.hxx")
-            // toolkit needs to lock around access to static methods in vcl
-            && !loplugin::isSamePathname(fn, SRCDIR "/toolkit/source/awt/vclxtoolkit.cxx")
+
+            // false+
+            && !loplugin::isSamePathname(fn, SRCDIR "/cppuhelper/source/component_context.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/toolkit/source/controls/tree/treecontrol.cxx")
             && !loplugin::isSamePathname(fn,
-                                         SRCDIR "/toolkit/source/controls/tree/treecontrolpeer.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/toolkit/source/awt/vclxcontainer.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/toolkit/source/awt/vclxdevice.cxx")
-            // touching shared global data
+                                         SRCDIR "/toolkit/source/helper/listenermultiplexer.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/include/toolkit/helper/macros.hxx")
             && !loplugin::isSamePathname(fn, SRCDIR
-                                         "/framework/source/fwi/classes/protocolhandlercache.cxx")
-            // lock around access to static methods in vcl
-            && !loplugin::isSamePathname(fn, SRCDIR "/framework/source/services/taskcreatorsrv.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/svx/source/dialog/SafeModeUI.cxx")
+                                         "/chart2/source/controller/main/CommandDispatch.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/chart2/source/controller/main/ChartView.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR
-                                         "/svx/source/accessibility/AccessibleFrameSelector.cxx")
+                                         "/chart2/source/controller/main/SelectionHelper.cxx")
+            && !loplugin::isSamePathname(
+                   fn, SRCDIR "/chart2/source/controller/accessibility/AccessibleChartView.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/desktop/source/offacc/acceptor.cxx")
+            && !loplugin::isSamePathname(
+                   fn, SRCDIR "/desktop/source/deployment/registry/component/dp_component.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/desktop/source/deployment/gui/dp_gui_dialog2.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/desktop/source/lib/init.cxx")
+            && !loplugin::isSamePathname(
+                   fn, SRCDIR "/sd/source/ui/slidesorter/cache/SlsCacheConfiguration.cxx")
+
+            // needs to lock around access to methods in vcl
+            && !loplugin::isSamePathname(fn, SRCDIR "/basctl/source/basicide/unomodel.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/cui/source/dialogs/AdditionsDialog.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/cui/source/dialogs/cuigaldlg.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/dbaccess/source/ui/browser/unodatbr.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/dbaccess/source/ui/uno/dbinteraction.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/dbaccess/source/ui/dlg/DbAdminImpl.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/dbaccess/source/ui/misc/UITools.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/desktop/source/lib/lokclipboard.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/editeng/source/misc/unolingu.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR
+                                         "/framework/source/uielement/popuptoolbarcontroller.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/framework/source/uielement/newmenucontroller.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/framework/source/uielement/menubarwrapper.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/framework/source/services/desktop.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/framework/source/layoutmanager/layoutmanager.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR
+                                         "/framework/source/layoutmanager/toolbarlayoutmanager.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR
+                                         "/framework/source/fwe/helper/actiontriggerhelper.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/unodoc.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/filtuno.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/funcuno.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/vba/vbaapplication.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sd/source/ui/unoidl/unodoc.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sd/source/ui/unoidl/unomodule.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sd/source/ui/remotecontrol/Receiver.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR
+                                         "/sd/source/ui/slidesorter/controller/SlsClipboard.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/appl/fwkhelper.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/appl/appinit.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/appl/shutdownicon.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/dialog/dockwin.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/statbar/stbitem.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/toolbox/tbxitem.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/svtools/source/java/javainteractionhandler.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/svx/source/accessibility/ShapeTypeHandler.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/svx/source/tbxctrls/tbunosearchcontrollers.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/svx/source/form/fmscriptingenv.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/svx/source/fmcomp/fmgridif.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/toolkit/source/awt/vclxspinbutton.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/toolkit/source/awt/vclxtoolkit.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/toolkit/source/controls/tree/treecontrolpeer.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/ucb/source/ucp/image/ucpimage.cxx")
+            && !loplugin::hasPathnamePrefix(fn, SRCDIR "/vcl/")
+
             // not sure
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/dbaccess/source/ui/browser/AsynchronousLink.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/framework/source/services/autorecovery.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/dialog/filedlghelper.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/appl/appdispatchprovider.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/ucb/source/ucp/tdoc/tdoc_storage.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/core/data/poolhelp.cxx")
+
             // touching shared global data
+            && !loplugin::isSamePathname(fn, SRCDIR
+                                         "/framework/source/fwi/classes/protocolhandlercache.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/basic/source/basmgr/basicmanagerrepository.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/basic/source/classes/sb.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/docuno.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/afmtuno.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/appluno.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/vba/vbaapplication.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/access/accdoc.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/access/acccontext.cxx")
+            && !loplugin::isSamePathname(fn,
+                                         SRCDIR "/sw/source/core/bastyp/proofreadingiterator.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unoftn.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unolinebreak.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unoobj.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unorefmk.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unotbl.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/filter/xml/xmltexti.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/uibase/uno/dlelstnr.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/access/accdoc.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/access/acccontext.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unocontentcontrol.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unobkm.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/docuno.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/afmtuno.cxx")
-            && !loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/unoobj/appluno.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unocoll.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/unocore/unostyle.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/filter/xml/xmltexti.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/uibase/uno/dlelstnr.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/uibase/uno/unoatxt.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/uibase/uno/unodoc.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/uibase/uno/unomodule.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/uibase/uno/SwXFilterOptions.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/uibase/shells/translatehelper.cxx")
+            && !loplugin::isSamePathname(fn, SRCDIR "/sw/source/ui/vba/vbaapplication.cxx")
             && !loplugin::isSamePathname(fn, SRCDIR "/unoxml/source/dom/documentbuilder.cxx")
             && !loplugin::isSamePathname(
                    fn, SRCDIR "/sd/source/ui/accessibility/AccessibleDrawDocumentView.cxx")
@@ -139,15 +218,15 @@ bool UnnecessaryLocking::TraverseCXXMethodDecl(CXXMethodDecl* cxxMethodDecl)
             && !loplugin::isSamePathname(fn,
                                          SRCDIR "/starmath/source/AccessibleSmElementsControl.cxx"))
         {
-            report(DiagnosticsEngine::Warning, "unnecessary locking", cxxMethodDecl->getBeginLoc())
-                << cxxMethodDecl->getSourceRange();
+            report(DiagnosticsEngine::Warning, "unnecessary locking", compoundStmt->getBeginLoc())
+                << compoundStmt->getSourceRange();
         }
     }
 
     m_TouchesThis.pop_back();
     m_IgnoreThis.pop_back();
 
-    return rv;
+    return true;
 }
 
 bool UnnecessaryLocking::isSolarMutexLockGuardStmt(const Stmt* stmt)
@@ -169,7 +248,7 @@ bool UnnecessaryLocking::isSolarMutexLockGuardStmt(const Stmt* stmt)
     return true;
 }
 
-const Stmt* UnnecessaryLocking::isOtherMutexLockGuardStmt(const Stmt* stmt)
+const CXXThisExpr* UnnecessaryLocking::isOtherMutexLockGuardStmt(const Stmt* stmt)
 {
     auto declStmt = dyn_cast<DeclStmt>(stmt);
     if (!declStmt)
@@ -180,16 +259,25 @@ const Stmt* UnnecessaryLocking::isOtherMutexLockGuardStmt(const Stmt* stmt)
     if (!varDecl)
         return nullptr;
     auto tc = loplugin::TypeCheck(varDecl->getType());
-    if (!tc.Class("unique_lock").StdNamespace() && !tc.Class("scoped_lock").StdNamespace())
+    if (!tc.Class("unique_lock").StdNamespace() && !tc.Class("scoped_lock").StdNamespace()
+        && !tc.Class("Guard") && !tc.Class("ClearableGuard") && !tc.Class("ResettableGuard"))
         return nullptr;
     auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(varDecl->getInit());
     if (!cxxConstructExpr || cxxConstructExpr->getNumArgs() < 1)
         return nullptr;
-    auto memberExpr = dyn_cast<MemberExpr>(cxxConstructExpr->getArg(0));
-    if (!memberExpr)
-        return nullptr;
-    auto thisStmt = memberExpr->getBase();
-    return thisStmt;
+    auto arg0 = cxxConstructExpr->getArg(0);
+    if (auto memberExpr = dyn_cast<MemberExpr>(arg0))
+    {
+        const CXXThisExpr* thisStmt
+            = dyn_cast<CXXThisExpr>(memberExpr->getBase()->IgnoreImplicit());
+        return thisStmt;
+    }
+    else if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(arg0))
+    {
+        return dyn_cast_or_null<CXXThisExpr>(
+            memberCallExpr->getImplicitObjectArgument()->IgnoreImplicit());
+    }
+    return nullptr;
 }
 
 bool UnnecessaryLocking::VisitCXXThisExpr(const CXXThisExpr* cxxThisExpr)
@@ -208,6 +296,25 @@ bool UnnecessaryLocking::VisitCXXThisExpr(const CXXThisExpr* cxxThisExpr)
     return true;
 }
 
+bool UnnecessaryLocking::VisitCallExpr(const CallExpr* callExpr)
+{
+    if (ignoreLocation(callExpr))
+        return true;
+    // just in case
+    if (m_TouchesThis.empty())
+        return true;
+    // already found something
+    if (m_TouchesThis.back())
+        return true;
+    const CXXMethodDecl* callee = dyn_cast_or_null<CXXMethodDecl>(callExpr->getDirectCallee());
+    if (!callee)
+        return true;
+    auto dc = loplugin::DeclCheck(callee->getParent());
+    if (dc.Class("VCLUnoHelper") || dc.Class("Application"))
+        m_TouchesThis.back() = true;
+    return true;
+}
+
 /** off by default because each warning needs to be carefully inspected */
 loplugin::Plugin::Registration<UnnecessaryLocking> unnecessarylocking("unnecessarylocking", false);
 }