[clang] 7ce1cfe - [alpha.webkit.UncountedLocalVarsChecker] Allow uncounted object references within trivial statements (#82229)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 7 01:06:24 PST 2024
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 at -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 at -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 at -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 at -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 at -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 at -1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ a->method();
+ }
+ for (RefCountable *b = provide_ref_ctnbl(); b != nullptr;) {
+ // expected-warning at -1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ b->method();
+ }
+ RefCountable *array[1];
+ for (RefCountable *c : array) {
+ // expected-warning at -1{{Local variable 'c' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ c->method();
+ }
+
+ while (RefCountable *d = provide_ref_ctnbl()) {
+ // expected-warning at -1{{Local variable 'd' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ d->method();
+ }
+ do {
+ RefCountable *e = provide_ref_ctnbl();
+ // expected-warning at -1{{Local variable 'e' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ e->method();
+ } while (1);
+ someFunction();
+}
+
} // namespace ignore_for_if
More information about the cfe-commits
mailing list