new loplugin:weakobject

find classes with more than one copy of OWeakObject in their inheritance
hierarchy, which is dodgy

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

2 files changed, 84 insertions, 136 deletions

diff --git a/compilerplugins/clang/test/weakobject.cxx b/compilerplugins/clang/test/weakobject.cxx
new file mode 100644
index 000000000000..7c7da55664d2
--- /dev/null
+++ b/compilerplugins/clang/test/weakobject.cxx
@@ -0,0 +1,31 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include "config_clang.h"
+
+namespace cppu
+{
+class OWeakObject
+{
+};
+}
+
+class Foo1 : public cppu::OWeakObject
+{
+};
+class Foo2 : public cppu::OWeakObject
+{
+};
+
+// expected-error@+1 {{more than one copy of cppu::OWeakObject inherited [loplugin:weakobject]}}
+class Foo3 : public Foo1, public Foo2
+{
+};
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/weakobject.cxx b/compilerplugins/clang/weakobject.cxx
index 055110ef9331..4801953cc44a 100644
--- a/compilerplugins/clang/weakobject.cxx
+++ b/compilerplugins/clang/weakobject.cxx
@@ -2,163 +2,80 @@
 /*
  * This file is part of the LibreOffice project.
  *
- * This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ * Based on LLVM/Clang.
+ *
+ * This file is distributed under the University of Illinois Open Source
+ * License. See LICENSE.TXT for details.
+ *
  */
-
 #ifndef LO_CLANG_SHARED_PLUGINS
 
-#include "check.hxx"
+#include <cassert>
+#include <string>
+#include <iostream>
+#include <fstream>
+#include <set>
+#include <unordered_set>
 #include "plugin.hxx"
+#include "check.hxx"
 
