https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/182614
>From b2dbab6965a213ad3ca81faff94d98ac04f358b7 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Fri, 20 Feb 2026 14:59:15 -0800 Subject: [PATCH 1/2] [alpha.webkit.NoDeleteChecker] Better diagnostics This PR improves NoDeleteChecker's bug report so that it's more actionable. Namely, when a function fails "triviali function analysis", we report the first statement or parameter which failed the triviality check. --- .../Checkers/WebKit/NoDeleteChecker.cpp | 33 +++++-- .../Checkers/WebKit/PtrTypesSemantics.cpp | 27 ++++-- .../Checkers/WebKit/PtrTypesSemantics.h | 12 ++- .../Checkers/WebKit/nodelete-annotation.cpp | 96 ++++++++++++++----- 4 files changed, 126 insertions(+), 42 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp index abbdc2967e859..2a9d216823392 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp @@ -12,6 +12,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DynamicRecursiveASTVisitor.h" +#include "clang/AST/QualTypeNames.h" #include "clang/Analysis/DomainSpecific/CocoaConventions.h" #include "clang/Basic/SourceLocation.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" @@ -85,7 +86,7 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { } void visitFunctionDecl(const FunctionDecl *FD) const { - if (!FD->doesThisDeclarationHaveABody()) + if (!FD->doesThisDeclarationHaveABody() || FD->isDependentContext()) return; if (!hasNoDeleteAnnotation(FD)) @@ -95,8 +96,15 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { if (!Body) return; - auto hasTrivialDtor = [&](VarDecl *D) { return TFA.hasTrivialDtor(D); }; - if (llvm::all_of(FD->parameters(), hasTrivialDtor) && TFA.isTrivial(Body)) + NamedDecl* ParamDecl = nullptr; + for (auto* D : FD->parameters()) { + if (!TFA.hasTrivialDtor(D)) { + ParamDecl = D; + break; + } + } + const Stmt *OffendingStmt = nullptr; + if (!ParamDecl && TFA.isTrivial(Body, &OffendingStmt)) return; SmallString<100> Buf; @@ -104,13 +112,24 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { Os << "A function "; printQuotedName(Os, FD); - Os << " has [[clang::annotate_type(\"webkit.nodelete\")]] but it contains " - "code that could destruct an object"; + Os << " has [[clang::annotate_type(\"webkit.nodelete\")]] but it contains "; + SourceLocation SrcLocToReport; + SourceRange Range; + if (ParamDecl) { + Os << "a parameter "; + printQuotedName(Os, ParamDecl); + Os << " which could destruct an object."; + SrcLocToReport = FD->getBeginLoc(); + Range = ParamDecl->getSourceRange(); + } else { + Os << "code that could destruct an object."; + SrcLocToReport = OffendingStmt->getBeginLoc(); + Range = OffendingStmt->getSourceRange(); + } - const SourceLocation SrcLocToReport = FD->getBeginLoc(); PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager()); auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc); - Report->addRange(FD->getSourceRange()); + Report->addRange(Range); Report->setDeclWithIssue(FD); BR->emitReport(std::move(Report)); } diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 8cd64c12b7a73..463050d4d3b36 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -483,8 +483,11 @@ class TrivialFunctionAnalysisVisitor // Returns false if at least one child is non-trivial. bool VisitChildren(const Stmt *S) { for (const Stmt *Child : S->children()) { - if (Child && !Visit(Child)) + if (Child && !Visit(Child)) { + if (OffendingStmt && !*OffendingStmt) + *OffendingStmt = Child; return false; + } } return true; @@ -493,7 +496,7 @@ class TrivialFunctionAnalysisVisitor template <typename StmtOrDecl, typename CheckFunction> bool WithCachedResult(const StmtOrDecl *S, CheckFunction Function) { auto CacheIt = Cache.find(S); - if (CacheIt != Cache.end()) + if (CacheIt != Cache.end() && !OffendingStmt) return CacheIt->second; // Treat a recursive statement to be trivial until proven otherwise. @@ -553,10 +556,13 @@ class TrivialFunctionAnalysisVisitor public: using CacheTy = TrivialFunctionAnalysis::CacheTy; - TrivialFunctionAnalysisVisitor(CacheTy &Cache) : Cache(Cache) {} + TrivialFunctionAnalysisVisitor(CacheTy &Cache, + const Stmt **OffendingStmt = nullptr) + : Cache(Cache), OffendingStmt(OffendingStmt) {} bool IsFunctionTrivial(const Decl *D) { - return WithCachedResult(D, [&]() { + const Stmt** SavedOffendingStmt = std::exchange(OffendingStmt, nullptr); + auto Result = WithCachedResult(D, [&]() { if (auto *FnDecl = dyn_cast<FunctionDecl>(D)) { if (isNoDeleteFunction(FnDecl)) return true; @@ -578,6 +584,8 @@ class TrivialFunctionAnalysisVisitor return false; return Visit(Body); }); + OffendingStmt = SavedOffendingStmt; + return Result; } bool HasTrivialDestructor(const VarDecl *VD) { @@ -903,17 +911,20 @@ class TrivialFunctionAnalysisVisitor private: CacheTy &Cache; CacheTy RecursiveFn; + const Stmt** OffendingStmt; }; bool TrivialFunctionAnalysis::isTrivialImpl( - const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) { - TrivialFunctionAnalysisVisitor V(Cache); + const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache, + const Stmt** OffendingStmt) { + TrivialFunctionAnalysisVisitor V(Cache, OffendingStmt); return V.IsFunctionTrivial(D); } bool TrivialFunctionAnalysis::isTrivialImpl( - const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache) { - TrivialFunctionAnalysisVisitor V(Cache); + const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache, + const Stmt** OffendingStmt) { + TrivialFunctionAnalysisVisitor V(Cache, OffendingStmt); return V.IsStatementTrivial(S); } diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 8a696a789c65b..844dd787e1123 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -168,8 +168,12 @@ bool isSingleton(const NamedDecl *F); class TrivialFunctionAnalysis { public: /// \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 isTrivial(const Decl *D, const Stmt **OffendingStmt = nullptr) const { + return isTrivialImpl(D, TheCache, OffendingStmt); + } + bool isTrivial(const Stmt *S, const Stmt **OffendingStmt = nullptr) const { + return isTrivialImpl(S, TheCache, OffendingStmt); + } bool hasTrivialDtor(const VarDecl *VD) const { return hasTrivialDtorImpl(VD, TheCache); } @@ -181,8 +185,8 @@ class TrivialFunctionAnalysis { llvm::DenseMap<llvm::PointerUnion<const Decl *, const Stmt *>, bool>; mutable CacheTy TheCache{}; - static bool isTrivialImpl(const Decl *D, CacheTy &Cache); - static bool isTrivialImpl(const Stmt *S, CacheTy &Cache); + static bool isTrivialImpl(const Decl *D, CacheTy &Cache, const Stmt**); + static bool isTrivialImpl(const Stmt *S, CacheTy &Cache, const Stmt**); static bool hasTrivialDtorImpl(const VarDecl *VD, CacheTy &Cache); }; diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp index 98f4017e5e3fd..16597ff011e55 100644 --- a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp @@ -3,15 +3,14 @@ #include "mock-types.h" void someFunction(); -void [[clang::annotate_type("webkit.nodelete")]] safeFunction(); +RefCountable* [[clang::annotate_type("webkit.nodelete")]] safeFunction(); void functionWithoutNoDeleteAnnotation() { someFunction(); } void [[clang::annotate_type("webkit.nodelete")]] callsUnsafe() { - // expected-warning@-1{{A function 'callsUnsafe' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} - someFunction(); + someFunction(); // expected-warning{{A function 'callsUnsafe' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } void [[clang::annotate_type("webkit.nodelete")]] callsSafe() { @@ -20,14 +19,72 @@ void [[clang::annotate_type("webkit.nodelete")]] callsSafe() { void [[clang::annotate_type("webkit.nodelete")]] declWithNoDelete(); void declWithNoDelete() { - // expected-warning@-1{{A function 'declWithNoDelete' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} - someFunction(); + someFunction(); // expected-warning{{A function 'declWithNoDelete' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } - void defWithNoDelete(); void [[clang::annotate_type("webkit.nodelete")]] defWithNoDelete() { -// expected-warning@-1{{A function 'defWithNoDelete' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} - someFunction(); + someFunction(); // expected-warning{{A function 'defWithNoDelete' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] funncWithUnsafeParam(Ref<RefCountable> t) { + // expected-warning@-1{{A function 'funncWithUnsafeParam' has [[clang::annotate_type("webkit.nodelete")]] but it contains a parameter 't' which could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] funncWithUnsafeParam(unsigned safe, Ref<RefCountable> unsafe) { + // expected-warning@-1{{A function 'funncWithUnsafeParam' has [[clang::annotate_type("webkit.nodelete")]] but it contains a parameter 'unsafe' which could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] funncWithUnsafeParam(Ref<RefCountable> unsafe, unsigned safe) { + // expected-warning@-1{{A function 'funncWithUnsafeParam' has [[clang::annotate_type("webkit.nodelete")]] but it contains a parameter 'unsafe' which could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] funncWithSafeParam(Ref<RefCountable>& safe1, Ref<RefCountable>* safe2) { +} + +void [[clang::annotate_type("webkit.nodelete")]] callsUnsafeInDoWhile() { + do { + someFunction(); // expected-warning{{A function 'callsUnsafeInDoWhile' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } while(0); +} + +void [[clang::annotate_type("webkit.nodelete")]] callsUnsafeInIf(bool safe) { + if (safe) + safeFunction(); + else + someFunction(); // expected-warning{{A function 'callsUnsafeInIf' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] declaresUnsafeVar(bool safe) { + if (safe) { + auto* t = safeFunction(); + } else { + RefPtr<RefCountable> t = safeFunction(); + // expected-warning@-1{{A function 'declaresUnsafeVar' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } +} + +void [[clang::annotate_type("webkit.nodelete")]] declaresVarInIf(bool safe) { + if (RefPtr<RefCountable> t = safeFunction()) { + // expected-warning@-1{{A function 'declaresVarInIf' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + t->method(); + } +} + +template <typename T> +struct TemplatedClass { + void [[clang::annotate_type("webkit.nodelete")]] methodCallsUnsafe(T* t) { + t->method(); // expected-warning{{A function 'methodCallsUnsafe' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + void [[clang::annotate_type("webkit.nodelete")]] methodCallsSafe(T* t) { + t->trivial(); + } +}; + +using TemplatedToRefCountable = TemplatedClass<RefCountable>; +void useTemplatedToRefCountable() { + TemplatedToRefCountable c; + c.methodCallsUnsafe(nullptr); + c.methodCallsSafe(nullptr); } class WeakRefCountable : public CanMakeWeakPtr<WeakRefCountable> { @@ -54,8 +111,7 @@ class SomeClass { 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}} - someFunction(); + someFunction(); // expected-warning{{A function 'unsafeMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } void [[clang::annotate_type("webkit.nodelete")]] safeMethod() { safeFunction(); @@ -63,8 +119,7 @@ class SomeClass { virtual void [[clang::annotate_type("webkit.nodelete")]] someVirtualMethod(); virtual void [[clang::annotate_type("webkit.nodelete")]] unsafeVirtualMethod() { - // expected-warning@-1{{A function 'unsafeVirtualMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} - someFunction(); + someFunction(); // expected-warning{{A function 'unsafeVirtualMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } virtual void [[clang::annotate_type("webkit.nodelete")]] safeVirtualMethod() { safeFunction(); @@ -72,8 +127,7 @@ class SomeClass { static void [[clang::annotate_type("webkit.nodelete")]] someStaticMethod(); static void [[clang::annotate_type("webkit.nodelete")]] unsafeStaticMethod() { - // expected-warning@-1{{A function 'unsafeStaticMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} - someFunction(); + someFunction(); // expected-warning{{A function 'unsafeStaticMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } static void [[clang::annotate_type("webkit.nodelete")]] safeStaticMethod() { safeFunction(); @@ -82,8 +136,7 @@ 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; + m_obj = obj; // expected-warning{{A function 'setObj' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } void [[clang::annotate_type("webkit.nodelete")]] swapObj(RefPtr<RefCountable>&& obj) { @@ -91,8 +144,7 @@ class SomeClass { } 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; + m_obj = nullptr; // expected-warning{{A function 'clearObj' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } void [[clang::annotate_type("webkit.nodelete")]] deposeArg(WeakRefCountable&& unused) { @@ -108,8 +160,7 @@ class SomeClass { } 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> obj = std::move(m_obj); // expected-warning{{A function 'deposeLocal' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } RefPtr<RefCountable> [[clang::annotate_type("webkit.nodelete")]] copyRefPtr() { @@ -141,8 +192,7 @@ class IntermediateClass : public SomeClass { class DerivedClass : public IntermediateClass { void anotherVirtualMethod() override { - // expected-warning@-1{{A function 'anotherVirtualMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} - someFunction(); + someFunction(); // expected-warning{{A function 'anotherVirtualMethod' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } }; @@ -201,6 +251,6 @@ void [[clang::annotate_type("webkit.nodelete")]] makeData() { } void [[clang::annotate_type("webkit.nodelete")]] makeSubData() { - // expected-warning@-1{{A function 'makeSubData' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} SubData::create()->doSomething(); + // expected-warning@-1{{A function 'makeSubData' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } >From da43c2723a20581fdaa2062707bc6fe9943c6746 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Fri, 20 Feb 2026 15:06:33 -0800 Subject: [PATCH 2/2] Fix formatting. --- .../StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp | 4 ++-- .../Checkers/WebKit/PtrTypesSemantics.cpp | 10 +++++----- .../StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp index 2a9d216823392..075c46c79f139 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp @@ -96,8 +96,8 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { if (!Body) return; - NamedDecl* ParamDecl = nullptr; - for (auto* D : FD->parameters()) { + NamedDecl *ParamDecl = nullptr; + for (auto *D : FD->parameters()) { if (!TFA.hasTrivialDtor(D)) { ParamDecl = D; break; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 463050d4d3b36..dffd056b04bbe 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -558,10 +558,10 @@ class TrivialFunctionAnalysisVisitor TrivialFunctionAnalysisVisitor(CacheTy &Cache, const Stmt **OffendingStmt = nullptr) - : Cache(Cache), OffendingStmt(OffendingStmt) {} + : Cache(Cache), OffendingStmt(OffendingStmt) {} bool IsFunctionTrivial(const Decl *D) { - const Stmt** SavedOffendingStmt = std::exchange(OffendingStmt, nullptr); + const Stmt **SavedOffendingStmt = std::exchange(OffendingStmt, nullptr); auto Result = WithCachedResult(D, [&]() { if (auto *FnDecl = dyn_cast<FunctionDecl>(D)) { if (isNoDeleteFunction(FnDecl)) @@ -911,19 +911,19 @@ class TrivialFunctionAnalysisVisitor private: CacheTy &Cache; CacheTy RecursiveFn; - const Stmt** OffendingStmt; + const Stmt **OffendingStmt; }; bool TrivialFunctionAnalysis::isTrivialImpl( const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache, - const Stmt** OffendingStmt) { + const Stmt **OffendingStmt) { TrivialFunctionAnalysisVisitor V(Cache, OffendingStmt); return V.IsFunctionTrivial(D); } bool TrivialFunctionAnalysis::isTrivialImpl( const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache, - const Stmt** OffendingStmt) { + const Stmt **OffendingStmt) { TrivialFunctionAnalysisVisitor V(Cache, OffendingStmt); return V.IsStatementTrivial(S); } diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 844dd787e1123..bcb4eee16bd2c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -185,8 +185,8 @@ class TrivialFunctionAnalysis { llvm::DenseMap<llvm::PointerUnion<const Decl *, const Stmt *>, bool>; mutable CacheTy TheCache{}; - static bool isTrivialImpl(const Decl *D, CacheTy &Cache, const Stmt**); - static bool isTrivialImpl(const Stmt *S, CacheTy &Cache, const Stmt**); + static bool isTrivialImpl(const Decl *D, CacheTy &Cache, const Stmt **); + static bool isTrivialImpl(const Stmt *S, CacheTy &Cache, const Stmt **); static bool hasTrivialDtorImpl(const VarDecl *VD, CacheTy &Cache); }; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
