[clang] 71b81e9 - [alpha.webkit.UncountedLocalVarsChecker] Recursive functions are erroneously treated as non-trivial (#110973)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 17 16:52:35 PDT 2024
Author: Ryosuke Niwa
Date: 2024-10-17T16:52:31-07:00
New Revision: 71b81e93d28c8db3f9cfa1d715c925a98ae4b153
URL: https://github.com/llvm/llvm-project/commit/71b81e93d28c8db3f9cfa1d715c925a98ae4b153
DIFF: https://github.com/llvm/llvm-project/commit/71b81e93d28c8db3f9cfa1d715c925a98ae4b153.diff
LOG: [alpha.webkit.UncountedLocalVarsChecker] Recursive functions are erroneously treated as non-trivial (#110973)
This PR fixes the bug that alpha.webkit.UncountedLocalVarsChecker
erroneously treats a trivial recursive function as non-trivial. This was
caused by TrivialFunctionAnalysis::isTrivialImpl which takes a statement
as an argument populating the cache with "false" while traversing the
statement to determine its triviality within a recursive function in
TrivialFunctionAnalysisVisitor's WithCachedResult. Because
IsFunctionTrivial honors an entry in the cache, this resulted in the
whole function to be treated as non-trivial.
Thankfully, TrivialFunctionAnalysisVisitor::IsFunctionTrivial already
handles recursive functions correctly so this PR applies the same logic
to TrivialFunctionAnalysisVisitor::WithCachedResult by sharing code
between the two functions. This avoids the cache to be pre-populated
with "false" while traversing statements in a recurisve function.
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 2298fe39850de5..e043806eadd6ac 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -273,16 +273,29 @@ 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));
+ template <typename StmtOrDecl, typename CheckFunction>
+ bool WithCachedResult(const StmtOrDecl *S, CheckFunction Function) {
+ auto CacheIt = Cache.find(S);
+ if (CacheIt != Cache.end())
+ return CacheIt->second;
+
+ // Treat a recursive statement to be trivial until proven otherwise.
+ auto [RecursiveIt, IsNew] = RecursiveFn.insert(std::make_pair(S, true));
if (!IsNew)
- return It->second;
+ return RecursiveIt->second;
+
bool Result = Function();
+
+ if (!Result) {
+ for (auto &It : RecursiveFn)
+ It.second = false;
+ }
+ RecursiveIt = RecursiveFn.find(S);
+ assert(RecursiveIt != RecursiveFn.end());
+ Result = RecursiveIt->second;
+ RecursiveFn.erase(RecursiveIt);
Cache[S] = Result;
+
return Result;
}
@@ -292,16 +305,7 @@ class TrivialFunctionAnalysisVisitor
TrivialFunctionAnalysisVisitor(CacheTy &Cache) : Cache(Cache) {}
bool IsFunctionTrivial(const Decl *D) {
- auto CacheIt = Cache.find(D);
- if (CacheIt != Cache.end())
- return CacheIt->second;
-
- // Treat a recursive function call to be trivial until proven otherwise.
- auto [RecursiveIt, IsNew] = RecursiveFn.insert(std::make_pair(D, true));
- if (!IsNew)
- return RecursiveIt->second;
-
- bool Result = [&]() {
+ return WithCachedResult(D, [&]() {
if (auto *CtorDecl = dyn_cast<CXXConstructorDecl>(D)) {
for (auto *CtorInit : CtorDecl->inits()) {
if (!Visit(CtorInit->getInit()))
@@ -312,20 +316,7 @@ class TrivialFunctionAnalysisVisitor
if (!Body)
return false;
return Visit(Body);
- }();
-
- if (!Result) {
- // D and its mutually recursive callers are all non-trivial.
- for (auto &It : RecursiveFn)
- It.second = false;
- }
- RecursiveIt = RecursiveFn.find(D);
- assert(RecursiveIt != RecursiveFn.end());
- Result = RecursiveIt->second;
- RecursiveFn.erase(RecursiveIt);
- Cache[D] = Result;
-
- return Result;
+ });
}
bool VisitStmt(const Stmt *S) {
@@ -586,11 +577,6 @@ bool TrivialFunctionAnalysis::isTrivialImpl(
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!");
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 25776870dd3ae0..b5f6b8535bf418 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -289,3 +289,42 @@ void foo() {
}
} // namespace local_assignment_to_global
+
+namespace local_var_in_recursive_function {
+
+struct TreeNode {
+ Ref<TreeNode> create() { return Ref(*new TreeNode); }
+
+ void ref() const { ++refCount; }
+ void deref() const {
+ if (!--refCount)
+ delete this;
+ }
+
+ int recursiveCost();
+ int recursiveWeight();
+ int weight();
+
+ int cost { 0 };
+ mutable unsigned refCount { 0 };
+ TreeNode* nextSibling { nullptr };
+ TreeNode* firstChild { nullptr };
+};
+
+int TreeNode::recursiveCost() {
+ // no warnings
+ unsigned totalCost = cost;
+ for (TreeNode* node = firstChild; node; node = node->nextSibling)
+ totalCost += recursiveCost();
+ return totalCost;
+}
+
+int TreeNode::recursiveWeight() {
+ unsigned totalCost = weight();
+ for (TreeNode* node = firstChild; node; node = node->nextSibling)
+ // expected-warning at -1{{Local variable 'node' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ totalCost += recursiveWeight();
+ return totalCost;
+}
+
+} // namespace local_var_in_recursive_function
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
index 1a42de90105a55..10da776f81575c 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
@@ -259,6 +259,15 @@ class RefCounted {
void mutuallyRecursive8() { mutuallyRecursive9(); someFunction(); }
void mutuallyRecursive9() { mutuallyRecursive8(); }
+ int recursiveCost() {
+ unsigned totalCost = 0;
+ for (unsigned i = 0; i < sizeof(children)/sizeof(*children); ++i) {
+ if (auto* child = children[i])
+ totalCost += child->recursiveCost();
+ }
+ return totalCost;
+ }
+
int trivial1() { return 123; }
float trivial2() { return 0.3; }
float trivial3() { return (float)0.4; }
@@ -448,6 +457,7 @@ class RefCounted {
Number* number { nullptr };
ComplexNumber complex;
Enum enumValue { Enum::Value1 };
+ RefCounted* children[4];
};
unsigned RefCounted::s_v = 0;
@@ -558,6 +568,8 @@ class UnrelatedClass {
getFieldTrivial().mutuallyRecursive9();
// expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+ getFieldTrivial().recursiveCost(); // no-warning
+
getFieldTrivial().someFunction();
// expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
getFieldTrivial().nonTrivial1();
More information about the cfe-commits
mailing list