compilerplugins/clang/test/weakobject.cxx | 31 ++++
compilerplugins/clang/weakobject.cxx | 189 +++++++--------------------
solenv/CompilerTest_compilerplugins_clang.mk | 1
3 files changed, 85 insertions(+), 136 deletions(-)
New commits:
commit 81697ca63d47decf165420c4373b2b53d7eab385
Author: Noel Grandin <noel.gran...@collabora.co.uk>
AuthorDate: Tue Jul 13 15:50:35 2021 +0200
Commit: Noel Grandin <noel.gran...@collabora.co.uk>
CommitDate: Wed Jul 14 14:39:39 2021 +0200
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.gran...@collabora.co.uk>
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
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk
b/solenv/CompilerTest_compilerplugins_clang.mk
index d3131efd0e7e..89bb3058f4a7 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -113,6 +113,7 @@ $(eval $(call
gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
compilerplugins/clang/test/useuniqueptr \
compilerplugins/clang/test/vclwidgets \
compilerplugins/clang/test/weakbase \
+ compilerplugins/clang/test/weakobject \
compilerplugins/clang/test/writeonlyvars \
compilerplugins/clang/test/xmlimport \
))
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
