https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92501
>From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Thu, 16 May 2024 23:24:13 -0700
Subject: [PATCH 01/10] [webkit.RefCntblBaseVirtualDtor] Ignore a base class
which has a specialized deref function for a given derived class.
When a base class B has a deref function which calls delete operator on a
derived class D,
don't emit a warning for B even if it did not have a virtual destructor.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 ++++++++++++-
.../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +++++++++++++++++-
2 files changed, 330 insertions(+), 6 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 7f4c3a7b787e8..36ab581f1edbd 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -11,16 +11,128 @@
#include "PtrTypesSemantics.h"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
#include <optional>
using namespace clang;
using namespace ento;
namespace {
+
+class DerefAnalysisVisitor
+ : public ConstStmtVisitor<DerefAnalysisVisitor, bool> {
+ // Returns true if any of child statements return true.
+ bool VisitChildren(const Stmt *S) {
+ for (const Stmt *Child : S->children()) {
+ if (Child && Visit(Child))
+ return true;
+ }
+ return false;
+ }
+
+ bool VisitBody(const Stmt *Body) {
+ if (!Body)
+ return false;
+
+ auto [It, IsNew] = VisitedBody.insert(Body);
+ if (!IsNew) // This body is recursive
+ return false;
+
+ return Visit(Body);
+ }
+
+public:
+ DerefAnalysisVisitor(const TemplateArgumentList &ArgList,
+ const CXXRecordDecl *ClassDecl)
+ : ArgList(&ArgList)
+ , ClassDecl(ClassDecl)
+ { }
+
+ DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl)
+ : ClassDecl(ClassDecl)
+ { }
+
+ bool HasSpecializedDelete(CXXMethodDecl *Decl) {
+ if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
+ return VisitBody(Tmpl->getBody());
+ return VisitBody(Decl->getBody());
+ }
+
+ bool VisitCallExpr(const CallExpr *CE) {
+ auto *Callee = CE->getCallee();
+ while (auto *Expr = dyn_cast<CastExpr>(Callee))
+ Callee = Expr->getSubExpr();
+ if (auto *DeclRef = dyn_cast<DeclRefExpr>(Callee)) {
+ auto *Decl = DeclRef->getDecl();
+ if (auto *VD = dyn_cast<VarDecl>(Decl)) {
+ if (auto *Init = VD->getInit()) {
+ if (auto *Lambda = dyn_cast<LambdaExpr>(Init))
+ return VisitBody(Lambda->getBody());
+ }
+ } else if (auto *FD = dyn_cast<FunctionDecl>(Decl))
+ return VisitBody(FD->getBody());
+ }
+ return false;
+ }
+
+ bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
+ auto *Callee = MCE->getMethodDecl();
+ if (!Callee)
+ return false;
+ return VisitBody(Callee->getBody());
+ }
+
+ bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) {
+ auto *Arg = E->getArgument();
+ while (Arg) {
+ if (auto *Paren = dyn_cast<ParenExpr>(Arg))
+ Arg = Paren->getSubExpr();
+ else if (auto *Cast = dyn_cast<ExplicitCastExpr>(Arg)) {
+ Arg = Cast->getSubExpr();
+ auto CastType = Cast->getType();
+ if (auto *PtrType = dyn_cast<PointerType>(CastType)) {
+ auto PointeeType = PtrType->getPointeeType();
+ while (auto *ET = dyn_cast<ElaboratedType>(PointeeType)) {
+ if (ET->isSugared())
+ PointeeType = ET->desugar();
+ }
+ if (auto *ParmType = dyn_cast<TemplateTypeParmType>(PointeeType)) {
+ if (ArgList) {
+ auto ParmIndex = ParmType->getIndex();
+ auto Type = ArgList->get(ParmIndex).getAsType();
+ if (auto *RD = dyn_cast<RecordType>(Type)) {
+ if (RD->getDecl() == ClassDecl)
+ return true;
+ }
+ }
+ } else if (auto *RD = dyn_cast<RecordType>(PointeeType)) {
+ if (RD->getDecl() == ClassDecl)
+ return true;
+ }
+ }
+ } else
+ break;
+ }
+ return false;
+ }
+
+ bool VisitStmt(const Stmt *S) { return VisitChildren(S); }
+
+ // Return false since the contents of lambda isn't necessarily executed.
+ // If it is executed, VisitCallExpr above will visit its body.
+ bool VisitLambdaExpr(const LambdaExpr *) { return false; }
+
+private:
+ const TemplateArgumentList* ArgList { nullptr };
+ const CXXRecordDecl* ClassDecl;
+ llvm::DenseSet<const Stmt *> VisitedBody;
+};
+
class RefCntblBaseVirtualDtorChecker
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
private:
@@ -91,8 +203,6 @@ class RefCntblBaseVirtualDtorChecker
const CXXRecordDecl *C = T->getAsCXXRecordDecl();
if (!C)
return false;
- if (isRefCountedClass(C))
- return false;
bool AnyInconclusiveBase = false;
const auto hasPublicRefInBase =
@@ -124,6 +234,9 @@ class RefCntblBaseVirtualDtorChecker
if (AnyInconclusiveBase || !hasRef || !hasDeref)
return false;
+ if (isClassWithSpecializedDelete(C, RD))
+ return false;
+
const auto *Dtor = C->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
ProblematicBaseSpecifier = Base;
@@ -182,6 +295,29 @@ class RefCntblBaseVirtualDtorChecker
ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
}
+ static bool isClassWithSpecializedDelete(const CXXRecordDecl *C,
+ const CXXRecordDecl *DerivedClass) {
+ if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+ for (auto *MethodDecl : C->methods()) {
+ if (safeGetName(MethodDecl) == "deref") {
+ DerefAnalysisVisitor DerefAnalysis(ClsTmplSpDecl->getTemplateArgs(),
+ DerivedClass);
+ if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
+ return true;
+ }
+ }
+ return false;
+ }
+ for (auto *MethodDecl : C->methods()) {
+ if (safeGetName(MethodDecl) == "deref") {
+ DerefAnalysisVisitor DerefAnalysis(DerivedClass);
+ if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
+ return true;
+ }
+ }
+ return false;
+ }
+
void reportBug(const CXXRecordDecl *DerivedClass,
const CXXBaseSpecifier *BaseSpec,
const CXXRecordDecl *ProblematicBaseClass) const {
diff --git
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index eeb62d5d89ec4..37f5d46110896 100644
---
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -58,24 +58,101 @@ class RefCounted : public RefCountedBase {
RefCounted() { }
};
+template <typename X, typename T>
+class ExoticRefCounted : RefCountedBase {
+public:
+ void deref() const {
+ if (derefBase())
+ delete (const_cast<T*>(static_cast<const T*>(this)));
+ }
+};
+
+template <typename X, typename T>
+class BadBase : RefCountedBase {
+public:
+ void deref() const {
+ if (derefBase())
+ delete (const_cast<X*>(static_cast<const X*>(this)));
+ }
+};
+
+template <typename T>
+class FancyDeref {
+public:
+ void ref() const
+ {
+ ++refCount;
+ }
+
+ void deref() const
+ {
+ --refCount;
+ if (refCount)
+ return;
+ auto deleteThis = [this] {
+ delete static_cast<const T*>(this);
+ };
+ deleteThis();
+ }
+private:
+ mutable unsigned refCount { 0 };
+};
+
+template <typename T>
+class BadFancyDeref {
+public:
+ void ref() const
+ {
+ ++refCount;
+ }
+
+ void deref() const
+ {
+ --refCount;
+ if (refCount)
+ return;
+ auto deleteThis = [this] {
+ delete static_cast<const T*>(this);
+ };
+ delete this;
+ }
+private:
+ mutable unsigned refCount { 0 };
+};
+
template <typename T>
class ThreadSafeRefCounted {
public:
- void ref() const;
- bool deref() const;
+ void ref() const { ++refCount; }
+ void deref() const {
+ if (!--refCount)
+ delete const_cast<T*>(static_cast<const T*>(this));
+ }
+private:
+ mutable unsigned refCount { 0 };
};
template <typename T>
class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
public:
- void ref() const;
- bool deref() const;
+ void ref() const { ++refCount; }
+ void deref() const {
+ if (!--refCount)
+ delete const_cast<T*>(static_cast<const T*>(this));
+ }
+private:
+ mutable unsigned refCount { 0 };
};
} // namespace WTF
class DerivedClass4 : public WTF::RefCounted<DerivedClass4> { };
+class DerivedClass4b : public WTF::ExoticRefCounted<int, DerivedClass4b> { };
+
+class DerivedClass4c : public WTF::BadBase<int, DerivedClass4c> { };
+// expected-warning@-1{{Class 'WTF::BadBase<int, DerivedClass4c>' is used as a
base of class 'DerivedClass4c' but doesn't have virtual destructor}}
+
class DerivedClass5 : public DerivedClass4 { };
// expected-warning@-1{{Class 'DerivedClass4' is used as a base of class
'DerivedClass5' but doesn't have virtual destructor}}
@@ -88,3 +165,114 @@ class DerivedClass8 : public
WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPt
class DerivedClass9 : public DerivedClass8 { };
// expected-warning@-1{{Class 'DerivedClass8' is used as a base of class
'DerivedClass9' but doesn't have virtual destructor}}
+
+class DerivedClass10 : public WTF::FancyDeref<DerivedClass10> { };
+
+class DerivedClass10b : public WTF::BadFancyDeref<DerivedClass10b> { };
+// expected-warning@-1{{Class 'WTF::BadFancyDeref<DerivedClass10b>' is used as
a base of class 'DerivedClass10b' but doesn't have virtual destructor}}
+
+class BaseClass1 {
+public:
+ void ref() const { ++refCount; }
+ void deref() const;
+private:
+ enum class Type { Base, Derived } type { Type::Base };
+ mutable unsigned refCount { 0 };
+};
+
+class DerivedClass11 : public BaseClass1 { };
+
+void BaseClass1::deref() const
+{
+ --refCount;
+ if (refCount)
+ return;
+ switch (type) {
+ case Type::Base:
+ delete const_cast<BaseClass1*>(this);
+ break;
+ case Type::Derived:
+ delete const_cast<DerivedClass11*>(static_cast<const
DerivedClass11*>(this));
+ break;
+ }
+}
+
+class BaseClass2;
+static void deleteBase2(BaseClass2*);
+
+class BaseClass2 {
+public:
+ void ref() const { ++refCount; }
+ void deref() const
+ {
+ if (!--refCount)
+ deleteBase2(const_cast<BaseClass2*>(this));
+ }
+ virtual bool isDerived() { return false; }
+private:
+ mutable unsigned refCount { 0 };
+};
+
+class DerivedClass12 : public BaseClass2 {
+ bool isDerived() final { return true; }
+};
+
+void deleteBase2(BaseClass2* obj) {
+ if (obj->isDerived())
+ delete static_cast<DerivedClass12*>(obj);
+ else
+ delete obj;
+}
+
+class BaseClass3 {
+public:
+ void ref() const { ++refCount; }
+ void deref() const
+ {
+ if (!--refCount)
+ const_cast<BaseClass3*>(this)->destory();
+ }
+ virtual bool isDerived() { return false; }
+
+private:
+ void destory();
+
+ mutable unsigned refCount { 0 };
+};
+
+class DerivedClass13 : public BaseClass3 {
+ bool isDerived() final { return true; }
+};
+
+void BaseClass3::destory() {
+ if (isDerived())
+ delete static_cast<DerivedClass13*>(this);
+ else
+ delete this;
+}
+
+class RecursiveBaseClass {
+public:
+ void ref() const {
+ if (otherObject)
+ otherObject->ref();
+ else
+ ++refCount;
+ }
+ void deref() const {
+ if (otherObject)
+ otherObject->deref();
+ else {
+ --refCount;
+ if (refCount)
+ return;
+ delete this;
+ }
+ }
+private:
+ RecursiveBaseClass* otherObject { nullptr };
+ mutable unsigned refCount { 0 };
+};
+
+class RecursiveDerivedClass : public RecursiveBaseClass { };
+// expected-warning@-1{{Class 'RecursiveBaseClass' is used as a base of class
'RecursiveDerivedClass' but doesn't have virtual destructor}}
>From c538cd4b291e550b830c2c450e095638f6785452 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Thu, 16 May 2024 23:37:40 -0700
Subject: [PATCH 02/10] Fix formatting.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 22 ++++++++-----------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 36ab581f1edbd..92c1c6ea75263 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -49,13 +49,9 @@ class DerefAnalysisVisitor
public:
DerefAnalysisVisitor(const TemplateArgumentList &ArgList,
const CXXRecordDecl *ClassDecl)
- : ArgList(&ArgList)
- , ClassDecl(ClassDecl)
- { }
+ : ArgList(&ArgList) , ClassDecl(ClassDecl) {}
- DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl)
- : ClassDecl(ClassDecl)
- { }
+ DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl)
{}
bool HasSpecializedDelete(CXXMethodDecl *Decl) {
if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
@@ -128,8 +124,8 @@ class DerefAnalysisVisitor
bool VisitLambdaExpr(const LambdaExpr *) { return false; }
private:
- const TemplateArgumentList* ArgList { nullptr };
- const CXXRecordDecl* ClassDecl;
+ const TemplateArgumentList *ArgList{nullptr};
+ const CXXRecordDecl *ClassDecl;
llvm::DenseSet<const Stmt *> VisitedBody;
};
@@ -309,11 +305,11 @@ class RefCntblBaseVirtualDtorChecker
return false;
}
for (auto *MethodDecl : C->methods()) {
- if (safeGetName(MethodDecl) == "deref") {
- DerefAnalysisVisitor DerefAnalysis(DerivedClass);
- if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
- return true;
- }
+ if (safeGetName(MethodDecl) == "deref") {
+ DerefAnalysisVisitor DerefAnalysis(DerivedClass);
+ if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
+ return true;
+ }
}
return false;
}
>From 97dfd40cf89901411ea6f21a0f9b7492f184ac39 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Thu, 16 May 2024 23:50:58 -0700
Subject: [PATCH 03/10] Fix formatting 2.
---
.../Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 92c1c6ea75263..17e5514850b21 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -49,7 +49,7 @@ class DerefAnalysisVisitor
public:
DerefAnalysisVisitor(const TemplateArgumentList &ArgList,
const CXXRecordDecl *ClassDecl)
- : ArgList(&ArgList) , ClassDecl(ClassDecl) {}
+ : ArgList(&ArgList), ClassDecl(ClassDecl) {}
DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl)
{}
>From 3e4d9495a1e52c04620036da026727655c65c86f Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Fri, 17 May 2024 01:54:21 -0700
Subject: [PATCH 04/10] [webkit.RefCntblBaseVirtualDtor] Only check immediate
base class
This PR changes RefCntblBaseVirtualDtor checker so that the checker will only
check
if a given class's immediate ref-counted base class has a virtual destructor,
not every ref-counted ancestor class.
This is sufficient because the checker will eventually see every class
declaration,
and transitively proves every ref-counted base class has a virtual destructor or
deref function which casts this pointer back to the derived class before
deleting.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 119 ++++++++----------
.../ref-cntbl-base-virtual-dtor-templates.cpp | 6 +
2 files changed, 58 insertions(+), 67 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 17e5514850b21..452d2b4a14a7e 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -172,80 +172,65 @@ class RefCntblBaseVirtualDtorChecker
if (shouldSkipDecl(RD))
return;
- CXXBasePaths Paths;
- Paths.setOrigin(RD);
-
- const CXXBaseSpecifier *ProblematicBaseSpecifier = nullptr;
- const CXXRecordDecl *ProblematicBaseClass = nullptr;
-
- const auto IsPublicBaseRefCntblWOVirtualDtor =
- [RD, &ProblematicBaseSpecifier,
- &ProblematicBaseClass](const CXXBaseSpecifier *Base, CXXBasePath &) {
- const auto AccSpec = Base->getAccessSpecifier();
- if (AccSpec == AS_protected || AccSpec == AS_private ||
- (AccSpec == AS_none && RD->isClass()))
- return false;
-
- auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
- auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
-
- bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
- bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
-
- QualType T = Base->getType();
- if (T.isNull())
- return false;
-
- const CXXRecordDecl *C = T->getAsCXXRecordDecl();
- if (!C)
- return false;
-
- bool AnyInconclusiveBase = false;
- const auto hasPublicRefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
- CXXBasePath &) {
- auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
- if (!hasRefInBase) {
- AnyInconclusiveBase = true;
- return false;
- }
- return (*hasRefInBase) != nullptr;
- };
- const auto hasPublicDerefInBase = [&AnyInconclusiveBase](
- const CXXBaseSpecifier *Base,
- CXXBasePath &) {
- auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
- if (!hasDerefInBase) {
+ for (auto& Base : RD->bases()) {
+ const auto AccSpec = Base.getAccessSpecifier();
+ if (AccSpec == AS_protected || AccSpec == AS_private ||
+ (AccSpec == AS_none && RD->isClass()))
+ continue;
+
+ auto hasRefInBase = clang::hasPublicMethodInBase(&Base, "ref");
+ auto hasDerefInBase = clang::hasPublicMethodInBase(&Base, "deref");
+
+ bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
+ bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+
+ QualType T = Base.getType();
+ if (T.isNull())
+ continue;
+
+ const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+ if (!C)
+ continue;
+
+ bool AnyInconclusiveBase = false;
+ const auto hasPublicRefInBase =
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
+ CXXBasePath &) {
+ auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+ if (!hasRefInBase) {
AnyInconclusiveBase = true;
return false;
}
- return (*hasDerefInBase) != nullptr;
+ return (*hasRefInBase) != nullptr;
};
- CXXBasePaths Paths;
- Paths.setOrigin(C);
- hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+ const auto hasPublicDerefInBase = [&AnyInconclusiveBase](
+ const CXXBaseSpecifier *Base,
+ CXXBasePath &) {
+ auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+ if (!hasDerefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasDerefInBase) != nullptr;
+ };
+ CXXBasePaths Paths;
+ Paths.setOrigin(C);
+ hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+ /*LookupInDependent =*/true);
+ hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
/*LookupInDependent =*/true);
- hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
- /*LookupInDependent =*/true);
- if (AnyInconclusiveBase || !hasRef || !hasDeref)
- return false;
-
- if (isClassWithSpecializedDelete(C, RD))
- return false;
-
- const auto *Dtor = C->getDestructor();
- if (!Dtor || !Dtor->isVirtual()) {
- ProblematicBaseSpecifier = Base;
- ProblematicBaseClass = C;
- return true;
- }
+ if (AnyInconclusiveBase || !hasRef || !hasDeref)
+ continue;
- return false;
- };
+ if (isClassWithSpecializedDelete(C, RD))
+ continue;
- if (RD->lookupInBases(IsPublicBaseRefCntblWOVirtualDtor, Paths,
- /*LookupInDependent =*/true)) {
- reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+ const auto *Dtor = C->getDestructor();
+ if (!Dtor || !Dtor->isVirtual()) {
+ auto* ProblematicBaseSpecifier = &Base;
+ auto* ProblematicBaseClass = C;
+ reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+ }
}
}
diff --git
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index 37f5d46110896..c42d8520b5f05 100644
---
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -276,3 +276,9 @@ class RecursiveBaseClass {
class RecursiveDerivedClass : public RecursiveBaseClass { };
// expected-warning@-1{{Class 'RecursiveBaseClass' is used as a base of class
'RecursiveDerivedClass' but doesn't have virtual destructor}}
+
+class DerivedClass14 : public WTF::RefCounted<DerivedClass14> {
+public:
+ virtual ~DerivedClass14() { }
+};
+class DerivedClass15 : public DerivedClass14 { };
>From cb956371721c2b573363fb2de00d59535877ca4f Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Fri, 17 May 2024 02:06:32 -0700
Subject: [PATCH 05/10] Fix formatting.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 28 +++++++++----------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 452d2b4a14a7e..e851d1f5d5df9 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -172,7 +172,7 @@ class RefCntblBaseVirtualDtorChecker
if (shouldSkipDecl(RD))
return;
- for (auto& Base : RD->bases()) {
+ for (auto &Base : RD->bases()) {
const auto AccSpec = Base.getAccessSpecifier();
if (AccSpec == AS_protected || AccSpec == AS_private ||
(AccSpec == AS_none && RD->isClass()))
@@ -194,8 +194,7 @@ class RefCntblBaseVirtualDtorChecker
bool AnyInconclusiveBase = false;
const auto hasPublicRefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
- CXXBasePath &) {
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
if (!hasRefInBase) {
AnyInconclusiveBase = true;
@@ -203,16 +202,15 @@ class RefCntblBaseVirtualDtorChecker
}
return (*hasRefInBase) != nullptr;
};
- const auto hasPublicDerefInBase = [&AnyInconclusiveBase](
- const CXXBaseSpecifier *Base,
- CXXBasePath &) {
- auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
- if (!hasDerefInBase) {
- AnyInconclusiveBase = true;
- return false;
- }
- return (*hasDerefInBase) != nullptr;
- };
+ const auto hasPublicDerefInBase =
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+ if (!hasDerefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasDerefInBase) != nullptr;
+ };
CXXBasePaths Paths;
Paths.setOrigin(C);
hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
@@ -227,8 +225,8 @@ class RefCntblBaseVirtualDtorChecker
const auto *Dtor = C->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
- auto* ProblematicBaseSpecifier = &Base;
- auto* ProblematicBaseClass = C;
+ auto *ProblematicBaseSpecifier = &Base;
+ auto *ProblematicBaseClass = C;
reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
}
}
>From 04a17ea95ae1f6ce224c26a1d1b388c1910e03e7 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Fri, 17 May 2024 21:03:04 -0700
Subject: [PATCH 06/10] [webkit.RefCntblBaseVirtualDtor] Fix a bug that
ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr generates a warning.
The bug was caused by DerefAnalysisVisitor attempting to find a delete
expression
within a non-specialized template. Because we could not determine the type of
the object being deleted in such cases, we were erroneously concluding that
deref didn't have a proper delete expression.
Fixed the bug by making DerefAnalysisVisitor::HasSpecializedDelete return
nullopt
when there is no fully specialized instance. Update the tests accordingly.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 42 +++++++++++-------
.../ref-cntbl-base-virtual-dtor-templates.cpp | 44 +++++++++++++++----
2 files changed, 63 insertions(+), 23 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index e851d1f5d5df9..2a5707837726b 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -53,10 +53,12 @@ class DerefAnalysisVisitor
DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl)
{}
- bool HasSpecializedDelete(CXXMethodDecl *Decl) {
+ std::optional<bool> HasSpecializedDelete(CXXMethodDecl *Decl) {
+ if (auto *Body = Decl->getBody())
+ return VisitBody(Body);
if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
- return VisitBody(Tmpl->getBody());
- return VisitBody(Decl->getBody());
+ return std::nullopt; // Indeterminate. There was no concrete instance.
+ return false;
}
bool VisitCallExpr(const CallExpr *CE) {
@@ -88,7 +90,7 @@ class DerefAnalysisVisitor
while (Arg) {
if (auto *Paren = dyn_cast<ParenExpr>(Arg))
Arg = Paren->getSubExpr();
- else if (auto *Cast = dyn_cast<ExplicitCastExpr>(Arg)) {
+ else if (auto *Cast = dyn_cast<CastExpr>(Arg)) {
Arg = Cast->getSubExpr();
auto CastType = Cast->getType();
if (auto *PtrType = dyn_cast<PointerType>(CastType)) {
@@ -109,6 +111,12 @@ class DerefAnalysisVisitor
} else if (auto *RD = dyn_cast<RecordType>(PointeeType)) {
if (RD->getDecl() == ClassDecl)
return true;
+ } else if (auto* ST =
dyn_cast<SubstTemplateTypeParmType>(PointeeType)) {
+ auto Type = ST->getReplacementType();
+ if (auto *RD = dyn_cast<RecordType>(Type)) {
+ if (RD->getDecl() == ClassDecl)
+ return true;
+ }
}
}
} else
@@ -172,7 +180,7 @@ class RefCntblBaseVirtualDtorChecker
if (shouldSkipDecl(RD))
return;
- for (auto &Base : RD->bases()) {
+ for (auto& Base : RD->bases()) {
const auto AccSpec = Base.getAccessSpecifier();
if (AccSpec == AS_protected || AccSpec == AS_private ||
(AccSpec == AS_none && RD->isClass()))
@@ -194,7 +202,8 @@ class RefCntblBaseVirtualDtorChecker
bool AnyInconclusiveBase = false;
const auto hasPublicRefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
+ CXXBasePath &) {
auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
if (!hasRefInBase) {
AnyInconclusiveBase = true;
@@ -220,13 +229,14 @@ class RefCntblBaseVirtualDtorChecker
if (AnyInconclusiveBase || !hasRef || !hasDeref)
continue;
- if (isClassWithSpecializedDelete(C, RD))
+ auto HasSpecializedDelete = isClassWithSpecializedDelete(C, RD);
+ if (!HasSpecializedDelete || *HasSpecializedDelete)
continue;
const auto *Dtor = C->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
- auto *ProblematicBaseSpecifier = &Base;
- auto *ProblematicBaseClass = C;
+ auto* ProblematicBaseSpecifier = &Base;
+ auto* ProblematicBaseClass = C;
reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
}
}
@@ -274,15 +284,16 @@ class RefCntblBaseVirtualDtorChecker
ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
}
- static bool isClassWithSpecializedDelete(const CXXRecordDecl *C,
- const CXXRecordDecl *DerivedClass) {
+ static std::optional<bool> isClassWithSpecializedDelete(
+ const CXXRecordDecl *C, const CXXRecordDecl *DerivedClass) {
if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
for (auto *MethodDecl : C->methods()) {
if (safeGetName(MethodDecl) == "deref") {
DerefAnalysisVisitor DerefAnalysis(ClsTmplSpDecl->getTemplateArgs(),
DerivedClass);
- if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
- return true;
+ auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+ if (!Result || *Result)
+ return Result;
}
}
return false;
@@ -290,8 +301,9 @@ class RefCntblBaseVirtualDtorChecker
for (auto *MethodDecl : C->methods()) {
if (safeGetName(MethodDecl) == "deref") {
DerefAnalysisVisitor DerefAnalysis(DerivedClass);
- if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
- return true;
+ auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+ if (!Result || *Result)
+ return Result;
}
}
return false;
diff --git
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index c42d8520b5f05..f04c68cb641f1 100644
---
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -132,16 +132,36 @@ class ThreadSafeRefCounted {
mutable unsigned refCount { 0 };
};
-template <typename T>
-class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
+class ThreadSafeRefCountedControlBlock {
public:
- void ref() const { ++refCount; }
- void deref() const {
- if (!--refCount)
- delete const_cast<T*>(static_cast<const T*>(this));
+ ThreadSafeRefCountedControlBlock(void* object) : object(object) { }
+ void strongRef() const { ++refCount; }
+ template <typename T>
+ void strongDeref() const {
+ --refCount;
+ if (refCount)
+ return;
+
+ auto deleteObject = [&] {
+ delete static_cast<const T*>(object);
+ delete this;
+ };
+
+ deleteObject();
}
private:
mutable unsigned refCount { 0 };
+ mutable void* object { nullptr };
+};
+
+template <typename T>
+class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
+public:
+ ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr() { }
+ void ref() const { controlBlock.strongRef(); }
+ void deref() const { controlBlock.template strongDeref<T>(); }
+private:
+ ThreadSafeRefCountedControlBlock& controlBlock { *new
ThreadSafeRefCountedControlBlock(static_cast<T*>(this)) };
};
} // namespace WTF
@@ -150,8 +170,12 @@ class DerivedClass4 : public
WTF::RefCounted<DerivedClass4> { };
class DerivedClass4b : public WTF::ExoticRefCounted<int, DerivedClass4b> { };
-class DerivedClass4c : public WTF::BadBase<int, DerivedClass4c> { };
-// expected-warning@-1{{Class 'WTF::BadBase<int, DerivedClass4c>' is used as a
base of class 'DerivedClass4c' but doesn't have virtual destructor}}
+class OtherType;
+class DerivedClass4c : public WTF::BadBase<OtherType, DerivedClass4c> { };
+// expected-warning@-1{{Class 'WTF::BadBase<OtherType, DerivedClass4c>' is
used as a base of class 'DerivedClass4c' but doesn't have virtual destructor}}
+class OtherType : public DerivedClass4c { };
+// expected-warning@-1{{Class 'DerivedClass4c' is used as a base of class
'OtherType' but doesn't have virtual destructor}}
+void UseDerived4c(DerivedClass4c &obj) { obj.deref(); }
class DerivedClass5 : public DerivedClass4 { };
// expected-warning@-1{{Class 'DerivedClass4' is used as a base of class
'DerivedClass5' but doesn't have virtual destructor}}
@@ -163,6 +187,9 @@ class DerivedClass7 : public DerivedClass6 { };
class DerivedClass8 : public
WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<DerivedClass8> { };
+class DerivedClass8b : public
WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<DerivedClass8b> { };
+void UseDerived8b(DerivedClass8b &obj) { obj.deref(); }
+
class DerivedClass9 : public DerivedClass8 { };
// expected-warning@-1{{Class 'DerivedClass8' is used as a base of class
'DerivedClass9' but doesn't have virtual destructor}}
@@ -170,6 +197,7 @@ class DerivedClass10 : public
WTF::FancyDeref<DerivedClass10> { };
class DerivedClass10b : public WTF::BadFancyDeref<DerivedClass10b> { };
// expected-warning@-1{{Class 'WTF::BadFancyDeref<DerivedClass10b>' is used as
a base of class 'DerivedClass10b' but doesn't have virtual destructor}}
+void UseDerived10b(DerivedClass10b &obj) { obj.deref(); }
class BaseClass1 {
public:
>From 2ee2bdff5ab887d5cbf700890178f609d39d7333 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Fri, 17 May 2024 21:31:05 -0700
Subject: [PATCH 07/10] Fix formatting.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 2a5707837726b..e565b2323063c 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -111,7 +111,8 @@ class DerefAnalysisVisitor
} else if (auto *RD = dyn_cast<RecordType>(PointeeType)) {
if (RD->getDecl() == ClassDecl)
return true;
- } else if (auto* ST =
dyn_cast<SubstTemplateTypeParmType>(PointeeType)) {
+ } else if (auto *ST =
+ dyn_cast<SubstTemplateTypeParmType>(PointeeType)) {
auto Type = ST->getReplacementType();
if (auto *RD = dyn_cast<RecordType>(Type)) {
if (RD->getDecl() == ClassDecl)
@@ -180,7 +181,7 @@ class RefCntblBaseVirtualDtorChecker
if (shouldSkipDecl(RD))
return;
- for (auto& Base : RD->bases()) {
+ for (auto &Base : RD->bases()) {
const auto AccSpec = Base.getAccessSpecifier();
if (AccSpec == AS_protected || AccSpec == AS_private ||
(AccSpec == AS_none && RD->isClass()))
@@ -202,8 +203,7 @@ class RefCntblBaseVirtualDtorChecker
bool AnyInconclusiveBase = false;
const auto hasPublicRefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
- CXXBasePath &) {
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
if (!hasRefInBase) {
AnyInconclusiveBase = true;
@@ -235,8 +235,8 @@ class RefCntblBaseVirtualDtorChecker
const auto *Dtor = C->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
- auto* ProblematicBaseSpecifier = &Base;
- auto* ProblematicBaseClass = C;
+ auto *ProblematicBaseSpecifier = &Base;
+ auto *ProblematicBaseClass = C;
reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
}
}
@@ -284,8 +284,9 @@ class RefCntblBaseVirtualDtorChecker
ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
}
- static std::optional<bool> isClassWithSpecializedDelete(
- const CXXRecordDecl *C, const CXXRecordDecl *DerivedClass) {
+ static std::optional<bool>
+ isClassWithSpecializedDelete(const CXXRecordDecl *C,
+ const CXXRecordDecl *DerivedClass) {
if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
for (auto *MethodDecl : C->methods()) {
if (safeGetName(MethodDecl) == "deref") {
>From ac43a0cc15f27150e2fa5ba7ff4a17e1002d0a13 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Mon, 20 May 2024 11:22:32 -0700
Subject: [PATCH 08/10] Add more test cases.
---
.../ref-cntbl-base-virtual-dtor-templates.cpp | 164 +++++++++++++-----
1 file changed, 122 insertions(+), 42 deletions(-)
diff --git
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index f04c68cb641f1..d4f751bc1a8a8 100644
---
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -10,8 +10,7 @@ struct DerivedClassTmpl1 : T { };
// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct
'DerivedClassTmpl1<RefCntblBase>' but doesn't have virtual destructor}}
DerivedClassTmpl1<RefCntblBase> a;
-
-
+void foo(DerivedClassTmpl1<RefCntblBase>& obj) { obj.deref(); }
template<class T>
struct DerivedClassTmpl2 : T { };
@@ -21,7 +20,6 @@ template<class T> int foo(T) { DerivedClassTmpl2<T> f; return
42; }
int b = foo(RefCntblBase{});
-
template<class T>
struct DerivedClassTmpl3 : T { };
// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct
'DerivedClassTmpl3<RefCntblBase>' but doesn't have virtual destructor}}
@@ -29,7 +27,6 @@ struct DerivedClassTmpl3 : T { };
typedef DerivedClassTmpl3<RefCntblBase> Foo;
Foo c;
-
namespace WTF {
class RefCountedBase {
@@ -98,6 +95,93 @@ class FancyDeref {
mutable unsigned refCount { 0 };
};
+namespace Detail {
+
+ template<typename Out, typename... In>
+ class CallableWrapperBase {
+ public:
+ virtual ~CallableWrapperBase() { }
+ virtual Out call(In...) = 0;
+ };
+
+ template<typename, typename, typename...> class CallableWrapper;
+
+ template<typename CallableType, typename Out, typename... In>
+ class CallableWrapper : public CallableWrapperBase<Out, In...> {
+ public:
+ explicit CallableWrapper(CallableType&& callable)
+ : m_callable(WTFMove(callable)) { }
+ CallableWrapper(const CallableWrapper&) = delete;
+ CallableWrapper& operator=(const CallableWrapper&) = delete;
+ Out call(In... in) final { return m_callable(in...); }
+ private:
+ CallableType m_callable;
+ };
+
+} // namespace Detail
+
+template<typename> class Function;
+
+template <typename Out, typename... In>
+class Function<Out(In...)> {
+public:
+ using Impl = Detail::CallableWrapperBase<Out, In...>;
+
+ Function() = default;
+
+ template<typename CallableType>
+ Function(CallableType&& callable)
+ : m_callableWrapper(new Detail::CallableWrapper<CallableType, Out,
In...>>(callable)) { }
+
+ template<typename FunctionType>
+ Function(FunctionType f)
+ : m_callableWrapper(new Detail::CallableWrapper<FunctionType, Out,
In...>>(f)) { }
+
+ ~Function() {
+ }
+
+ Out operator()(In... in) const {
+ ASSERT(m_callableWrapper);
+ return m_callableWrapper->call(in...);
+ }
+
+ explicit operator bool() const { return !!m_callableWrapper; }
+
+private:
+ Impl* m_callableWrapper;
+};
+
+void ensureOnMainThread(const Function<void()>&& function);
+
+enum class DestructionThread { Any, MainThread };
+
+template <typename T, DestructionThread destructionThread =
DestructionThread::Any>
+class FancyDeref2 {
+public:
+ void ref() const
+ {
+ ++refCount;
+ }
+
+ void deref() const
+ {
+ --refCount;
+ if (refCount)
+ return;
+ const_cast<FancyDeref2<T, destructionThread>*>(this)->destroy();
+ }
+
+private:
+ void destroy() {
+ delete static_cast<T*>(this);
+ }
+ mutable unsigned refCount { 0 };
+};
+
+template <typename S>
+class DerivedFancyDeref2 : public FancyDeref2<S> {
+};
+
template <typename T>
class BadFancyDeref {
public:
@@ -132,36 +216,16 @@ class ThreadSafeRefCounted {
mutable unsigned refCount { 0 };
};
-class ThreadSafeRefCountedControlBlock {
-public:
- ThreadSafeRefCountedControlBlock(void* object) : object(object) { }
- void strongRef() const { ++refCount; }
- template <typename T>
- void strongDeref() const {
- --refCount;
- if (refCount)
- return;
-
- auto deleteObject = [&] {
- delete static_cast<const T*>(object);
- delete this;
- };
-
- deleteObject();
- }
-private:
- mutable unsigned refCount { 0 };
- mutable void* object { nullptr };
-};
-
template <typename T>
class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
public:
- ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr() { }
- void ref() const { controlBlock.strongRef(); }
- void deref() const { controlBlock.template strongDeref<T>(); }
+ void ref() const { ++refCount; }
+ void deref() const {
+ if (!--refCount)
+ delete const_cast<T*>(static_cast<const T*>(this));
+ }
private:
- ThreadSafeRefCountedControlBlock& controlBlock { *new
ThreadSafeRefCountedControlBlock(static_cast<T*>(this)) };
+ mutable unsigned refCount { 0 };
};
} // namespace WTF
@@ -170,34 +234,39 @@ class DerivedClass4 : public
WTF::RefCounted<DerivedClass4> { };
class DerivedClass4b : public WTF::ExoticRefCounted<int, DerivedClass4b> { };
-class OtherType;
-class DerivedClass4c : public WTF::BadBase<OtherType, DerivedClass4c> { };
-// expected-warning@-1{{Class 'WTF::BadBase<OtherType, DerivedClass4c>' is
used as a base of class 'DerivedClass4c' but doesn't have virtual destructor}}
-class OtherType : public DerivedClass4c { };
-// expected-warning@-1{{Class 'DerivedClass4c' is used as a base of class
'OtherType' but doesn't have virtual destructor}}
-void UseDerived4c(DerivedClass4c &obj) { obj.deref(); }
+class DerivedClass4cSub;
+class DerivedClass4c : public WTF::BadBase<DerivedClass4cSub, DerivedClass4c>
{ };
+// expected-warning@-1{{Class 'WTF::BadBase<DerivedClass4cSub,
DerivedClass4c>' is used as a base of class 'DerivedClass4c' but doesn't have
virtual destructor}}
+class DerivedClass4cSub : public DerivedClass4c { };
+void UseDerivedClass4c(DerivedClass4c &obj) { obj.deref(); }
class DerivedClass5 : public DerivedClass4 { };
// expected-warning@-1{{Class 'DerivedClass4' is used as a base of class
'DerivedClass5' but doesn't have virtual destructor}}
+void UseDerivedClass5(DerivedClass5 &obj) { obj.deref(); }
class DerivedClass6 : public WTF::ThreadSafeRefCounted<DerivedClass6> { };
+void UseDerivedClass6(DerivedClass6 &obj) { obj.deref(); }
class DerivedClass7 : public DerivedClass6 { };
// expected-warning@-1{{Class 'DerivedClass6' is used as a base of class
'DerivedClass7' but doesn't have virtual destructor}}
+void UseDerivedClass7(DerivedClass7 &obj) { obj.deref(); }
class DerivedClass8 : public
WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<DerivedClass8> { };
-
-class DerivedClass8b : public
WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<DerivedClass8b> { };
-void UseDerived8b(DerivedClass8b &obj) { obj.deref(); }
+void UseDerivedClass8(DerivedClass8 &obj) { obj.deref(); }
class DerivedClass9 : public DerivedClass8 { };
// expected-warning@-1{{Class 'DerivedClass8' is used as a base of class
'DerivedClass9' but doesn't have virtual destructor}}
+void UseDerivedClass9(DerivedClass9 &obj) { obj.deref(); }
class DerivedClass10 : public WTF::FancyDeref<DerivedClass10> { };
+void UseDerivedClass10(DerivedClass10 &obj) { obj.deref(); }
+
+class DerivedClass10b : public WTF::DerivedFancyDeref2<DerivedClass10b> { };
+void UseDerivedClass10b(DerivedClass10b &obj) { obj.deref(); }
-class DerivedClass10b : public WTF::BadFancyDeref<DerivedClass10b> { };
-// expected-warning@-1{{Class 'WTF::BadFancyDeref<DerivedClass10b>' is used as
a base of class 'DerivedClass10b' but doesn't have virtual destructor}}
-void UseDerived10b(DerivedClass10b &obj) { obj.deref(); }
+class DerivedClass10c : public WTF::BadFancyDeref<DerivedClass10c> { };
+// expected-warning@-1{{Class 'WTF::BadFancyDeref<DerivedClass10c>' is used as
a base of class 'DerivedClass10c' but doesn't have virtual destructor}}
+void UseDerivedClass10c(DerivedClass10c &obj) { obj.deref(); }
class BaseClass1 {
public:
@@ -225,6 +294,8 @@ void BaseClass1::deref() const
}
}
+void UseDerivedClass11(DerivedClass11& obj) { obj.deref(); }
+
class BaseClass2;
static void deleteBase2(BaseClass2*);
@@ -245,6 +316,8 @@ class DerivedClass12 : public BaseClass2 {
bool isDerived() final { return true; }
};
+void UseDerivedClass11(DerivedClass12& obj) { obj.deref(); }
+
void deleteBase2(BaseClass2* obj) {
if (obj->isDerived())
delete static_cast<DerivedClass12*>(obj);
@@ -272,6 +345,8 @@ class DerivedClass13 : public BaseClass3 {
bool isDerived() final { return true; }
};
+void UseDerivedClass11(DerivedClass13& obj) { obj.deref(); }
+
void BaseClass3::destory() {
if (isDerived())
delete static_cast<DerivedClass13*>(this);
@@ -309,4 +384,9 @@ class DerivedClass14 : public
WTF::RefCounted<DerivedClass14> {
public:
virtual ~DerivedClass14() { }
};
+
+void UseDerivedClass14(DerivedClass14& obj) { obj.deref(); }
+
class DerivedClass15 : public DerivedClass14 { };
+
+void UseDerivedClass15(DerivedClass15& obj) { obj.deref(); }
>From 7a2509f380dc5143a5a0bdf65224b6198aa4a122 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Mon, 20 May 2024 15:56:26 -0700
Subject: [PATCH 09/10] [webkit.RefCntblBaseVirtualDtor] Exclude CRTP
(curiously recurring template pattern) classes from analysis.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 57 ++++++++++++++++++-
.../ref-cntbl-base-virtual-dtor-templates.cpp | 8 ++-
2 files changed, 63 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index e565b2323063c..a145d6e3117db 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SetVector.h"
#include <optional>
using namespace clang;
@@ -168,13 +169,56 @@ class RefCntblBaseVirtualDtorChecker
bool shouldVisitImplicitCode() const { return false; }
bool VisitCXXRecordDecl(const CXXRecordDecl *RD) {
- Checker->visitCXXRecordDecl(RD);
+ if (!RD->hasDefinition())
+ return true;
+
+ Decls.insert(RD);
+
+ for (auto &Base : RD->bases()) {
+ const auto AccSpec = Base.getAccessSpecifier();
+ if (AccSpec == AS_protected || AccSpec == AS_private ||
+ (AccSpec == AS_none && RD->isClass()))
+ continue;
+
+ QualType T = Base.getType();
+ if (T.isNull())
+ continue;
+
+ const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+ if (!C)
+ continue;
+
+ if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+ auto &Args = CTSD->getTemplateArgs();
+ for (unsigned i = 0; i < Args.size(); ++i) {
+ if (Args[i].getKind() != TemplateArgument::Type)
+ continue;
+ auto TemplT = Args[i].getAsType();
+ if (TemplT.isNull())
+ continue;
+
+ bool IsCRTP = TemplT->getAsCXXRecordDecl() == RD;
+ if (!IsCRTP)
+ continue;
+ CRTPs.insert(C);
+ }
+ }
+ }
+
return true;
}
+
+ llvm::SetVector<const CXXRecordDecl*> Decls;
+ llvm::DenseSet<const CXXRecordDecl*> CRTPs;
};
LocalVisitor visitor(this);
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+ for (auto *RD : visitor.Decls) {
+ if (visitor.CRTPs.contains(RD))
+ continue;
+ visitCXXRecordDecl(RD);
+ }
}
void visitCXXRecordDecl(const CXXRecordDecl *RD) const {
@@ -232,6 +276,17 @@ class RefCntblBaseVirtualDtorChecker
auto HasSpecializedDelete = isClassWithSpecializedDelete(C, RD);
if (!HasSpecializedDelete || *HasSpecializedDelete)
continue;
+ if (C->lookupInBases([&](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto *T = Base->getType().getTypePtrOrNull();
+ if (!T)
+ return false;
+ auto *R = T->getAsCXXRecordDecl();
+ if (!R)
+ return false;
+ auto Result = isClassWithSpecializedDelete(R, RD);
+ return Result && *Result;
+ }, Paths, /*LookupInDependent =*/true))
+ continue;
const auto *Dtor = C->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
diff --git
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index d4f751bc1a8a8..4fc1624d7a154 100644
---
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -56,7 +56,7 @@ class RefCounted : public RefCountedBase {
};
template <typename X, typename T>
-class ExoticRefCounted : RefCountedBase {
+class ExoticRefCounted : public RefCountedBase {
public:
void deref() const {
if (derefBase())
@@ -240,6 +240,12 @@ class DerivedClass4c : public
WTF::BadBase<DerivedClass4cSub, DerivedClass4c> {
class DerivedClass4cSub : public DerivedClass4c { };
void UseDerivedClass4c(DerivedClass4c &obj) { obj.deref(); }
+class DerivedClass4d : public WTF::RefCounted<DerivedClass4d> {
+public:
+ virtual ~DerivedClass4d() { }
+};
+class DerivedClass4dSub : public DerivedClass4d { };
+
class DerivedClass5 : public DerivedClass4 { };
// expected-warning@-1{{Class 'DerivedClass4' is used as a base of class
'DerivedClass5' but doesn't have virtual destructor}}
void UseDerivedClass5(DerivedClass5 &obj) { obj.deref(); }
>From ac804b300867c908c57afc9f1ca49c484bbac4da Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Mon, 20 May 2024 16:02:53 -0700
Subject: [PATCH 10/10] Fix formatting.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 26 ++++++++++---------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index a145d6e3117db..cd339911fbf38 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -208,8 +208,8 @@ class RefCntblBaseVirtualDtorChecker
return true;
}
- llvm::SetVector<const CXXRecordDecl*> Decls;
- llvm::DenseSet<const CXXRecordDecl*> CRTPs;
+ llvm::SetVector<const CXXRecordDecl *> Decls;
+ llvm::DenseSet<const CXXRecordDecl *> CRTPs;
};
LocalVisitor visitor(this);
@@ -276,16 +276,18 @@ class RefCntblBaseVirtualDtorChecker
auto HasSpecializedDelete = isClassWithSpecializedDelete(C, RD);
if (!HasSpecializedDelete || *HasSpecializedDelete)
continue;
- if (C->lookupInBases([&](const CXXBaseSpecifier *Base, CXXBasePath &) {
- auto *T = Base->getType().getTypePtrOrNull();
- if (!T)
- return false;
- auto *R = T->getAsCXXRecordDecl();
- if (!R)
- return false;
- auto Result = isClassWithSpecializedDelete(R, RD);
- return Result && *Result;
- }, Paths, /*LookupInDependent =*/true))
+ if (C->lookupInBases(
+ [&](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto *T = Base->getType().getTypePtrOrNull();
+ if (!T)
+ return false;
+ auto *R = T->getAsCXXRecordDecl();
+ if (!R)
+ return false;
+ auto Result = isClassWithSpecializedDelete(R, RD);
+ return Result && *Result;
+ },
+ Paths, /*LookupInDependent =*/true))
continue;
const auto *Dtor = C->getDestructor();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
