https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/183711
>From 07477ee120576299a891efe6b7f147752f13e114 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Fri, 27 Feb 2026 01:10:19 -0800 Subject: [PATCH 1/3] [alpha.webkit.NoDeleteChecker] Check if each field is trivially destructive This PR fixes the bug that NoDeleteChecker and trivial function analysis were not detecting any non-trivial destruction of class member variables. When evaluating a delete expression or calling a destructor directly for triviality, check if each field in the class and its base classes is trivially destructive. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 30 ++++ .../Checkers/WebKit/nodelete-annotation.cpp | 152 +++++++++++++++++- 2 files changed, 180 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index c7ecdd9b6a402..15cd7bd8aa2b6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -539,6 +539,9 @@ class TrivialFunctionAnalysisVisitor if (R->hasDefinition() && R->hasTrivialDestructor()) return true; + if (HasFieldWithNonTrivialDtor(R)) + return false; + // For Webkit, side-effects are fine as long as we don't delete objects, // so check recursively. if (const auto *Dtor = R->getDestructor()) @@ -557,6 +560,30 @@ class TrivialFunctionAnalysisVisitor return false; // Otherwise it's likely not trivial. } + bool HasFieldWithNonTrivialDtor(const CXXRecordDecl *R) { + + auto HasNonTrivialField = [&](const CXXRecordDecl *R) { + for (const FieldDecl *F : R->fields()) { + if (!CanTriviallyDestruct(F->getType())) + return true; + } + return false; + }; + + if (HasNonTrivialField(R)) + return true; + + CXXBasePaths Paths; + Paths.setOrigin(const_cast<CXXRecordDecl *>(R)); + return R->lookupInBases([&](const CXXBaseSpecifier *B, CXXBasePath &) { + auto *T = B->getType().getTypePtrOrNull(); + if (!T) + return false; + auto *R = T->getAsCXXRecordDecl(); + return R && HasNonTrivialField(R); + }, Paths, /*LookupInDependent =*/true); + } + public: using CacheTy = TrivialFunctionAnalysis::CacheTy; @@ -765,6 +792,9 @@ class TrivialFunctionAnalysisVisitor if (!Callee) return false; + if (isa<CXXDestructorDecl>(Callee) && !CanTriviallyDestruct(MCE->getObjectType())) + return false; + auto Name = safeGetName(Callee); if (Name == "ref" || Name == "incrementCheckedPtrCount") return true; diff --git a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp index abe4aba18899e..c030402166258 100644 --- a/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp +++ b/clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp @@ -2,6 +2,63 @@ #include "mock-types.h" +void *memcpy(void *dst, const void *src, unsigned int size); +void *malloc(unsigned int size); +void free(void *); + +namespace WTF { + + template <typename T> + class Vector { + public: + ~Vector() { destory(); } + + void append(const T& v) + { + if (m_size >= m_capacity) + grow(m_capacity * 2); + new (m_buffer + m_size) T(); + m_buffer[m_size] = v; + m_size++; + } + + void shrink(unsigned newSize) + { + unsigned currentSize = m_size; + while (currentSize > newSize) { + --currentSize; + m_buffer[currentSize].~T(); + } + m_size = currentSize; + } + + private: + void grow(unsigned newCapacity) { + T* newBuffer = static_cast<T*>(malloc(sizeof(T) * newCapacity)); + memcpy(newBuffer, m_buffer, sizeof(T) * m_size); + destory(); + m_buffer = newBuffer; + m_capacity = newCapacity; + } + + void destory() { + if (!m_buffer) + return; + for (unsigned i = 0; i < m_size; ++i) + m_buffer[i]->~T(); + free(m_buffer); + m_buffer = nullptr; + } + + T* m_buffer { nullptr }; + unsigned m_size { 0 }; + unsigned m_capacity { 0 }; + }; + +} // namespace WTF + +using WTF::Vector; + void someFunction(); RefCountable* [[clang::annotate_type("webkit.nodelete")]] safeFunction(); @@ -150,6 +207,8 @@ class SomeClass { m_obj.swap(obj); } + void [[clang::annotate_type("webkit.nodelete")]] assignObj(Ref<RefCountable>&& obj); + void [[clang::annotate_type("webkit.nodelete")]] clearObj(RefCountable* obj) { m_obj = nullptr; // expected-warning{{A function 'clearObj' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} } @@ -183,8 +242,8 @@ class SomeClass { } WeakRefCountable* [[clang::annotate_type("webkit.nodelete")]] useWeakPtr() { - WeakPtr localWeak = m_weakObj.get(); - return localWeak.get(); + auto* localWeak = m_weakObj.get(); + return localWeak; } private: @@ -193,6 +252,12 @@ class SomeClass { WeakPtr<WeakRefCountable> m_weakObj; }; + +void SomeClass::assignObj(Ref<RefCountable>&& obj) { + m_obj = std::move(obj); + // expected-warning@-1{{A function 'assignObj' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} +} + class IntermediateClass : public SomeClass { void anotherVirtualMethod() override; }; @@ -284,3 +349,86 @@ void [[clang::annotate_type("webkit.nodelete")]] makeObjectWithConstructor() { ObjectWithConstructor obj5(ptrs); ObjectWithConstructor obj6(ObjectWithConstructor::E::V1); } + +struct ObjectWithNonTrivialDestructor { + ~ObjectWithNonTrivialDestructor(); +}; + +struct Container { + Ref<Container> create() { return adoptRef(*new Container); } + void ref() const { refCount++; } + void deref() const { + refCount--; + if (!refCount) + delete this; + } + + ObjectWithNonTrivialDestructor obj; + +private: + mutable unsigned refCount { 0 }; + + Container() = default; +}; + +struct SubContainer : public Container { +}; + +struct OtherContainerBase { + ObjectWithNonTrivialDestructor obj; +}; + +struct OtherContainer : public OtherContainerBase { + Ref<OtherContainer> create() { return adoptRef(*new OtherContainer); } + void ref() const { refCount++; } + void deref() const { + refCount--; + if (!refCount) + delete this; + } + +private: + mutable unsigned refCount { 0 }; + + OtherContainer() = default; +}; + +struct ObjectWithContainers { + RefPtr<Container> container; + RefPtr<SubContainer> subContainer; + RefPtr<OtherContainer> otherContainer; + + void [[clang::annotate_type("webkit.nodelete")]] setContainer(Ref<Container>&& newContainer) { + container = std::move(newContainer); + // expected-warning@-1{{A function 'setContainer' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + void [[clang::annotate_type("webkit.nodelete")]] setSubContainer(Ref<SubContainer>&& newContainer) { + subContainer = std::move(newContainer); + // expected-warning@-1{{A function 'setSubContainer' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + void [[clang::annotate_type("webkit.nodelete")]] setOtherContainer(Ref<OtherContainer>&& newContainer) { + otherContainer = std::move(newContainer); + // expected-warning@-1{{A function 'setOtherContainer' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + Vector<Container> containerList; + Vector<SubContainer> subContainerList; + Vector<OtherContainer> otherContainerList; + + void [[clang::annotate_type("webkit.nodelete")]] shrinkVector1() { + containerList.shrink(0); + // expected-warning@-1{{A function 'shrinkVector1' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + void [[clang::annotate_type("webkit.nodelete")]] shrinkVector2() { + subContainerList.shrink(0); + // expected-warning@-1{{A function 'shrinkVector2' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } + + void [[clang::annotate_type("webkit.nodelete")]] shrinkVector3() { + otherContainerList.shrink(0); + // expected-warning@-1{{A function 'shrinkVector3' has [[clang::annotate_type("webkit.nodelete")]] but it contains code that could destruct an object}} + } +}; >From 7a7d40b5e997764b4083e11c03c8f144e4781589 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Fri, 27 Feb 2026 01:44:38 -0800 Subject: [PATCH 2/3] Fix formatting and add missing hasDefinition check. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 15cd7bd8aa2b6..b6e3858f95c17 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -560,7 +560,7 @@ class TrivialFunctionAnalysisVisitor return false; // Otherwise it's likely not trivial. } - bool HasFieldWithNonTrivialDtor(const CXXRecordDecl *R) { + bool HasFieldWithNonTrivialDtor(const CXXRecordDecl *Cls) { auto HasNonTrivialField = [&](const CXXRecordDecl *R) { for (const FieldDecl *F : R->fields()) { @@ -570,18 +570,23 @@ class TrivialFunctionAnalysisVisitor return false; }; - if (HasNonTrivialField(R)) + if (HasNonTrivialField(Cls)) return true; + if (!Cls->hasDefinition()) + return false; + CXXBasePaths Paths; - Paths.setOrigin(const_cast<CXXRecordDecl *>(R)); - return R->lookupInBases([&](const CXXBaseSpecifier *B, CXXBasePath &) { - auto *T = B->getType().getTypePtrOrNull(); - if (!T) - return false; - auto *R = T->getAsCXXRecordDecl(); - return R && HasNonTrivialField(R); - }, Paths, /*LookupInDependent =*/true); + Paths.setOrigin(const_cast<CXXRecordDecl *>(Cls)); + return Cls->lookupInBases( + [&](const CXXBaseSpecifier *B, CXXBasePath &) { + auto *T = B->getType().getTypePtrOrNull(); + if (!T) + return false; + auto *R = T->getAsCXXRecordDecl(); + return R && HasNonTrivialField(R); + }, + Paths, /*LookupInDependent =*/true); } public: @@ -792,7 +797,8 @@ class TrivialFunctionAnalysisVisitor if (!Callee) return false; - if (isa<CXXDestructorDecl>(Callee) && !CanTriviallyDestruct(MCE->getObjectType())) + if (isa<CXXDestructorDecl>(Callee) && + !CanTriviallyDestruct(MCE->getObjectType())) return false; auto Name = safeGetName(Callee); >From b476e816648607988dd27610ad5ac8316344de8f Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Fri, 27 Feb 2026 02:26:48 -0800 Subject: [PATCH 3/3] Add a new cache for whether a given class' destruction invokes a non-trivial function or not. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 54 +++++++++++-------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index b6e3858f95c17..646598a3ee350 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -561,32 +561,41 @@ class TrivialFunctionAnalysisVisitor } bool HasFieldWithNonTrivialDtor(const CXXRecordDecl *Cls) { + auto CacheIt = FieldDtorCache.find(Cls); + if (CacheIt != FieldDtorCache.end()) + return CacheIt->second; - auto HasNonTrivialField = [&](const CXXRecordDecl *R) { - for (const FieldDecl *F : R->fields()) { - if (!CanTriviallyDestruct(F->getType())) - return true; - } - return false; - }; + bool Result = ([&] { + auto HasNonTrivialField = [&](const CXXRecordDecl *R) { + for (const FieldDecl *F : R->fields()) { + if (!CanTriviallyDestruct(F->getType())) + return true; + } + return false; + }; - if (HasNonTrivialField(Cls)) - return true; + if (HasNonTrivialField(Cls)) + return true; - if (!Cls->hasDefinition()) - return false; + if (!Cls->hasDefinition()) + return false; - CXXBasePaths Paths; - Paths.setOrigin(const_cast<CXXRecordDecl *>(Cls)); - return Cls->lookupInBases( - [&](const CXXBaseSpecifier *B, CXXBasePath &) { - auto *T = B->getType().getTypePtrOrNull(); - if (!T) - return false; - auto *R = T->getAsCXXRecordDecl(); - return R && HasNonTrivialField(R); - }, - Paths, /*LookupInDependent =*/true); + CXXBasePaths Paths; + Paths.setOrigin(const_cast<CXXRecordDecl *>(Cls)); + return Cls->lookupInBases( + [&](const CXXBaseSpecifier *B, CXXBasePath &) { + auto *T = B->getType().getTypePtrOrNull(); + if (!T) + return false; + auto *R = T->getAsCXXRecordDecl(); + return R && HasNonTrivialField(R); + }, + Paths, /*LookupInDependent =*/true); + })(); + + FieldDtorCache[Cls] = Result; + + return Result; } public: @@ -955,6 +964,7 @@ class TrivialFunctionAnalysisVisitor private: CacheTy &Cache; + CacheTy FieldDtorCache; CacheTy RecursiveFn; const Stmt **OffendingStmt; }; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