-/* OWeakObject::release() disposes weak references.  If that isn't done
- * because a sub-class improperly overrides release() then
- * OWeakConnectionPoint::m_pObject continues to point to the deleted object
- * and that could result in use-after-free.
- */
-
-namespace {
+/*
+Check for places where we end up with more than one copy of cppu::OweakObject in a class, which
+really should not happen - we should be using one of the AggImplInheritanceHelper classes then
+to inherit.
+*/
 
-class WeakObject
-    : public loplugin::FilteringPlugin<WeakObject>
+namespace
+{
+class WeakObject : public loplugin::FilteringPlugin<WeakObject>
 {
-
 public:
-    explicit WeakObject(loplugin::InstantiationData const& rData): FilteringPlugin(rData)
-    {}
-
-    virtual bool preRun() override {
-        return compiler.getLangOpts().CPlusPlus; // no OWeakObject in C
-    }
-    void run() override {
-        if( preRun()) {
-            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
-        }
+    explicit WeakObject(loplugin::InstantiationData const& data)
+        : FilteringPlugin(data)
+    {
     }
 
-    bool isDerivedFromOWeakObject(CXXMethodDecl const*const pMethodDecl)
+    virtual bool preRun() override
     {
-        QualType const pClass(pMethodDecl->getParent()->getTypeForDecl(), 0);
-        if (loplugin::TypeCheck(pClass).Class("OWeakObject").Namespace("cppu"))
-        {
-            return true;
-        }
-        // hopefully it's faster to recurse overridden methods than the
-        // thicket of WeakImplHelper32 but that is purely speculation
-        for (auto it = pMethodDecl->begin_overridden_methods();
-             it != pMethodDecl->end_overridden_methods(); ++it)
-        {
-            if (isDerivedFromOWeakObject(*it))
-            {
-                return true;
-            }
-        }
-        return false;
+        return true;
     }
 
-    bool VisitCXXMethodDecl(CXXMethodDecl const*const pMethodDecl)
+    virtual void run() override
     {
-        if (ignoreLocation(pMethodDecl)) {
-            return true;
-        }
-        if (!pMethodDecl->isThisDeclarationADefinition()
-            || pMethodDecl->isLateTemplateParsed())
-        {
-            return true;
-        }
-        if (!pMethodDecl->isInstance()) {
-            return true;
-        }
-// this is too "simple", if a NamedDecl class has a getName() member expecting it to actually work would clearly be unreasonable    if (pMethodDecl->getName() != "release") {
-        if (auto const i = pMethodDecl->getIdentifier()) {
-            if (i != nullptr && !i->isStr("release")) {
-                return true;
-            }
-        }
-        if (pMethodDecl->getNumParams() != 0) {
-            return true;
-        }
-        if (loplugin::TypeCheck(QualType(pMethodDecl->getParent()->getTypeForDecl(), 0)).Class("OWeakObject").Namespace("cppu"))
-        {
-            return true;
-        }
+        if (preRun())
+            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+    }
 
-        CXXMethodDecl const* pOverridden(nullptr);
-        for (auto it = pMethodDecl->begin_overridden_methods();
-             it != pMethodDecl->end_overridden_methods(); ++it)
-        {
-            if (auto const i = (*it)->getIdentifier()) {
-                if (i != nullptr && i->isStr("release")) {
-                    pOverridden = *it;
-                    break;
-                }
-            }
-        }
-        if (pOverridden == nullptr)
-        {
-            return true;
-        }
-        if (!isDerivedFromOWeakObject(pOverridden))
-        {
-            return true;
-        }
-        CompoundStmt const*const pCompoundStatement(
-                dyn_cast<CompoundStmt>(pMethodDecl->getBody()));
-        for (auto i = pCompoundStatement->body_begin();
-             i != pCompoundStatement->body_end(); ++i)
-        {
-            // note: this is not a CXXMemberCallExpr
-            CallExpr const*const pCallExpr(dyn_cast<CallExpr>(*i));
-            if (pCallExpr)
-            {
-                // note: this is only sometimes a CXXMethodDecl
-                FunctionDecl const*const pCalled(pCallExpr->getDirectCallee());
-                if (pCalled->getName() == "release")
-                {
-//this never works  && pCalled == pOverridden
-                    if (pCalled->getParent() == pOverridden->getParent())
-                    {
-                        return true;
-                    }
-                    // Allow this convenient shortcut:
-                    auto td = dyn_cast<TypeDecl>(pCalled->getParent());
-                    if (td != nullptr
-                        && (loplugin::TypeCheck(td).Class("OWeakObject").Namespace("cppu").GlobalNamespace()
-                            || loplugin::TypeCheck(td).Class("OWeakAggObject").Namespace("cppu").GlobalNamespace()))
-                    {
-                        return true;
-                    }
-                }
-            }
-        }
+    bool VisitCXXRecordDecl( const CXXRecordDecl* decl);
 
-        // allowlist
-        auto tc  = loplugin::TypeCheck(pMethodDecl->getParent());
-        if (   tc.Class("OWeakAggObject").Namespace("cppu").GlobalNamespace() // conditional call
-            || tc.Class("WeakComponentImplHelperBase").Namespace("cppu").GlobalNamespace() // extra magic
-            || tc.Class("WeakAggComponentImplHelperBase").Namespace("cppu").GlobalNamespace() // extra magic
-            || tc.Class("CDOMImplementation").Namespace("DOM").GlobalNamespace() // a static oddity
-            || tc.Class("SwXTextFrame").GlobalNamespace() // ambiguous, 3 parents
-            || tc.Class("SwXTextDocument").GlobalNamespace() // ambiguous, ~4 parents
-            || tc.Class("SdStyleSheet").GlobalNamespace() // same extra magic as WeakComponentImplHelperBase
-            || tc.Class("SdXImpressDocument").GlobalNamespace() // same extra magic as WeakComponentImplHelperBase
-           )
+};
+
+bool WeakObject::VisitCXXRecordDecl(const CXXRecordDecl* decl)
+{
+    if (ignoreLocation(decl))
+        return true;
+    if (!decl->hasDefinition())
+        return true;
+    if (decl->hasAnyDependentBases())
+        return true;
+    int cnt = 0;
+    decl->forallBases(
+        [&cnt] (const CXXRecordDecl *BaseDefinition) -> bool
         {
+            if (loplugin::DeclCheck(BaseDefinition).Class("OWeakObject").Namespace("cppu").GlobalNamespace())
+                ++cnt;
             return true;
-        }
-
-        report(DiagnosticsEngine::Warning,
-                "override of OWeakObject::release() does not call superclass release()",
-                pMethodDecl->getLocation())
-            << pMethodDecl->getSourceRange();
-
+        });
+    if (cnt < 2)
         return true;
-    }
 
-};
+    report(DiagnosticsEngine::Warning, "more than one copy of cppu::OWeakObject inherited",
+            compat::getBeginLoc(decl))
+        << decl->getSourceRange();
+    return true;
+}
 
-loplugin::Plugin::Registration<WeakObject> weakobject("weakobject");
+loplugin::Plugin::Registration<WeakObject> weakobject("weakobject", false);
 
 } // namespace