llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/181576.diff 6 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/NoDeleteChecker.cpp (+12-1) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+60-1) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+5) - (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+14-4) - (modified) clang/test/Analysis/Checkers/WebKit/nodelete-annotation.cpp (+30) - (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+5) ``````````diff 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..bdea5e208a7d1 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,43 @@ 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) { + // Get T out of const T& / T&&. + if (auto *RT = dyn_cast<ReferenceType>(Type)) { + 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 +620,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 +780,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)) @@ -857,4 +910,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..99b07691f6e75 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..0f4efadef4e6b 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(); @@ -29,6 +31,7 @@ void [[clang::annotate_type("webkit.nodelete")]] defWithNoDelete() { } 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 +60,33 @@ 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(RefPtr<RefCountable>&& 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")]] 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); + } + +private: + RefPtr<RefCountable> m_obj; }; 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() { `````````` </details> https://github.com/llvm/llvm-project/pull/181576 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
