[alpha.webkit.UncountedLocalVarsChecker] Allow uncounted object references within trivial statements #82229
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesThis 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. Patch is 22.59 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/82229.diff 5 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 6f236db0474079..9be1f10d097ab5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -244,18 +244,42 @@ class TrivialFunctionAnalysisVisitor
// Returns false if at least one child is non-trivial.
bool VisitChildren(const Stmt *S) {
- for (const Stmt *Child : S->children()) {
- if (Child && !Visit(Child))
+ return withCachedResult(S, [&]() {
+ for (const Stmt *Child : S->children()) {
+ if (Child && !Visit(Child))
+ return false;
+ }
+ return true;
+ });
+ }
+
+ bool VisitSubExpr(const Expr *E) {
+ return withCachedResult(E, [&]() {
+ if (!Visit(E))
return false;
- }
+ return true;
+ });
+ }
- return true;
+ template <typename StmtType, typename CheckFunction>
+ bool withCachedResult(const StmtType *S, CheckFunction Function) {
+ auto It = StatementCache.find(S);
+ if (It != StatementCache.end())
+ return It->second;
+ bool Result = Function();
+ StatementCache[S] = Result;
+ return Result;
}
public:
- using CacheTy = TrivialFunctionAnalysis::CacheTy;
+ using FunctionCacheTy = TrivialFunctionAnalysis::FunctionCacheTy;
+ using StatementCacheTy = TrivialFunctionAnalysis::StatementCacheTy;
- TrivialFunctionAnalysisVisitor(CacheTy &Cache) : Cache(Cache) {}
+ TrivialFunctionAnalysisVisitor(FunctionCacheTy &FunctionCache,
+ StatementCacheTy &StatementCache)
+ : FunctionCache(FunctionCache)
+ , StatementCache(StatementCache) {
+ }
bool VisitStmt(const Stmt *S) {
// All statements are non-trivial unless overriden later.
@@ -271,13 +295,21 @@ 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 true;
+ return withCachedResult(RS, [&]() {
+ if (auto *RV = RS->getRetValue())
+ return Visit(RV);
+ return true;
+ });
+ }
+
+ bool VisitCXXForRangeStmt(const CXXForRangeStmt *FS) {
+ return VisitChildren(FS);
}
bool VisitDeclStmt(const DeclStmt *DS) { return VisitChildren(DS); }
bool VisitDoStmt(const DoStmt *DS) { return VisitChildren(DS); }
+ bool VisitForStmt(const ForStmt *FS) { return VisitChildren(FS); }
+ bool VisitWhileStmt(const WhileStmt *WS) { return VisitChildren(WS); }
bool VisitIfStmt(const IfStmt *IS) { return VisitChildren(IS); }
bool VisitSwitchStmt(const SwitchStmt *SS) { return VisitChildren(SS); }
bool VisitCaseStmt(const CaseStmt *CS) { return VisitChildren(CS); }
@@ -285,17 +317,27 @@ class TrivialFunctionAnalysisVisitor
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)
- return Visit(UO->getSubExpr());
-
- // Other operators are non-trivial.
- return false;
+ return withCachedResult(UO, [&]() {
+ auto op = UO->getOpcode();
+ if (op == UO_Deref || op == UO_AddrOf || op == UO_LNot)
+ return Visit(UO->getSubExpr());
+ if (UO->isIncrementOp() || UO->isDecrementOp()) {
+ 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;
+ });
}
bool VisitBinaryOperator(const BinaryOperator *BO) {
// Binary operators are trivial if their operands are trivial.
- return Visit(BO->getLHS()) && Visit(BO->getRHS());
+ return withCachedResult(BO, [&]() {
+ return Visit(BO->getLHS()) && Visit(BO->getRHS());
+ });
}
bool VisitConditionalOperator(const ConditionalOperator *CO) {
@@ -304,15 +346,20 @@ class TrivialFunctionAnalysisVisitor
}
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))
- return VD->hasConstantInitialization() && VD->getEvaluatedValue();
- }
- return false;
+ return withCachedResult(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;
+ return VD->getInit() ? Visit(VD->getInit()) : true;
+ }
+ }
+ return false;
+ });
}
bool VisitStaticAssertDecl(const StaticAssertDecl *SAD) {
@@ -321,39 +368,45 @@ class TrivialFunctionAnalysisVisitor
}
bool VisitCallExpr(const CallExpr *CE) {
- if (!checkArguments(CE))
- return false;
+ return withCachedResult(CE, [&]() {
+ if (!checkArguments(CE))
+ return false;
- auto *Callee = CE->getDirectCallee();
- if (!Callee)
- return false;
- const auto &Name = safeGetName(Callee);
+ auto *Callee = CE->getDirectCallee();
+ if (!Callee)
+ return false;
+ const auto &Name = safeGetName(Callee);
- if (Name == "WTFCrashWithInfo" || Name == "WTFBreakpointTrap" ||
- Name == "compilerFenceForCrash" || Name == "__builtin_unreachable")
- return true;
+ if (Name == "WTFCrashWithInfo" || Name == "WTFBreakpointTrap" ||
+ Name == "compilerFenceForCrash" || Name == "__builtin_unreachable")
+ return true;
- return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
+ return TrivialFunctionAnalysis::isTrivialImpl(Callee, FunctionCache,
+ StatementCache);
+ });
}
bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
- if (!checkArguments(MCE))
- return false;
+ return withCachedResult(MCE, [&]() {
+ if (!checkArguments(MCE))
+ return false;
- bool TrivialThis = Visit(MCE->getImplicitObjectArgument());
- if (!TrivialThis)
- return false;
+ bool TrivialThis = Visit(MCE->getImplicitObjectArgument());
+ if (!TrivialThis)
+ return false;
- auto *Callee = MCE->getMethodDecl();
- if (!Callee)
- return false;
+ auto *Callee = MCE->getMethodDecl();
+ if (!Callee)
+ return false;
- std::optional<bool> IsGetterOfRefCounted = isGetterOfRefCounted(Callee);
- if (IsGetterOfRefCounted && *IsGetterOfRefCounted)
- return true;
+ std::optional<bool> IsGetterOfRefCounted = isGetterOfRefCounted(Callee);
+ if (IsGetterOfRefCounted && *IsGetterOfRefCounted)
+ return true;
- // Recursively descend into the callee to confirm that it's trivial as well.
- return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
+ // Recursively descend into the callee to confirm it's trivial as well.
+ return TrivialFunctionAnalysis::isTrivialImpl(Callee, FunctionCache,
+ StatementCache);
+ });
}
bool checkArguments(const CallExpr *CE) {
@@ -365,44 +418,52 @@ class TrivialFunctionAnalysisVisitor
}
bool VisitCXXConstructExpr(const CXXConstructExpr *CE) {
- for (const Expr *Arg : CE->arguments()) {
- if (Arg && !Visit(Arg))
- return false;
- }
+ return withCachedResult(CE, [&]() {
+ for (const Expr *Arg : CE->arguments()) {
+ if (Arg && !Visit(Arg))
+ return false;
+ }
- // Recursively descend into the callee to confirm that it's trivial.
- return TrivialFunctionAnalysis::isTrivialImpl(CE->getConstructor(), Cache);
+ // Recursively descend into the callee to confirm that it's trivial.
+ return TrivialFunctionAnalysis::isTrivialImpl(CE->getConstructor(),
+ FunctionCache,
+ StatementCache);
+ });
}
bool VisitImplicitCastExpr(const ImplicitCastExpr *ICE) {
- return Visit(ICE->getSubExpr());
+ return VisitSubExpr(ICE->getSubExpr());
}
bool VisitExplicitCastExpr(const ExplicitCastExpr *ECE) {
- return Visit(ECE->getSubExpr());
+ return VisitSubExpr(ECE->getSubExpr());
}
bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *VMT) {
- return Visit(VMT->getSubExpr());
+ return VisitSubExpr(VMT->getSubExpr());
}
bool VisitExprWithCleanups(const ExprWithCleanups *EWC) {
- return Visit(EWC->getSubExpr());
+ return VisitSubExpr(EWC->getSubExpr());
}
- bool VisitParenExpr(const ParenExpr *PE) { return Visit(PE->getSubExpr()); }
+ bool VisitParenExpr(const ParenExpr *PE) {
+ return VisitSubExpr(PE->getSubExpr());
+ }
bool VisitInitListExpr(const InitListExpr *ILE) {
- for (const Expr *Child : ILE->inits()) {
- if (Child && !Visit(Child))
- return false;
- }
- return true;
+ return withCachedResult(ILE, [&]() {
+ for (const Expr *Child : ILE->inits()) {
+ if (Child && !Visit(Child))
+ return false;
+ }
+ return true;
+ });
}
bool VisitMemberExpr(const MemberExpr *ME) {
// Field access is allowed but the base pointer may itself be non-trivial.
- return Visit(ME->getBase());
+ return VisitSubExpr(ME->getBase());
}
bool VisitCXXThisExpr(const CXXThisExpr *CTE) {
@@ -428,16 +489,18 @@ class TrivialFunctionAnalysisVisitor
}
private:
- CacheTy Cache;
+ FunctionCacheTy FunctionCache;
+ StatementCacheTy StatementCache;
};
bool TrivialFunctionAnalysis::isTrivialImpl(
- const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) {
+ const Decl *D, TrivialFunctionAnalysis::FunctionCacheTy &FunctionCache,
+ TrivialFunctionAnalysis::StatementCacheTy &StatementCache) {
// If the function isn't in the cache, conservatively assume that
// it's not trivial until analysis completes. This makes every recursive
// function non-trivial. This also guarantees that each function
// will be scanned at most once.
- auto [It, IsNew] = Cache.insert(std::make_pair(D, false));
+ auto [It, IsNew] = FunctionCache.insert(std::make_pair(D, false));
if (!IsNew)
return It->second;
@@ -445,10 +508,29 @@ bool TrivialFunctionAnalysis::isTrivialImpl(
if (!Body)
return false;
- TrivialFunctionAnalysisVisitor V(Cache);
+ TrivialFunctionAnalysisVisitor V(FunctionCache, StatementCache);
bool Result = V.Visit(Body);
if (Result)
- Cache[D] = true;
+ FunctionCache[D] = true;
+
+ return Result;
+}
+
+bool TrivialFunctionAnalysis::isTrivialImpl(
+ const Stmt *S, TrivialFunctionAnalysis::FunctionCacheTy &FunctionCache,
+ TrivialFunctionAnalysis::StatementCacheTy &StatementCache) {
+ // 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.
+
+ auto It = StatementCache.find(S);
+ if (It != StatementCache.end())
+ return It->second;
+
+ TrivialFunctionAnalysisVisitor V(FunctionCache, StatementCache);
+ bool Result = V.Visit(S);
+ StatementCache[S] = Result;
return Result;
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index e07cd31395747d..3f4cdd1f2ffb02 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -19,6 +19,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>
@@ -70,15 +71,27 @@ bool isSingleton(const FunctionDecl *F);
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 Decl *D) const {
+ return isTrivialImpl(D, TheFunctionCache, TheStatementCache);
+ }
+
+ bool isTrivial(const Stmt *S) const {
+ return isTrivialImpl(S, TheFunctionCache, TheStatementCache);
+ }
private:
friend class TrivialFunctionAnalysisVisitor;
- using CacheTy = llvm::DenseMap<const Decl *, bool>;
- mutable CacheTy TheCache{};
+ using FunctionCacheTy = llvm::DenseMap<const Decl *, bool>;
+ mutable FunctionCacheTy TheFunctionCache{};
+
+ using StatementCacheTy = llvm::DenseMap<const Stmt *, bool>;
+ mutable StatementCacheTy TheStatementCache{};
- static bool isTrivialImpl(const Decl *D, CacheTy &Cache);
+ static bool isTrivialImpl(const Decl *D, FunctionCacheTy &FunctionCache,
+ StatementCacheTy &StatementCache);
+ static bool isTrivialImpl(const Stmt *S, FunctionCacheTy &FunctionCache,
+ StatementCacheTy &StatementCache);
};
} // namespace clang
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 5a72f53b12edaa..547e7fc089844f 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);
@@ -133,6 +111,8 @@ class UncountedLocalVarsChecker
"WebKit coding guidelines"};
mutable BugReporter *BR;
+ TrivialFunctionAnalysis TFA;
+
public:
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
BugReporter &BRArg) const {
@@ -171,6 +151,24 @@ class UncountedLocalVarsChecker
std::optional<bool> IsUncountedPtr = isUncountedPtr(ArgType);
if (IsUncountedPtr && *IsUncountedPtr) {
+
+ ASTContext &ctx = V->getASTContext();
+ for (DynTypedNodeList ancestors = ctx.getParents(*V); !ancestors.empty();
+ ancestors = ctx.getParents(*ancestors.begin())) {
+ for (auto &ancestor : ancestors) {
+ if (auto *S = ancestor.get<IfStmt>(); S && TFA.isTrivial(S))
+ return;
+ if (auto *S = ancestor.get<ForStmt>(); S && TFA.isTrivial(S))
+ return;
+ if (auto *S = ancestor.get<CXXForRangeStmt>(); S && TFA.isTrivial(S))
+ return;
+ if (auto *S = ancestor.get<WhileStmt>(); S && TFA.isTrivial(S))
+ return;
+ if (auto *S = ancestor.get<CompoundStmt>(); S && TFA.isTrivial(S))
+ return;
+ }
+ }
+
const Expr *const InitExpr = V->getInit();
if (!InitExpr)
return; // FIXME: later on we might warn on uninitialized vars too
@@ -178,6 +176,7 @@ class UncountedLocalVarsChecker
const clang::Expr *const InitArgOrigin =
tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false)
.first;
+
if (!InitArgOrigin)
return;
@@ -189,20 +188,17 @@ 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
// by another checker.
if (isa<ParmVarDecl>(MaybeGuardian))
@@ -219,9 +215,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 d08a997aa8c043..67fe382e8e2038 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -45,6 +45,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..3fe04f775fbbcb 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,25 @@ 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();
+}
+
} // 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 +87,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();
+ // expect...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
rniwa
force-pushed
the
ignore-trivial-statements
branch
4 times, most recently
from
383c7bf to
8b2cf0b
Compare
|
Ooo interesting. I want to spend a bit more time thinking whether this has to go into every callback, maybe it can be a set-and-forget thing? (Probably not.) Also it might be a good idea to cache only statements that may ever get directly queried. (This seems to be exactly necessary and sufficient.) For now they're just the 5 control flow statements right? Maybe add caching to these 5 callbacks and skip the rest? (I suspect it may matter for performance as well, dunno hard to tell though.) |
rniwa
force-pushed
the
ignore-trivial-statements
branch
from
8b2cf0b to
0996406
Compare
What do you mean by set-and-forget? |
I mean like, so that you didn't have to remember to edit every callback in a specific way in order to have caching for every callback. But it might also be a bad idea given that we probably don't actually want every callback to be cached. But maybe there's a way to also make it flexible, dunno. Yeah I don't immediately see how to do that, your solution is probably perfect. Can you confirm that only 5 callbacks currently need to be cached? If yes, it's probably be great to just edit them manually to add caching when necessary, because now it's a conscious decision. |
haoNoQ
reviewed
Feb 29, 2024
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
Outdated
Show resolved
Hide resolved
rniwa
force-pushed
the
ignore-trivial-statements
branch
2 times, most recently
from
394facd to
52c6e51
Compare
…ences within trivial statements 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.
…raversing the AST context up to find trivial statements.
rniwa
force-pushed
the
ignore-trivial-statements
branch
from
52c6e51 to
3a3448f
Compare
haoNoQ
reviewed
Mar 4, 2024
| isRefcountedStringsHack(MaybeGuardian)) && | ||
| isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nothing really changed here right? Also coding guidelines actually prefer early returns (https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code).
There was a problem hiding this comment.
So before this PR, those early exits took place when MaybeGuardianArgType was nullptr or if MaybeGuardianArgType wasn't a CXXRecordDecl. This PR changes so that we only return when all these conditions hold true or when it's a ParmVarDecl.
There was a problem hiding this comment.
Specifically, without this change, we'd get the following error:
clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp Line 162 (directive at clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp:163):
Local variable 'c' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]
Also made TrivialFunctionAnalysisVisitor's Cache a reference instead of a copy so that TrivialFunctionAnalysis::isTrivialImpl can retrieve the result out of the cache.
|
Actually, we need to cache the results for |
…iable declaration could end up in an infinite recursion without it.
haoNoQ
approved these changes
Mar 7, 2024
…ializer's triviality.
This was referenced Mar 7, 2024
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Mar 8, 2024
…ences within trivial statements (llvm#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.