[webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor. #92837
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesExempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy 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. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. Patch is 23.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92837.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 7f4c3a7b787e8..cd339911fbf38 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -11,16 +11,134 @@
#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 "llvm/ADT/SetVector.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) {}
+
+ std::optional<bool> HasSpecializedDelete(CXXMethodDecl *Decl) {
+ if (auto *Body = Decl->getBody())
+ return VisitBody(Body);
+ if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
+ return std::nullopt; // Indeterminate. There was no concrete instance.
+ return false;
+ }
+
+ 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<CastExpr>(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 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
+ 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:
@@ -51,63 +169,94 @@ 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 {
if (shouldSkipDecl(RD))
return;
- CXXBasePaths Paths;
- Paths.setOrigin(RD);
+ for (auto &Base : RD->bases()) {
+ const auto AccSpec = Base.getAccessSpecifier();
+ if (AccSpec == AS_protected || AccSpec == AS_private ||
+ (AccSpec == AS_none && RD->isClass()))
+ continue;
- const CXXBaseSpecifier *ProblematicBaseSpecifier = nullptr;
- const CXXRecordDecl *ProblematicBaseClass = nullptr;
+ auto hasRefInBase = clang::hasPublicMethodInBase(&Base, "ref");
+ auto hasDerefInBase = clang::hasPublicMethodInBase(&Base, "deref");
- 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;
+ bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
+ bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
- auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
- auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+ QualType T = Base.getType();
+ if (T.isNull())
+ continue;
- bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
- bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+ const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+ if (!C)
+ continue;
- QualType T = Base->getType();
- if (T.isNull())
- return false;
-
- const CXXRecordDecl *C = T->getAsCXXRecordDecl();
- if (!C)
- return false;
- if (isRefCountedClass(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 &) {
+ 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) {
AnyInconclusiveBase = true;
@@ -115,28 +264,38 @@ class RefCntblBaseVirtualDtorChecker
}
return (*hasDerefInBase) != nullptr;
};
- CXXBasePaths Paths;
- Paths.setOrigin(C);
- hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+ 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;
-
- const auto *Dtor = C->getDestructor();
- if (!Dtor || !Dtor->isVirtual()) {
- ProblematicBaseSpecifier = Base;
- ProblematicBaseClass = C;
- return true;
- }
-
- return false;
- };
-
- if (RD->lookupInBases(IsPublicBaseRefCntblWOVirtualDtor, Paths,
- /*LookupInDependent =*/true)) {
- reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+ if (AnyInconclusiveBase || !hasRef || !hasDeref)
+ continue;
+
+ 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()) {
+ auto *ProblematicBaseSpecifier = &Base;
+ auto *ProblematicBaseClass = C;
+ reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+ }
}
}
@@ -182,6 +341,32 @@ class RefCntblBaseVirtualDtorChecker
ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
}
+ 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);
+ auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+ if (!Result || *Result)
+ return Result;
+ }
+ }
+ return false;
+ }
+ for (auto *MethodDecl : C->methods()) {
+ if (safeGetName(MethodDecl) == "deref") {
+ DerefAnalysisVisitor DerefAnalysis(DerivedClass);
+ auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+ if (!Result || *Result)
+ return Result;
+ }
+ }
+ 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..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
@@ -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 {
@@ -58,33 +55,344 @@ class RefCounted : public RefCountedBase {
RefCounted() { }
};
+template <typename X, typename T>
+class ExoticRefCounted : public 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 };
+};
+
+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:
+ 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 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 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(); }
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> { };
+void UseDerivedClass8(DerivedClass8 &obj) { obj.deref(); }
class DerivedClass9 : public DerivedClass8 { };
// expected-warning@-1{{Class 'DerivedClass8' is used as a base of class 'D...
[truncated]
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesExempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy 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. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated. Patch is 23.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92837.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 7f4c3a7b787e8..cd339911fbf38 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -11,16 +11,134 @@
#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 "llvm/ADT/SetVector.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) {}
+
+ std::optional<bool> HasSpecializedDelete(CXXMethodDecl *Decl) {
+ if (auto *Body = Decl->getBody())
+ return VisitBody(Body);
+ if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
+ return std::nullopt; // Indeterminate. There was no concrete instance.
+ return false;
+ }
+
+ 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<CastExpr>(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 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
+ 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:
@@ -51,63 +169,94 @@ 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 {
if (shouldSkipDecl(RD))
return;
- CXXBasePaths Paths;
- Paths.setOrigin(RD);
+ for (auto &Base : RD->bases()) {
+ const auto AccSpec = Base.getAccessSpecifier();
+ if (AccSpec == AS_protected || AccSpec == AS_private ||
+ (AccSpec == AS_none && RD->isClass()))
+ continue;
- const CXXBaseSpecifier *ProblematicBaseSpecifier = nullptr;
- const CXXRecordDecl *ProblematicBaseClass = nullptr;
+ auto hasRefInBase = clang::hasPublicMethodInBase(&Base, "ref");
+ auto hasDerefInBase = clang::hasPublicMethodInBase(&Base, "deref");
- 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;
+ bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
+ bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
- auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
- auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+ QualType T = Base.getType();
+ if (T.isNull())
+ continue;
- bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
- bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+ const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+ if (!C)
+ continue;
- QualType T = Base->getType();
- if (T.isNull())
- return false;
-
- const CXXRecordDecl *C = T->getAsCXXRecordDecl();
- if (!C)
- return false;
- if (isRefCountedClass(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 &) {
+ 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) {
AnyInconclusiveBase = true;
@@ -115,28 +264,38 @@ class RefCntblBaseVirtualDtorChecker
}
return (*hasDerefInBase) != nullptr;
};
- CXXBasePaths Paths;
- Paths.setOrigin(C);
- hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+ 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;
-
- const auto *Dtor = C->getDestructor();
- if (!Dtor || !Dtor->isVirtual()) {
- ProblematicBaseSpecifier = Base;
- ProblematicBaseClass = C;
- return true;
- }
-
- return false;
- };
-
- if (RD->lookupInBases(IsPublicBaseRefCntblWOVirtualDtor, Paths,
- /*LookupInDependent =*/true)) {
- reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+ if (AnyInconclusiveBase || !hasRef || !hasDeref)
+ continue;
+
+ 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()) {
+ auto *ProblematicBaseSpecifier = &Base;
+ auto *ProblematicBaseClass = C;
+ reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+ }
}
}
@@ -182,6 +341,32 @@ class RefCntblBaseVirtualDtorChecker
ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
}
+ 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);
+ auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+ if (!Result || *Result)
+ return Result;
+ }
+ }
+ return false;
+ }
+ for (auto *MethodDecl : C->methods()) {
+ if (safeGetName(MethodDecl) == "deref") {
+ DerefAnalysisVisitor DerefAnalysis(DerivedClass);
+ auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+ if (!Result || *Result)
+ return Result;
+ }
+ }
+ 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..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
@@ -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 {
@@ -58,33 +55,344 @@ class RefCounted : public RefCountedBase {
RefCounted() { }
};
+template <typename X, typename T>
+class ExoticRefCounted : public 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 };
+};
+
+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:
+ 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 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 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(); }
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> { };
+void UseDerivedClass8(DerivedClass8 &obj) { obj.deref(); }
class DerivedClass9 : public DerivedClass8 { };
// expected-warning@-1{{Class 'DerivedClass8' is used as a base of class 'D...
[truncated]
|
rniwa
force-pushed
the
allow-crtp-without-virtual-dtor
branch
from
6061b5e to
9cbdf82
Compare
… destructor. Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy 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. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated.
rniwa
force-pushed
the
allow-crtp-without-virtual-dtor
branch
from
9cbdf82 to
9c2ae2b
Compare
haoNoQ
reviewed
May 24, 2024
| return false; | ||
|
|
||
| auto [It, IsNew] = VisitedBody.insert(Body); | ||
| if (!IsNew) // This body is recursive |
There was a problem hiding this comment.
Shouldn't it return whatever it returned previously for that body?
There was a problem hiding this comment.
Actually, the only way we'd hit this code path is if the function is recursive and we're still in the middle of recursively visiting the function definition so the best we can do here is to return false (since we haven't finished traversing the function body yet).
There was a problem hiding this comment.
Oh ok it's a one-shot visitor that scans one function and dies. But if it is instantiated again to scan another function that calls a previously visited function, it'll re-scan it from scratch.
So like, this ensures termination but there's also probably room for major optimizations there. We don't really have strict performance requirements, esp. for project-specific checks, but at a glance this could matter on some TUs, so it may be a good idea to keep an eye on performance.
There was a problem hiding this comment.
Yeah. We could keep the visitor around for the duration of the checker & cache the results. Will address that in a follow up.
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
Outdated
Show resolved
Hide resolved
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
Outdated
Show resolved
Hide resolved
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
haoNoQ
approved these changes
May 25, 2024
| return false; | ||
|
|
||
| auto [It, IsNew] = VisitedBody.insert(Body); | ||
| if (!IsNew) // This body is recursive |
There was a problem hiding this comment.
Oh ok it's a one-shot visitor that scans one function and dies. But if it is instantiated again to scan another function that calls a previously visited function, it'll re-scan it from scratch.
So like, this ensures termination but there's also probably room for major optimizations there. We don't really have strict performance requirements, esp. for project-specific checks, but at a glance this could matter on some TUs, so it may be a good idea to keep an eye on performance.
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
Outdated
Show resolved
Hide resolved
chapuni
reviewed
May 26, 2024
| std::optional<bool> HasSpecializedDelete(CXXMethodDecl *Decl) { | ||
| if (auto *Body = Decl->getBody()) | ||
| return VisitBody(Body); | ||
| if (auto *Tmpl = Decl->getTemplateInstantiationPattern()) |
There was a problem hiding this comment.
This causes a warning.
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp:61:15: warning: variable 'Tmpl' set but not used [-Wunused-but-set-variable]
There was a problem hiding this comment.
Oh oops, sorry about that.
There was a problem hiding this comment.
Here's a PR to fix it: #93403
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Feb 3, 2025
… destructor. (llvm#92837) Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor. To do this, this PR introduces new DerefFuncDeleteExprVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class. This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy 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. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary. It's possible for DerefFuncDeleteExprVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete operation acting on "this" pointer with an appropriate cast from the requirement that a ref-countable superclass must have a virtual destructor.
To do this, this PR introduces new DerefAnalysisVisitor, which looks for a delete operation with an explicit cast to the derived class in a base class.
This PR also changes the checker so that we only check a given class's immediate base class instead of all ancestor base classes in the class hierarchy. This is sufficient because the checker will eventually see the definition for every class in the class hierarchy 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. Without this change, we would keep traversing the same list of base classes whenever we encounter a new subclass, which is wholly unnecessary.
It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't enough information to determine whether a given templated superclass invokes delete operation on a subclass when the template isn't fully specialized for the subclass. In this case, we return std::nullopt in HasSpecializedDelete, and visitCXXRecordDecl will skip this declaration. This is okay because the checker will eventually see a concreate fully specialized class definition if it ever gets instantiated.