[clang] [alpha.webkit.NoDeleteChecker] Blame the code that is actually unsafe (PR #224728)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Sat Oct 3 23:51:43 PDT 2026
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/224728
>From 4a60d626ed3b2c393a32f4ea86bd8d9bcb53d3b4 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Fri, 18 Sep 2026 13:05:53 -0700
Subject: [PATCH] [alpha.webkit.NoDeleteChecker] Blame the code that is
actually unsafe
The checker reported the whole top-level statement containing the problem,
because VisitChildren was the only place that recorded an offending statement
and IsFunctionTrivial clears the recorder before descending into a callee. For
offset = std::min<unsigned>(9, offset + localHeadingOffset(*htmlAncestor));
that put the caret on the entire assignment, which reads as an accusation
against std::min even though std::min is trivial and the real problem is an
opaque function three levels down inside the second argument.
Record the offending statement in a Visit() wrapper instead. Recursion unwinds
innermost-first, so the deepest failing node wins and the caret lands on
'localHeadingOffset(*htmlAncestor)'. Nodes without a source location are
skipped in favour of the nearest enclosing node that was actually written.
Track the deepest callee that could not be proven trivial alongside it and
emit it as a note, so the diagnostic names the function the user has to fix.
The note is suppressed when the offending statement is already the call to
that function. Computing this requires descending into callees that the shared
memoization cache would short-circuit, so it runs on a private cache via
explainNonTriviality(), only on the path that is about to warn.
Also guard the cache lookup in IsStatementTrivial with the same condition
WithCachedResult uses. A cache hit there would report failure without
recording an offending statement, leaving the checker to dereference null.
---
.../Checkers/WebKit/NoDeleteChecker.cpp | 61 ++++++++++++++++---
.../Checkers/WebKit/PtrTypesSemantics.cpp | 58 ++++++++++++++----
.../Checkers/WebKit/PtrTypesSemantics.h | 33 ++++++++--
.../Checkers/WebKit/nodelete-annotation.cpp | 51 +++++++++++++---
.../WebKit/nodelete-lazy-initialize.cpp | 2 +-
5 files changed, 169 insertions(+), 36 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp
index cf4518bb962d7..fb53534ad1715 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp
@@ -86,7 +86,7 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> {
}
const FieldDecl *Field = nullptr;
- const Stmt *OffendingStmt = nullptr;
+ const Stmt *OffendingInit = nullptr;
bool IsCtor = false;
bool IsDtor = false;
if (auto *Ctor = dyn_cast<CXXConstructorDecl>(FD)) {
@@ -94,9 +94,9 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> {
Field = TFA.fieldWithNonTrivialCtor(Ctor->getParent());
if (!Field) {
for (auto *CtorInit : Ctor->inits()) {
- if (!TFA.isTrivial(CtorInit->getInit(), &OffendingStmt)) {
- if (!OffendingStmt)
- OffendingStmt = CtorInit->getInit();
+ auto* Init = CtorInit->getInit();
+ if (!TFA.isTrivial(Init)) {
+ OffendingInit = Init;
break;
}
}
@@ -106,8 +106,7 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> {
Field = TFA.fieldWithNonTrivialDtor(Dtor->getParent());
}
- if (!ParamDecl && !Field && !OffendingStmt &&
- TFA.isTrivial(Body, &OffendingStmt))
+ if (!ParamDecl && !Field && !OffendingInit && TFA.isTrivial(Body))
return;
SmallString<100> Buf;
@@ -130,30 +129,74 @@ class NoDeleteChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> {
Os << "contains ";
SourceLocation SrcLocToReport;
SourceRange Range;
+ NonTrivialityReason Reason;
if (ParamDecl) {
Os << "a parameter ";
printQuotedName(Os, ParamDecl);
Os << " which could destruct an object.";
SrcLocToReport = FD->getBeginLoc();
Range = ParamDecl->getSourceRange();
- } else if (Field) {
+ } else if (Field && !OffendingInit) {
Os << "a member variable ";
printQuotedName(Os, Field);
Os << " that could destruct an object.";
SrcLocToReport = FD->getBeginLoc();
Range = Field->getSourceRange();
} else {
+ Reason = TrivialFunctionAnalysis::computeReason(OffendingInit ? OffendingInit : Body);
Os << "code that could destruct an object.";
- SrcLocToReport = OffendingStmt->getBeginLoc();
- Range = OffendingStmt->getSourceRange();
+ const Stmt *Offender = Reason.OffendingStmt;
+ SrcLocToReport = Offender ? Offender->getBeginLoc() : FD->getBeginLoc();
+ Range = Offender ? Offender->getSourceRange() : FD->getSourceRange();
}
PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
Report->addRange(Range);
Report->setDeclWithIssue(FD);
+ addRootCauseNote(*Report, Reason);
BR->emitReport(std::move(Report));
}
+
+ static const FunctionDecl *getDirectCallee(const Stmt *S) {
+ if (const auto *CE = dyn_cast_or_null<CallExpr>(S))
+ return CE->getDirectCallee();
+ if (const auto *CE = dyn_cast_or_null<CXXConstructExpr>(S))
+ return CE->getConstructor();
+ return nullptr;
+ }
+
+ // The offending statement is often just the nearest call to a function that
+ // is itself unsafe several levels down. Point at the function at the bottom
+ // of that chain, since that is where the fix belongs.
+ void addRootCauseNote(BasicBugReport &Report,
+ const NonTrivialityReason &Reason) const {
+ const FunctionDecl *RootCause = Reason.RootCause;
+ // Implicit special members have nothing worth pointing at.
+ if (!RootCause || !RootCause->getLocation().isValid())
+ return;
+
+ // Nothing to add when the offending statement is the call to the root
+ // cause; the primary diagnostic already points right at it.
+ const FunctionDecl *Callee = getDirectCallee(Reason.OffendingStmt);
+ if (Callee && Callee->getCanonicalDecl() == RootCause->getCanonicalDecl())
+ return;
+
+ SmallString<100> Buf;
+ llvm::raw_svector_ostream Os(Buf);
+ printQuotedName(Os, RootCause);
+ if (RootCause->doesThisDeclarationHaveABody()) {
+ Os << " could destruct an object.";
+ } else {
+ Os << " has no visible definition here, so it is assumed to destruct an "
+ "object. Annotate it with "
+ "[[clang::annotate_type(\"webkit.nodelete\")]] if it does not.";
+ }
+
+ PathDiagnosticLocation Loc(RootCause->getLocation(),
+ BR->getSourceManager());
+ Report.addNote(Os.str(), Loc, RootCause->getSourceRange());
+ }
};
} // namespace
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 1cd2822575845..11cc173e9c5a6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -680,15 +680,13 @@ bool isSingleton(const NamedDecl *F) {
// (non-recursive) visitor.
class TrivialFunctionAnalysisVisitor
: public ConstStmtVisitor<TrivialFunctionAnalysisVisitor, bool> {
+ using Base = ConstStmtVisitor<TrivialFunctionAnalysisVisitor, bool>;
// 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 (OffendingStmt && !*OffendingStmt)
- *OffendingStmt = Child;
+ if (Child && !Visit(Child))
return false;
- }
}
return true;
@@ -846,10 +844,28 @@ class TrivialFunctionAnalysisVisitor
using CacheTy = TrivialFunctionAnalysis::CacheTy;
TrivialFunctionAnalysisVisitor(CacheTy &Cache,
- const Stmt **OffendingStmt = nullptr)
- : Cache(Cache), OffendingStmt(OffendingStmt) {}
+ NonTrivialityReason *Reason = nullptr)
+ : Cache(Cache), OffendingStmt(Reason ? &Reason->OffendingStmt : nullptr),
+ RootCause(Reason ? &Reason->RootCause : nullptr) {}
+
+ // Hides ConstStmtVisitor::Visit so that every recursive step in this class
+ // funnels through here. Recursion unwinds innermost-first, so the first
+ // statement recorded is the deepest one that failed -- the code actually
+ // responsible, rather than the enclosing statement that contains it.
+ // Implicit nodes have no location to point at, so they are passed over in
+ // favour of the nearest enclosing node that was actually written.
+ bool Visit(const Stmt *S) {
+ bool Result = Base::Visit(S);
+ if (!Result && OffendingStmt && !*OffendingStmt &&
+ S->getBeginLoc().isValid())
+ *OffendingStmt = S;
+ return Result;
+ }
bool IsFunctionTrivial(const Decl *D) {
+ // Blame stays within the function being analyzed: a statement in a callee
+ // is not a useful location for the primary diagnostic. The root cause is
+ // tracked separately and does cross function boundaries.
const Stmt **SavedOffendingStmt = std::exchange(OffendingStmt, nullptr);
auto Result = WithCachedResult(D, [&]() {
auto *FnDecl = dyn_cast<FunctionDecl>(D);
@@ -898,6 +914,12 @@ class TrivialFunctionAnalysisVisitor
return Visit(Body);
});
OffendingStmt = SavedOffendingStmt;
+ // Unwinding innermost-first means the deepest callee in the chain claims
+ // the root cause, which is the one the user has to do something about.
+ if (!Result && RootCause && !*RootCause) {
+ if (const auto *FnDecl = dyn_cast<FunctionDecl>(D))
+ *RootCause = FnDecl;
+ }
return Result;
}
@@ -923,8 +945,10 @@ class TrivialFunctionAnalysisVisitor
}
bool IsStatementTrivial(const Stmt *S) {
+ // Skip the cache while diagnosing: a cache hit would report failure without
+ // recording which statement is to blame.
auto CacheIt = Cache.find(S);
- if (CacheIt != Cache.end())
+ if (CacheIt != Cache.end() && !OffendingStmt)
return CacheIt->second;
bool Result = Visit(S);
Cache[S] = Result;
@@ -1306,22 +1330,30 @@ class TrivialFunctionAnalysisVisitor
CacheTy FieldDtorCache;
CacheTy RecursiveFn;
const Stmt **OffendingStmt;
+ const FunctionDecl **RootCause;
};
bool TrivialFunctionAnalysis::isTrivialImpl(
- const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache,
- const Stmt **OffendingStmt) {
- TrivialFunctionAnalysisVisitor V(Cache, OffendingStmt);
+ const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) {
+ TrivialFunctionAnalysisVisitor V(Cache);
return V.IsFunctionTrivial(D);
}
bool TrivialFunctionAnalysis::isTrivialImpl(
- const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache,
- const Stmt **OffendingStmt) {
- TrivialFunctionAnalysisVisitor V(Cache, OffendingStmt);
+ const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache) {
+ TrivialFunctionAnalysisVisitor V(Cache);
return V.IsStatementTrivial(S);
}
+NonTrivialityReason TrivialFunctionAnalysis::computeReason(const Stmt *S) {
+ NonTrivialityReason Reason;
+ CacheTy Cache;
+ TrivialFunctionAnalysisVisitor V(Cache, &Reason);
+ [[maybe_unused]] bool Trivial = V.IsStatementTrivial(S);
+ assert(!Trivial && "explainNonTriviality called on a trivial statement");
+ return Reason;
+}
+
bool TrivialFunctionAnalysis::hasTrivialDtorImpl(const VarDecl *VD,
CacheTy &Cache) {
TrivialFunctionAnalysisVisitor V(Cache);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index d174fb6c37175..343ebf1e43e99 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -208,16 +208,32 @@ bool isTrivialBuiltinFunction(const FunctionDecl *F);
/// \returns true if \p F is a static singleton function.
bool isSingleton(const NamedDecl *F);
+/// Explains why TrivialFunctionAnalysis rejected a statement, so that a
+/// diagnostic can blame the code that is actually responsible.
+struct NonTrivialityReason {
+ /// The innermost non-trivial statement inside the analyzed function's own
+ /// body. Without this, a diagnostic would have to blame the whole enclosing
+ /// statement, which often reads as an accusation against an innocent callee
+ /// that merely happens to appear first, e.g. the std::min in
+ /// `x = std::min(a, unsafe())`.
+ const Stmt *OffendingStmt = nullptr;
+
+ /// The deepest callee that could not be proven free of destruction. Null when
+ /// the offending statement destructs an object by itself, e.g. a delete
+ /// expression or a local variable with a non-trivial destructor.
+ const FunctionDecl *RootCause = nullptr;
+};
+
/// An inter-procedural analysis facility that detects functions with "trivial"
/// behavior with respect to reference counting, such as simple field getters.
class TrivialFunctionAnalysis {
public:
/// \returns true if \p D is a "trivial" function.
- bool isTrivial(const Decl *D, const Stmt **OffendingStmt = nullptr) const {
- return isTrivialImpl(D, TheCache, OffendingStmt);
+ bool isTrivial(const Decl *D) const {
+ return isTrivialImpl(D, TheCache);
}
- bool isTrivial(const Stmt *S, const Stmt **OffendingStmt = nullptr) const {
- return isTrivialImpl(S, TheCache, OffendingStmt);
+ bool isTrivial(const Stmt *S) const {
+ return isTrivialImpl(S, TheCache);
}
bool hasTrivialDtor(const VarDecl *VD) const {
return hasTrivialDtorImpl(VD, TheCache);
@@ -229,6 +245,11 @@ class TrivialFunctionAnalysis {
return fieldWithNonTrivialDtorImpl(RD, TheCache);
}
+ /// \returns why \p S is not trivial. Runs on a private, empty cache because
+ /// pinpointing the root cause requires descending into callees that a shared
+ /// cache would short-circuit. Only call this when about to emit a diagnostic.
+ static NonTrivialityReason computeReason(const Stmt *S);
+
private:
friend class TrivialFunctionAnalysisVisitor;
@@ -236,8 +257,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);
+ static bool isTrivialImpl(const Stmt *S, CacheTy &Cache);
static bool hasTrivialDtorImpl(const VarDecl *VD, CacheTy &Cache);
static const FieldDecl *fieldWithNonTrivialCtorImpl(const CXXRecordDecl *RD,
CacheTy &Cache);
diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp
index 11cf81c6bf0bc..7be33c76c3219 100644
--- a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp
@@ -2,6 +2,11 @@
#include "mock-types.h"
+// Each warning also points at the root cause of the destruction. Anything held
+// in a Ref/RefPtr bottoms out in RefCountable::deref, which lives in the shared
+// header, so that note has to be expected by file and line.
+// expected-note at mock-types.h:437 11 {{'deref' could destruct an object}}
+
void *memcpy(void *dst, const void *src, unsigned int size);
void *malloc(unsigned int size);
void free(void *);
@@ -337,7 +342,7 @@ struct Data {
++refCount;
}
- void deref() {
+ void deref() { // expected-note 3 {{'deref' could destruct an object}}
--refCount;
if (!refCount)
delete this;
@@ -408,13 +413,13 @@ void [[clang::annotate_type("webkit.nodelete")]] makeObjectWithConstructor() {
}
struct ObjectWithNonTrivialDestructor {
- ~ObjectWithNonTrivialDestructor();
+ ~ObjectWithNonTrivialDestructor(); // expected-note 3 {{'~ObjectWithNonTrivialDestructor' has no visible definition here, so it is assumed to destruct an object}}
};
struct Container {
Ref<Container> create() { return adoptRef(*new Container); }
void ref() const { refCount++; }
- void deref() const {
+ void deref() const { // expected-note 2 {{'deref' could destruct an object}}
refCount--;
if (!refCount)
delete this;
@@ -438,7 +443,7 @@ struct OtherContainerBase {
struct OtherContainer : public OtherContainerBase {
Ref<OtherContainer> create() { return adoptRef(*new OtherContainer); }
void ref() const { refCount++; }
- void deref() const {
+ void deref() const { // expected-note {{'deref' could destruct an object}}
refCount--;
if (!refCount)
delete this;
@@ -492,7 +497,7 @@ struct ObjectWithContainers {
struct SomeObject {
void ref() const;
- void deref() const;
+ void deref() const; // expected-note 6 {{'deref' has no visible definition here, so it is assumed to destruct an object}}
void doTrivialWork() { }
@@ -577,6 +582,38 @@ struct MemberAssignment {
Vector<Ref<SomeObject>> m_objects;
};
+namespace blame_the_root_cause {
+
+// The reported statement must be the innermost expression that is actually
+// unsafe, not the whole enclosing statement. Blaming the statement makes the
+// first call in it look guilty -- for `min(9, offset + opaque())` that is
+// 'min', which is entirely innocent. The note then names the function at the
+// bottom of the chain, which is where the fix belongs.
+
+template <typename T>
+T [[clang::annotate_type("webkit.nodelete")]] min(const T& a, const T& b) {
+ return b < a ? b : a;
+}
+
+unsigned opaqueHelper(); // expected-note {{'opaqueHelper' has no visible definition here, so it is assumed to destruct an object}}
+
+unsigned safeHelper() { return 1; }
+
+unsigned wrapsOpaqueHelper() { return opaqueHelper(); }
+
+void [[clang::annotate_type("webkit.nodelete")]] callsMinWithSafeArgs(unsigned offset) {
+ offset = min<unsigned>(9, offset + safeHelper());
+ (void)offset;
+}
+
+void [[clang::annotate_type("webkit.nodelete")]] callsMinWithUnsafeArg(unsigned offset) {
+ offset = min<unsigned>(9, offset + wrapsOpaqueHelper());
+ // expected-warning at -1{{A function 'callsMinWithUnsafeArg' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}}
+ (void)offset;
+}
+
+} // namespace blame_the_root_cause
+
namespace copy_elision_edge_cases {
// These cases all inhibit NRVO/copy elision (so a real move or copy constructor runs into the return slot),
@@ -663,7 +700,7 @@ namespace temp_object_typecheck {
struct Tracked {
Tracked();
- ~Tracked();
+ ~Tracked(); // expected-note {{'~Tracked' has no visible definition here, so it is assumed to destruct an object}}
};
Tracked [[clang::annotate_type("webkit.nodelete")]] makeTracked();
@@ -762,7 +799,7 @@ namespace create_with_default_constructor {
};
struct ObjectWithOpaqueCtor {
- ObjectWithOpaqueCtor();
+ ObjectWithOpaqueCtor(); // expected-note {{'ObjectWithOpaqueCtor' has no visible definition here, so it is assumed to destruct an object}}
};
struct ObjectWithDefaultConstructorWithOpaqueCtorMemberVariables {
diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp
index 6d8ddb13e817e..b9f4df2c05065 100644
--- a/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp
@@ -15,7 +15,7 @@ template<typename T, typename U>
struct RefObj {
static Ref<RefObj> [[clang::annotate_type("webkit.nodelete")]] create(int = 0);
void ref() const;
- void deref() const;
+ void deref() const; // expected-note 2 {{'deref' has no visible definition here, so it is assumed to destruct an object}}
int value() const;
};
More information about the cfe-commits
mailing list