Author: Ryosuke Niwa Date: 2024-03-07T01:06:20-08:00 New Revision: 7ce1cfed9a11735f0f4ee8a3a8bebfa87ee76d07
URL: https://github.com/llvm/llvm-project/commit/7ce1cfed9a11735f0f4ee8a3a8bebfa87ee76d07 DIFF: https://github.com/llvm/llvm-project/commit/7ce1cfed9a11735f0f4ee8a3a8bebfa87ee76d07.diff LOG: [alpha.webkit.UncountedLocalVarsChecker] Allow uncounted object references within trivial statements (#82229) This PR makes alpha.webkit.UncountedLocalVarsChecker ignore raw references and pointers to a ref counted type which appears within "trival" statements. To do this, this PR extends TrivialFunctionAnalysis so that it can also analyze "triviality" of statements as well as that of functions Each Visit* function is now augmented with withCachedResult, which is responsible for looking up and updating the cache for each Visit* functions. As this PR dramatically improves the false positive rate of the checker, it also deletes the code to ignore raw pointers and references within if and for statements. Added: Modified: clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp clang/test/Analysis/Checkers/WebKit/mock-types.h clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 01b191ab0eeaf4..287f6a52870056 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -253,6 +253,19 @@ class TrivialFunctionAnalysisVisitor return true; } + template <typename CheckFunction> + bool WithCachedResult(const Stmt *S, CheckFunction Function) { + // If the statement isn't in the cache, conservatively assume that + // it's not trivial until analysis completes. Insert false to the cache + // first to avoid infinite recursion. + auto [It, IsNew] = Cache.insert(std::make_pair(S, false)); + if (!IsNew) + return It->second; + bool Result = Function(); + Cache[S] = Result; + return Result; + } + public: using CacheTy = TrivialFunctionAnalysis::CacheTy; @@ -267,7 +280,7 @@ class TrivialFunctionAnalysisVisitor bool VisitCompoundStmt(const CompoundStmt *CS) { // A compound statement is allowed as long each individual sub-statement // is trivial. - return VisitChildren(CS); + return WithCachedResult(CS, [&]() { return VisitChildren(CS); }); } bool VisitReturnStmt(const ReturnStmt *RS) { @@ -279,17 +292,36 @@ class TrivialFunctionAnalysisVisitor bool VisitDeclStmt(const DeclStmt *DS) { return VisitChildren(DS); } bool VisitDoStmt(const DoStmt *DS) { return VisitChildren(DS); } - bool VisitIfStmt(const IfStmt *IS) { return VisitChildren(IS); } + bool VisitIfStmt(const IfStmt *IS) { + return WithCachedResult(IS, [&]() { return VisitChildren(IS); }); + } + bool VisitForStmt(const ForStmt *FS) { + return WithCachedResult(FS, [&]() { return VisitChildren(FS); }); + } + bool VisitCXXForRangeStmt(const CXXForRangeStmt *FS) { + return WithCachedResult(FS, [&]() { return VisitChildren(FS); }); + } + bool VisitWhileStmt(const WhileStmt *WS) { + return WithCachedResult(WS, [&]() { return VisitChildren(WS); }); + } bool VisitSwitchStmt(const SwitchStmt *SS) { return VisitChildren(SS); } bool VisitCaseStmt(const CaseStmt *CS) { return VisitChildren(CS); } bool VisitDefaultStmt(const DefaultStmt *DS) { return VisitChildren(DS); } bool VisitUnaryOperator(const UnaryOperator *UO) { // Operator '*' and '!' are allowed as long as the operand is trivial. - if (UO->getOpcode() == UO_Deref || UO->getOpcode() == UO_AddrOf || - UO->getOpcode() == UO_LNot) + auto op = UO->getOpcode(); + if (op == UO_Deref || op == UO_AddrOf || op == UO_LNot) return Visit(UO->getSubExpr()); + if (UO->isIncrementOp() || UO->isDecrementOp()) { + // Allow increment or decrement of a POD type. + if (auto *RefExpr = dyn_cast<DeclRefExpr>(UO->getSubExpr())) { + if (auto *Decl = dyn_cast<VarDecl>(RefExpr->getDecl())) + return Decl->isLocalVarDeclOrParm() && + Decl->getType().isPODType(Decl->getASTContext()); + } + } // Other operators are non-trivial. return false; } @@ -304,22 +336,6 @@ class TrivialFunctionAnalysisVisitor return VisitChildren(CO); } - bool VisitDeclRefExpr(const DeclRefExpr *DRE) { - if (auto *decl = DRE->getDecl()) { - if (isa<ParmVarDecl>(decl)) - return true; - if (isa<EnumConstantDecl>(decl)) - return true; - if (auto *VD = dyn_cast<VarDecl>(decl)) { - if (VD->hasConstantInitialization() && VD->getEvaluatedValue()) - return true; - auto *Init = VD->getInit(); - return !Init || Visit(Init); - } - } - return false; - } - bool VisitAtomicExpr(const AtomicExpr *E) { return VisitChildren(E); } bool VisitStaticAssertDecl(const StaticAssertDecl *SAD) { @@ -436,6 +452,11 @@ class TrivialFunctionAnalysisVisitor return true; } + bool VisitDeclRefExpr(const DeclRefExpr *DRE) { + // The use of a variable is trivial. + return true; + } + // Constant literal expressions are always trivial bool VisitIntegerLiteral(const IntegerLiteral *E) { return true; } bool VisitFloatingLiteral(const FloatingLiteral *E) { return true; } @@ -449,7 +470,7 @@ class TrivialFunctionAnalysisVisitor } private: - CacheTy Cache; + CacheTy &Cache; }; bool TrivialFunctionAnalysis::isTrivialImpl( @@ -474,4 +495,17 @@ bool TrivialFunctionAnalysis::isTrivialImpl( return Result; } +bool TrivialFunctionAnalysis::isTrivialImpl( + const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache) { + // If the statement isn't in the cache, conservatively assume that + // it's not trivial until analysis completes. Unlike a function case, + // we don't insert an entry into the cache until Visit returns + // since Visit* functions themselves make use of the cache. + + TrivialFunctionAnalysisVisitor V(Cache); + bool Result = V.Visit(S); + assert(Cache.contains(S) && "Top-level statement not properly cached!"); + return Result; +} + } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index e07cd31395747d..9ed8e7cab6abb9 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -11,6 +11,7 @@ #include "llvm/ADT/APInt.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/PointerUnion.h" #include <optional> namespace clang { @@ -19,6 +20,7 @@ class CXXMethodDecl; class CXXRecordDecl; class Decl; class FunctionDecl; +class Stmt; class Type; // Ref-countability of a type is implicitly defined by Ref<T> and RefPtr<T> @@ -71,14 +73,17 @@ 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); } private: friend class TrivialFunctionAnalysisVisitor; - using CacheTy = llvm::DenseMap<const Decl *, bool>; + using CacheTy = + 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); }; } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 5a72f53b12edaa..6036ad58cf253c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -26,28 +26,6 @@ using namespace ento; namespace { -// for ( int a = ...) ... true -// for ( int a : ...) ... true -// if ( int* a = ) ... true -// anything else ... false -bool isDeclaredInForOrIf(const VarDecl *Var) { - assert(Var); - auto &ASTCtx = Var->getASTContext(); - auto parent = ASTCtx.getParents(*Var); - - if (parent.size() == 1) { - if (auto *DS = parent.begin()->get<DeclStmt>()) { - DynTypedNodeList grandParent = ASTCtx.getParents(*DS); - if (grandParent.size() == 1) { - return grandParent.begin()->get<ForStmt>() || - grandParent.begin()->get<IfStmt>() || - grandParent.begin()->get<CXXForRangeStmt>(); - } - } - } - return false; -} - // FIXME: should be defined by anotations in the future bool isRefcountedStringsHack(const VarDecl *V) { assert(V); @@ -143,6 +121,11 @@ class UncountedLocalVarsChecker // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> { const UncountedLocalVarsChecker *Checker; + + TrivialFunctionAnalysis TFA; + + using Base = RecursiveASTVisitor<LocalVisitor>; + explicit LocalVisitor(const UncountedLocalVarsChecker *Checker) : Checker(Checker) { assert(Checker); @@ -155,6 +138,36 @@ class UncountedLocalVarsChecker Checker->visitVarDecl(V); return true; } + + bool TraverseIfStmt(IfStmt *IS) { + if (!TFA.isTrivial(IS)) + return Base::TraverseIfStmt(IS); + return true; + } + + bool TraverseForStmt(ForStmt *FS) { + if (!TFA.isTrivial(FS)) + return Base::TraverseForStmt(FS); + return true; + } + + bool TraverseCXXForRangeStmt(CXXForRangeStmt *FRS) { + if (!TFA.isTrivial(FRS)) + return Base::TraverseCXXForRangeStmt(FRS); + return true; + } + + bool TraverseWhileStmt(WhileStmt *WS) { + if (!TFA.isTrivial(WS)) + return Base::TraverseWhileStmt(WS); + return true; + } + + bool TraverseCompoundStmt(CompoundStmt *CS) { + if (!TFA.isTrivial(CS)) + return Base::TraverseCompoundStmt(CS); + return true; + } }; LocalVisitor visitor(this); @@ -189,18 +202,16 @@ class UncountedLocalVarsChecker dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) { const auto *MaybeGuardianArgType = MaybeGuardian->getType().getTypePtr(); - if (!MaybeGuardianArgType) - return; - const CXXRecordDecl *const MaybeGuardianArgCXXRecord = - MaybeGuardianArgType->getAsCXXRecordDecl(); - if (!MaybeGuardianArgCXXRecord) - return; - - if (MaybeGuardian->isLocalVarDecl() && - (isRefCounted(MaybeGuardianArgCXXRecord) || - isRefcountedStringsHack(MaybeGuardian)) && - isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) { - return; + if (MaybeGuardianArgType) { + const CXXRecordDecl *const MaybeGuardianArgCXXRecord = + MaybeGuardianArgType->getAsCXXRecordDecl(); + if (MaybeGuardianArgCXXRecord) { + if (MaybeGuardian->isLocalVarDecl() && + (isRefCounted(MaybeGuardianArgCXXRecord) || + isRefcountedStringsHack(MaybeGuardian)) && + isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) + return; + } } // Parameters are guaranteed to be safe for the duration of the call @@ -219,9 +230,6 @@ class UncountedLocalVarsChecker if (!V->isLocalVarDecl()) return true; - if (isDeclaredInForOrIf(V)) - return true; - return false; } diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index e2b3401d407392..aab99197dfa49e 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -62,6 +62,8 @@ struct RefCountable { static Ref<RefCountable> create(); void ref() {} void deref() {} + void method(); + int trivial() { return 123; } }; template <typename T> T *downcast(T *t) { return t; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index 0fcd3b21376caf..00673e91f471ea 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -2,6 +2,8 @@ #include "mock-types.h" +void someFunction(); + namespace raw_ptr { void foo() { RefCountable *bar; @@ -16,6 +18,13 @@ void foo_ref() { RefCountable automatic; RefCountable &bar = automatic; // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + someFunction(); + bar.method(); +} + +void foo_ref_trivial() { + RefCountable automatic; + RefCountable &bar = automatic; } void bar_ref(RefCountable &) {} @@ -32,6 +41,8 @@ void foo2() { // missing embedded scope here RefCountable *bar = foo.get(); // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + someFunction(); + bar->method(); } void foo3() { @@ -47,11 +58,35 @@ void foo4() { { RefCountable *bar = foo.get(); } } } + +void foo5() { + RefPtr<RefCountable> foo; + auto* bar = foo.get(); + bar->trivial(); +} + +void foo6() { + RefPtr<RefCountable> foo; + auto* bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + bar->method(); +} + +struct SelfReferencingStruct { + SelfReferencingStruct* ptr; + RefCountable* obj { nullptr }; +}; + +void foo7(RefCountable* obj) { + SelfReferencingStruct bar = { &bar, obj }; + bar.obj->method(); +} + } // namespace guardian_scopes namespace auto_keyword { class Foo { - RefCountable *provide_ref_ctnbl() { return nullptr; } + RefCountable *provide_ref_ctnbl(); void evil_func() { RefCountable *bar = provide_ref_ctnbl(); @@ -62,13 +97,24 @@ class Foo { // expected-warning@-1{{Local variable 'baz2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} [[clang::suppress]] auto *baz_suppressed = provide_ref_ctnbl(); // no-warning } + + void func() { + RefCountable *bar = provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + if (bar) + bar->method(); + } }; } // namespace auto_keyword namespace guardian_casts { void foo1() { RefPtr<RefCountable> foo; - { RefCountable *bar = downcast<RefCountable>(foo.get()); } + { + RefCountable *bar = downcast<RefCountable>(foo.get()); + bar->method(); + } + foo->method(); } void foo2() { @@ -76,6 +122,7 @@ void foo2() { { RefCountable *bar = static_cast<RefCountable *>(downcast<RefCountable>(foo.get())); + someFunction(); } } } // namespace guardian_casts @@ -83,7 +130,11 @@ void foo2() { namespace guardian_ref_conversion_operator { void foo() { Ref<RefCountable> rc; - { RefCountable &rr = rc; } + { + RefCountable &rr = rc; + rr.method(); + someFunction(); + } } } // namespace guardian_ref_conversion_operator @@ -92,9 +143,47 @@ RefCountable *provide_ref_ctnbl() { return nullptr; } void foo() { // no warnings - if (RefCountable *a = provide_ref_ctnbl()) { } - for (RefCountable *a = provide_ref_ctnbl(); a != nullptr;) { } + if (RefCountable *a = provide_ref_ctnbl()) + a->trivial(); + for (RefCountable *b = provide_ref_ctnbl(); b != nullptr;) + b->trivial(); RefCountable *array[1]; - for (RefCountable *a : array) { } + for (RefCountable *c : array) + c->trivial(); + while (RefCountable *d = provide_ref_ctnbl()) + d->trivial(); + do { + RefCountable *e = provide_ref_ctnbl(); + e->trivial(); + } while (1); + someFunction(); } + +void bar() { + if (RefCountable *a = provide_ref_ctnbl()) { + // expected-warning@-1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + a->method(); + } + for (RefCountable *b = provide_ref_ctnbl(); b != nullptr;) { + // expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + b->method(); + } + RefCountable *array[1]; + for (RefCountable *c : array) { + // expected-warning@-1{{Local variable 'c' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + c->method(); + } + + while (RefCountable *d = provide_ref_ctnbl()) { + // expected-warning@-1{{Local variable 'd' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + d->method(); + } + do { + RefCountable *e = provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'e' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + e->method(); + } while (1); + someFunction(); +} + } // namespace ignore_for_if _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
