https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/200912
From 3ec0c08b9c3f6d4376b1e6699019e44730d52a05 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Fri, 29 May 2026 11:48:52 -0700 Subject: [PATCH 1/3] [alpha.webkit.NoDeleteChecker] Treat a r-value smart pointer argument or return value as no-delete. Skip the checkin g of the destructor of T in ExprWithCleanups / CXXBindTemporaryExpr when returning or passing in a function argument using r-value reference since such a construct never invokes delete. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 27 +++++- .../Analysis/Checkers/WebKit/call-args.cpp | 17 ++++ .../Analysis/Checkers/WebKit/mock-types.h | 1 + .../Checkers/WebKit/nodelete-annotation.cpp | 82 ++++++++++++++++++- .../WebKit/nodelete-lazy-initialize.cpp | 36 ++++++++ 5 files changed, 158 insertions(+), 5 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 2ca34ff0587e1..7568eabb7932c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -713,9 +713,8 @@ class TrivialFunctionAnalysisVisitor } bool VisitReturnStmt(const ReturnStmt *RS) { - // A return statement is allowed as long as the return value is trivial. if (auto *RV = RS->getRetValue()) - return Visit(RV); + return VisitIgnoringTempWithoutDestruction(RV); return true; } @@ -895,15 +894,35 @@ class TrivialFunctionAnalysisVisitor bool checkArguments(const CallExpr *CE) { for (const Expr *Arg : CE->arguments()) { - if (Arg && !Visit(Arg)) + if (Arg && !VisitIgnoringTempWithoutDestruction(Arg)) return false; } return true; } + bool VisitIgnoringTempWithoutDestruction(const Expr *Arg) { + QualType OriginalQT = Arg->getType(); + auto *Type = OriginalQT.getTypePtrOrNull(); + if (!Type) + return Visit(Arg); + auto *CXXRD = Type->getAsCXXRecordDecl(); + if (!CXXRD || !isSmartPtrClass(safeGetName(CXXRD))) + return Visit(Arg); + Arg = Arg->IgnoreParenCasts(); + if (!Arg->isPRValue()) + return Visit(Arg); + if (auto *ExprWithClean = dyn_cast<ExprWithCleanups>(Arg)) + Arg = ExprWithClean->getSubExpr()->IgnoreParenCasts(); + if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg)) { + if (OriginalQT == BTE->getType()) + return Visit(BTE->getSubExpr()); + } + return Visit(Arg); + } + bool VisitCXXConstructExpr(const CXXConstructExpr *CE) { for (const Expr *Arg : CE->arguments()) { - if (Arg && !Visit(Arg)) + if (Arg && !VisitIgnoringTempWithoutDestruction(Arg)) return false; } diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp index f15991134c58a..bcd3a9592d55d 100644 --- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp +++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp @@ -557,4 +557,21 @@ namespace call_with_weak_ptr { weakPtr->method(); // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} } + + struct Provider { + RefCountableWithWeakPtr* provide(); + }; + int intValue(); + + struct Container { + Container(Provider& provider) + : m_weakPtr(provider.provide()) + , m_value(intValue()) + { } + + private: + WeakPtr<RefCountableWithWeakPtr> m_weakPtr; + int m_value; + }; + } diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index af63268ac9695..de5c2d35f2408 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -202,6 +202,7 @@ template <typename T> struct RefPtr { t = o.t; o.t = tmp; } + operator T*() { return t; } T *get() const { return t; } T *operator->() const { return t; } T &operator*() const { return *t; } diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp index e0dfab150e0a4..d93aa0f78bc87 100644 --- a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp @@ -316,10 +316,23 @@ class Derived : public Base<Type> { }; struct Data { - static Ref<Data> create() { + static Ref<Data> [[clang::annotate_type("webkit.nodelete")]] create() { return adoptRef(*new Data); } + static Ref<Data> [[clang::annotate_type("webkit.nodelete")]] create(double) { + return adoptRef(*new Data(RefCountable::create()->next())); + // expected-warning@-1{{A function 'create' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + static Data* [[clang::annotate_type("webkit.nodelete")]] create(int) { + return adoptRef(new Data); // expected-warning{{A function 'create' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + static Ref<Data> create(const char*) { + return std::move(adoptRef(*new Data)); + } + void ref() { ++refCount; } @@ -338,6 +351,7 @@ struct Data { protected: Data() = default; + Data(RefCountable*) { } private: unsigned refCount { 0 }; @@ -562,3 +576,69 @@ struct MemberAssignment { RefPtr<SomeObject> m_someObject; Vector<Ref<SomeObject>> m_objects; }; + +namespace copy_elision_edge_cases { + +// These cases all inhibit NRVO/copy elision (so a real move or copy constructor runs into the return slot), +// but none of them perform any local destruction: +// - Moved-from operands are emptied; their dtors are no-ops at the caller. +// - Globals/statics are not destructed here at all. +// - The return-slot temporary is destructed by the caller, not by us. +// All the mock Ref<T> copy/move ctors only manipulate pointers and a +// refcount, so the trivial-ctor analysis correctly classifies these as +// safe. The tests below document that the checker accepts each shape. + +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnStdMoved(Ref<RefCountable>&& obj) { + return std::move(obj); +} + +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnFromBranches(bool b, Ref<RefCountable>&& a, Ref<RefCountable>&& c) { + if (b) + return std::move(a); + return std::move(c); +} + +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnDerefedParam(Ref<RefCountable>* param) { + return *param; +} + +// Returning a by-value parameter also requires a real copy/move construction. +// The function is still flagged here because of unsafe-parameter diagnostic that fires for the parameter declaration. +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnByValueParam(Ref<RefCountable> param) { + // expected-warning@-1{{A function 'returnByValueParam' has [[clang::annotate_type("webkit.nodelete")]] but it contains a parameter 'param' which could destruct an object}} + return param; +} + +extern Ref<RefCountable> g_ref; +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnGlobal() { + return g_ref; +} + +struct StaticHolder { + static Ref<RefCountable> s_ref; +}; +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnClassStatic() { + return StaticHolder::s_ref; +} + +} // namespace copy_elision_edge_cases + +namespace temp_object_typecheck { + +struct Tracked { + Tracked(); + ~Tracked(); +}; + +Tracked [[clang::annotate_type("webkit.nodelete")]] makeTracked(); + +struct Box { + Box(const Tracked&) {} +}; + +Box [[clang::annotate_type("webkit.nodelete")]] makeBox() { + return Box(makeTracked()); + // expected-warning@-1{{A function 'makeBox' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + +} // namespace temp_object_typecheck \ No newline at end of file diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp new file mode 100644 index 0000000000000..aabe949aae07d --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp @@ -0,0 +1,36 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoDeleteChecker -verify %s + +#include "mock-types.h" + +void crash(); + +template<typename T, typename U> +[[clang::suppress]] inline void [[clang::annotate_type("webkit.nodelete")]] lazyInitialize(const RefPtr<T>& ptr, Ref<U>&& obj) +{ + if (ptr) + crash(); + const_cast<RefPtr<T>&>(ptr) = WTF::move(obj); +} + +struct RefObj { + static Ref<RefObj> [[clang::annotate_type("webkit.nodelete")]] create(int = 0); + void ref() const; + void deref() const; + int value() const; +}; + +struct Container { + + void [[clang::annotate_type("webkit.nodelete")]] foo() { + if (!m_bar) + lazyInitialize(m_bar, RefObj::create()); + } + + void [[clang::annotate_type("webkit.nodelete")]] bar() { + if (!m_bar) + lazyInitialize(m_bar, RefObj::create(RefObj::create()->value())); + // expected-warning@-1{{A function 'bar' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + const RefPtr<RefObj> m_bar; +}; From 0b2bcb6d62f7d057b332d102ea8b930800fb5a08 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Tue, 2 Jun 2026 12:28:13 +0100 Subject: [PATCH 2/3] [analyzer][WebKit] NoDelete: only elide returned-prvalue temporaries The TrivialFunctionAnalysis temporary-destructor skip added for webkit.nodelete was applied to both return values and call/constructor arguments. It is only sound for return values: a returned class prvalue is constructed directly into the caller's return slot (C++17 guaranteed copy elision), so the temporary is destructed by the caller, not in the analyzed function. An argument temporary's lifetime instead ends at the full-expression in the *caller* (the caller destroys arguments, e.g. per the Itanium C++ ABI), so its destructor runs in the analyzed function and may invoke delete. Whether it actually does depends on the callee taking ownership, which the checker does not prove -- so skipping it produced false negatives (e.g. `sink(make())` was accepted while a discarded `make();` was flagged, despite both destroying the same temporary at the same point). Restrict the skip to return values: revert checkArguments() and VisitCXXConstructExpr() to the normal Visit(), and rename the helper to visitReturnValueElidingTemp() with a comment explaining the rationale. Verified against -O2 codegen (arm64-apple-darwin): a returned prvalue tail-calls the factory with the sret pointer forwarded and emits no destructor, whereas every argument form (by value, rvalue reference, const reference) and a discarded temporary emit a ~T() call in the analyzed function -- matching the Itanium rule that the caller destroys arguments. Tests: nodelete-lazy-initialize.cpp's foo() now correctly warns (its argument temporary is no longer elided); add a characterization namespace to nodelete-annotation.cpp pinning the returns-vs-arguments boundary. Assisted-By: claude --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 24 ++++++++-- .../Checkers/WebKit/nodelete-annotation.cpp | 45 ++++++++++++++++++- .../WebKit/nodelete-lazy-initialize.cpp | 5 +++ 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 7568eabb7932c..30291f5acca07 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -713,8 +713,12 @@ class TrivialFunctionAnalysisVisitor } bool VisitReturnStmt(const ReturnStmt *RS) { + // A return statement is allowed as long as the return value is trivial. A + // returned smart-pointer prvalue is special: under guaranteed copy elision + // the temporary *is* the function's return slot, so it is destructed by the + // caller, not here. Hence we may ignore that temporary's destructor. if (auto *RV = RS->getRetValue()) - return VisitIgnoringTempWithoutDestruction(RV); + return visitReturnValueElidingTemp(RV); return true; } @@ -894,13 +898,25 @@ class TrivialFunctionAnalysisVisitor bool checkArguments(const CallExpr *CE) { for (const Expr *Arg : CE->arguments()) { - if (Arg && !VisitIgnoringTempWithoutDestruction(Arg)) + if (Arg && !Visit(Arg)) return false; } return true; } - bool VisitIgnoringTempWithoutDestruction(const Expr *Arg) { + // Triviality check for a return value that may elide a smart-pointer + // temporary's destructor. + // + // This is only valid for *return values*: a returned class prvalue is + // constructed directly into the function's return slot (C++17 guaranteed copy + // elision), so the temporary is destructed by the caller rather than here. + // + // It is deliberately NOT applied to call/constructor arguments. An argument + // temporary's lifetime ends at the full-expression *in this function* (the + // caller destroys arguments, e.g. per the Itanium C++ ABI), so its destructor + // runs here and may invoke delete. Proving otherwise would require + // interprocedural ownership analysis, so arguments are checked normally. + bool visitReturnValueElidingTemp(const Expr *Arg) { QualType OriginalQT = Arg->getType(); auto *Type = OriginalQT.getTypePtrOrNull(); if (!Type) @@ -922,7 +938,7 @@ class TrivialFunctionAnalysisVisitor bool VisitCXXConstructExpr(const CXXConstructExpr *CE) { for (const Expr *Arg : CE->arguments()) { - if (Arg && !VisitIgnoringTempWithoutDestruction(Arg)) + if (Arg && !Visit(Arg)) return false; } diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp index d93aa0f78bc87..6c0c0820543ce 100644 --- a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp @@ -641,4 +641,47 @@ Box [[clang::annotate_type("webkit.nodelete")]] makeBox() { // expected-warning@-1{{A function 'makeBox' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } -} // namespace temp_object_typecheck \ No newline at end of file +} // namespace temp_object_typecheck + +namespace argument_temporaries_are_not_elided { + +// Only a *returned* prvalue is elided into the caller's return slot. A +// smart-pointer temporary passed as a call argument is destructed in this +// function at the end of the full-expression (the caller destroys arguments), +// so its destructor -- which may run delete -- is correctly flagged, no matter +// how the callee binds it (by value, by rvalue reference, or by const +// reference). The factory and sinks are annotated no-delete so the only +// possible offender is the argument temporary's destruction. + +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] makeRef(); +void [[clang::annotate_type("webkit.nodelete")]] sinkByValue(Ref<RefCountable>); +void [[clang::annotate_type("webkit.nodelete")]] sinkByRvalueRef(Ref<RefCountable>&&); +void [[clang::annotate_type("webkit.nodelete")]] observeByConstRef(const Ref<RefCountable>&); + +// Returned prvalue: constructed into the caller's return slot -> no local +// destruction here. +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnedPrvalueIsElided() { + return makeRef(); +} + +void [[clang::annotate_type("webkit.nodelete")]] passedByValueIsFlagged() { + sinkByValue(makeRef()); + // expected-warning@-1{{A function 'passedByValueIsFlagged' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] passedByRvalueRefIsFlagged() { + sinkByRvalueRef(makeRef()); + // expected-warning@-1{{A function 'passedByRvalueRefIsFlagged' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] passedByConstRefIsFlagged() { + observeByConstRef(makeRef()); + // expected-warning@-1{{A function 'passedByConstRefIsFlagged' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + +void [[clang::annotate_type("webkit.nodelete")]] discardedTemporaryIsFlagged() { + makeRef(); + // expected-warning@-1{{A function 'discardedTemporaryIsFlagged' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + +} // namespace argument_temporaries_are_not_elided diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp index aabe949aae07d..6d8ddb13e817e 100644 --- a/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-lazy-initialize.cpp @@ -24,6 +24,11 @@ struct Container { void [[clang::annotate_type("webkit.nodelete")]] foo() { if (!m_bar) lazyInitialize(m_bar, RefObj::create()); + // expected-warning@-1{{A function 'foo' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + // The 'RefObj::create()' temporary is passed as an argument, so its + // lifetime ends at this full-expression in 'foo' (the caller destroys + // arguments) and its destructor may run delete. Only returned prvalues + // are elided, so this is correctly flagged. } void [[clang::annotate_type("webkit.nodelete")]] bar() { From d81f8bb03cd56120e1108f4b7b392905413fc9a8 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Tue, 2 Jun 2026 12:30:55 +0100 Subject: [PATCH 3/3] [analyzer][WebKit] NoDelete: compare canonical types when eliding a returned temporary visitReturnValueElidingTemp() decided whether the bound temporary is the returned object using exact QualType equality (OriginalQT == BTE->getType()). Exact equality is sensitive to sugar (typedefs/aliases) and cv-qualifiers, so it can silently fail to recognise the temporary even when it is the same smart-pointer type. Compare canonical, unqualified types instead, which states the intent directly. This is defensive hardening: on the return path the return-value expression and its bound temporary share the same type spelling, so no existing test distinguishes the two comparisons; the canonical form guards against future sugar/qualifier drift. A characterization test (returned_prvalue_typedef) documents that a typedef'd returned prvalue is elided. Assisted-By: claude --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 7 ++++++- .../Checkers/WebKit/nodelete-annotation.cpp | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 30291f5acca07..d5ed7fc78148a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -930,7 +930,12 @@ class TrivialFunctionAnalysisVisitor if (auto *ExprWithClean = dyn_cast<ExprWithCleanups>(Arg)) Arg = ExprWithClean->getSubExpr()->IgnoreParenCasts(); if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg)) { - if (OriginalQT == BTE->getType()) + // Only elide when the temporary *is* the returned object, i.e. it has the + // same smart-pointer type as the return value. Compare canonical, + // unqualified types rather than relying on exact QualType identity, which + // is sensitive to sugar (typedefs/aliases) and cv-qualifiers. + if (OriginalQT.getCanonicalType().getUnqualifiedType() == + BTE->getType().getCanonicalType().getUnqualifiedType()) return Visit(BTE->getSubExpr()); } return Visit(Arg); diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp index 6c0c0820543ce..a9c50cfb1f45f 100644 --- a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp @@ -685,3 +685,19 @@ void [[clang::annotate_type("webkit.nodelete")]] discardedTemporaryIsFlagged() { } } // namespace argument_temporaries_are_not_elided + +namespace returned_prvalue_typedef { + +// A returned prvalue spelled through a typedef/alias is still the return-slot +// object and must be elided. The elision relies on a canonical, unqualified +// type comparison rather than exact QualType identity. + +using RefRC = Ref<RefCountable>; +RefRC [[clang::annotate_type("webkit.nodelete")]] makeAlias(); + +Ref<RefCountable> [[clang::annotate_type("webkit.nodelete")]] returnTypedefPrvalue() { + return makeAlias(); // no warning: the elided temporary is the return slot. +} + +} // namespace returned_prvalue_typedef + _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
