https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/181576
>From 87c1e4b3f76d940610b984e7ee8bf649127e516c Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Sun, 15 Feb 2026 13:57:17 -0800 Subject: [PATCH] [WebKit Checkers] Trivial analysis should check destructors of function parameters and local variables This PR fixes the bug in TrivialFunctionAnalysisVisitor that it wasn't checking the triviality of destructors of function parameters and local variables. This meant that code calls a non-trivial desturctors such as RefPtr<T>::~RefPtr<T> which calls T::~T to be incorrectly treated as trivial, resulting in false negatives. To do this, we manually visit every function parameter and local variable declaration and check the triviality of its destructor recursively. --- .../Checkers/WebKit/NoDeleteChecker.cpp | 13 +++- .../Checkers/WebKit/PtrTypesSemantics.cpp | 65 +++++++++++++++++- .../Checkers/WebKit/PtrTypesSemantics.h | 5 ++ .../Analysis/Checkers/WebKit/mock-types.h | 18 +++-- .../Checkers/WebKit/nodelete-annotation.cpp | 67 +++++++++++++++++++ .../Checkers/WebKit/uncounted-local-vars.cpp | 5 ++ 6 files changed, 166 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp index 2740890704767..ee163bdd516ef 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp @@ -92,7 +92,18 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { return; auto Body = FD->getBody(); - if (!Body || TFA.isTrivial(Body)) + if (!Body) + return; + + bool ParamHaveTrivialDtors = true; + for (auto* Param : FD->parameters()) { + if (!TFA.hasTrivialDtor(Param)) { + ParamHaveTrivialDtors = false; + break; + } + } + + if (ParamHaveTrivialDtors && TFA.isTrivial(Body)) return; SmallString<100> Buf; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index c47dabf2ec5b0..0d52dd474d47d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -527,6 +527,10 @@ class TrivialFunctionAnalysisVisitor return true; if (FnDecl->isVirtualAsWritten()) return false; + for (auto* Param : FnDecl->parameters()) { + if (!HasTrivialDestructor(Param)) + return false; + } } return WithCachedResult(D, [&]() { if (auto *CtorDecl = dyn_cast<CXXConstructorDecl>(D)) { @@ -541,6 +545,45 @@ class TrivialFunctionAnalysisVisitor return Visit(Body); }); } + + bool HasTrivialDestructor(const VarDecl *VD) { + auto QT = VD->getType(); + if (QT.isPODType(VD->getASTContext())) + return true; + auto *Type = QT.getTypePtrOrNull(); + if (!Type) + return false; + const CXXRecordDecl *R = Type->getAsCXXRecordDecl(); + if (!R) { + if (isa<LValueReferenceType>(Type)) + return true; // T& does not run its destructor. + if (auto *RT = dyn_cast<RValueReferenceType>(Type)) { + // For T&&, we evaluate the destructor of T. + QT = RT->getPointeeType(); + Type = QT.getTypePtrOrNull(); + if (!Type) + return false; + R = Type->getAsCXXRecordDecl(); + } + } + if (!R) { + if (auto *AT = dyn_cast<ConstantArrayType>(Type)) { + QT = AT->getElementType(); + Type = QT.getTypePtrOrNull(); + if (!Type) + return false; + if (isa<PointerType>(Type)) // An array of pointer does not have a destructor. + return true; + R = Type->getAsCXXRecordDecl(); + } + } + if (!R) + return false; + auto *Dtor = R->getDestructor(); + if (!Dtor || Dtor->isTrivial()) + return true; + return IsFunctionTrivial(Dtor); + } bool IsStatementTrivial(const Stmt *S) { auto CacheIt = Cache.find(S); @@ -579,7 +622,15 @@ class TrivialFunctionAnalysisVisitor return true; } - bool VisitDeclStmt(const DeclStmt *DS) { return VisitChildren(DS); } + bool VisitDeclStmt(const DeclStmt *DS) { + for (auto& Decl : DS->decls()) { + if (auto *VD = dyn_cast<VarDecl>(Decl)) { + if (!HasTrivialDestructor(VD)) + return false; + } + } + return VisitChildren(DS); + } bool VisitDoStmt(const DoStmt *DS) { return VisitChildren(DS); } bool VisitIfStmt(const IfStmt *IS) { return WithCachedResult(IS, [&]() { return VisitChildren(IS); }); @@ -731,6 +782,10 @@ class TrivialFunctionAnalysisVisitor return true; } + bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr* E) { + return Visit(E->getExpr()); + } + bool checkArguments(const CallExpr *CE) { for (const Expr *Arg : CE->arguments()) { if (Arg && !Visit(Arg)) @@ -769,7 +824,7 @@ class TrivialFunctionAnalysisVisitor bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE) { if (auto *Temp = BTE->getTemporary()) { - if (!TrivialFunctionAnalysis::isTrivialImpl(Temp->getDestructor(), Cache)) + if (!IsFunctionTrivial(Temp->getDestructor())) return false; } return Visit(BTE->getSubExpr()); @@ -857,4 +912,10 @@ bool TrivialFunctionAnalysis::isTrivialImpl( return V.IsStatementTrivial(S); } +bool TrivialFunctionAnalysis::hasTrivialDtorImpl( + const VarDecl *VD, CacheTy &Cache) { + TrivialFunctionAnalysisVisitor V(Cache); + return V.HasTrivialDestructor(VD); +} + } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 431357a2150be..c76f3d47bc08e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -28,6 +28,7 @@ class Stmt; class TranslationUnitDecl; class Type; class TypedefDecl; +class VarDecl; // Ref-countability of a type is implicitly defined by Ref<T> and RefPtr<T> // implementation. It can be modeled as: type T having public methods ref() and @@ -169,6 +170,9 @@ class TrivialFunctionAnalysis { /// \returns true if \p D is a "trivial" function. bool isTrivial(const Decl *D) const { return isTrivialImpl(D, TheCache); } bool isTrivial(const Stmt *S) const { return isTrivialImpl(S, TheCache); } + bool hasTrivialDtor(const VarDecl* VD) const { + return hasTrivialDtorImpl(VD, TheCache); + } private: friend class TrivialFunctionAnalysisVisitor; @@ -179,6 +183,7 @@ class TrivialFunctionAnalysis { static bool isTrivialImpl(const Decl *D, CacheTy &Cache); static bool isTrivialImpl(const Stmt *S, CacheTy &Cache); + static bool hasTrivialDtorImpl(const VarDecl *VD, CacheTy &Cache); }; } // namespace clang diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index 8a24a3c64e0e4..3ed32d8b48caf 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -227,12 +227,19 @@ template <typename T> bool operator!=(const RefPtr<T> &, T &) { return false; } struct RefCountable { static Ref<RefCountable> create(); static std::unique_ptr<RefCountable> makeUnique(); - void ref() {} - void deref() {} + void ref() { ++m_refCount; } + void deref() { + --m_refCount; + if (!--m_refCount) + delete this; + } void method(); void constMethod() const; int trivial() { return 123; } RefCountable* next(); + +private: + unsigned m_refCount { 0 }; }; template <typename T> T *downcast(T *t) { return t; } @@ -280,11 +287,14 @@ template <typename T> struct CheckedPtr { class CheckedObj { public: - void incrementCheckedPtrCount(); - void decrementCheckedPtrCount(); + void incrementCheckedPtrCount() { ++m_ptrCount; } + void decrementCheckedPtrCount() { --m_ptrCount; } void method(); int trivial() { return 123; } CheckedObj* next(); + +private: + unsigned m_ptrCount { 0 }; }; class RefCountableAndCheckable { diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp index 82667a7916f42..213414505929f 100644 --- a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp @@ -1,5 +1,7 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoDeleteChecker -verify %s +#include "mock-types.h" + void someFunction(); void [[clang::annotate_type("webkit.nodelete")]] safeFunction(); @@ -28,7 +30,27 @@ void [[clang::annotate_type("webkit.nodelete")]] defWithNoDelete() { someFunction(); } +class WeakRefCountable : public CanMakeWeakPtr<WeakRefCountable> { +public: + static Ref<WeakRefCountable> create(); + + ~WeakRefCountable(); + + void ref() { m_refCount++; } + void deref() { + m_refCount--; + if (!m_refCount) + delete this; + } + +private: + WeakRefCountable(); + + unsigned m_refCount { 0 }; +}; + class SomeClass { +public: void [[clang::annotate_type("webkit.nodelete")]] someMethod(); void [[clang::annotate_type("webkit.nodelete")]] unsafeMethod() { // expected-warning@-1{{A function 'unsafeMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} @@ -57,6 +79,51 @@ class SomeClass { } virtual void [[clang::annotate_type("webkit.nodelete")]] anotherVirtualMethod(); + + void [[clang::annotate_type("webkit.nodelete")]] setObj(RefCountable* obj) { + // expected-warning@-1{{A function 'setObj' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + m_obj = obj; + } + + void [[clang::annotate_type("webkit.nodelete")]] swapObj(RefPtr<RefCountable>&& obj) { + // expected-warning@-1{{A function 'swapObj' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + m_obj.swap(obj); + } + + void [[clang::annotate_type("webkit.nodelete")]] clearObj(RefCountable* obj) { + // expected-warning@-1{{A function 'clearObj' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + m_obj = nullptr; + } + + void [[clang::annotate_type("webkit.nodelete")]] deposeArg(WeakRefCountable&& unused) { + // expected-warning@-1{{A function 'deposeArg' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + void [[clang::annotate_type("webkit.nodelete")]] deposeArgPtr(RefPtr<RefCountable>&& unused) { + // expected-warning@-1{{A function 'deposeArgPtr' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + void [[clang::annotate_type("webkit.nodelete")]] deposeLocal() { + // expected-warning@-1{{A function 'deposeLocal' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + RefPtr<RefCountable> obj = std::move(m_obj); + } + + RefPtr<RefCountable> [[clang::annotate_type("webkit.nodelete")]] copyRefPtr() { + return m_obj; + } + + Ref<WeakRefCountable> [[clang::annotate_type("webkit.nodelete")]] copyRef() { + return *m_weakObj.get(); + } + + RefPtr<WeakRefCountable> [[clang::annotate_type("webkit.nodelete")]] getWeakPtr() { + return m_weakObj.get(); + } + +private: + RefPtr<RefCountable> m_obj; + Ref<RefCountable> m_ref; + WeakPtr<WeakRefCountable> m_weakObj; }; class IntermediateClass : public SomeClass { diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index e8022b7fe8ba0..10aaf041e19dd 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -63,7 +63,12 @@ void foo4() { void foo5() { RefPtr<RefCountable> foo; auto* bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} bar->trivial(); + { + auto* baz = foo.get(); + baz->trivial(); + } } void foo6() { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
