[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Sun May 12 15:22:28 PDT 2024
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/91876
>From a4b877b240ede15260f08fcb4a4622dd45a13d0a Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at apple.com>
Date: Sat, 11 May 2024 20:18:52 -0700
Subject: [PATCH 1/3] [analyzer] Allow recursive functions to be trivial.
---
.../Checkers/WebKit/PtrTypesSemantics.cpp | 18 +++++++++---------
.../Checkers/WebKit/uncounted-obj-arg.cpp | 6 ++++++
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index ad493587affa0..dd930ea4b4ab5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -498,22 +498,22 @@ class TrivialFunctionAnalysisVisitor
bool TrivialFunctionAnalysis::isTrivialImpl(
const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) {
- // 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));
+ // Treat every recursive function as trivial until otherwise proven.
+ // This guarantees each function is evaluated at most once.
+ auto [It, IsNew] = Cache.insert(std::make_pair(D, true));
if (!IsNew)
return It->second;
const Stmt *Body = D->getBody();
- if (!Body)
- return false;
+ if (!Body) {
+ Cache[D] = false;
+ return false;
+ }
TrivialFunctionAnalysisVisitor V(Cache);
bool Result = V.Visit(Body);
- if (Result)
- Cache[D] = true;
+ if (!Result)
+ Cache[D] = false;
return Result;
}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
index 073f3252160ee..39bc76197d204 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
@@ -181,6 +181,8 @@ class RefCounted {
void method();
void someFunction();
int otherFunction();
+ unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); }
+ unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); }
int trivial1() { return 123; }
float trivial2() { return 0.3; }
@@ -417,6 +419,10 @@ class UnrelatedClass {
RefCounted::singleton().trivial18(); // no-warning
RefCounted::singleton().someFunction(); // no-warning
+ getFieldTrivial().recursiveFunction(7); // no-warning
+ getFieldTrivial().recursiveComplexFunction(9);
+ // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+
getFieldTrivial().someFunction();
// expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
getFieldTrivial().nonTrivial1();
>From ceb70513d342aadb339fde6cf7ccabc96a243dfd Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at apple.com>
Date: Sat, 11 May 2024 20:24:10 -0700
Subject: [PATCH 2/3] Fix formatting.
---
clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index dd930ea4b4ab5..83a0300c627e8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -507,7 +507,7 @@ bool TrivialFunctionAnalysis::isTrivialImpl(
const Stmt *Body = D->getBody();
if (!Body) {
Cache[D] = false;
- return false;
+ return false;
}
TrivialFunctionAnalysisVisitor V(Cache);
>From 90cf7dde6e71b1d11ee0d88cd8afe0bce47531e1 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at apple.com>
Date: Sun, 12 May 2024 15:15:33 -0700
Subject: [PATCH 3/3] Fix the TrivialFunctionAnalysis::isTrivialImpl for
mutually recursive functions.
Instead of assuming every function to be initially trivial, we explicitly track
the set of functions that we're currently visting. When one of the currently visited
function is determined to be not trivial, we clear this set to signal that all
mutually recursive functions are non-trivial. We conclude that a function is trivial
when Visit() call on the function body returned true **AND** the set still contains
the function.
To implement this new algorithm, a new public function, IsFunctionTrivial,
is introduced to TrivialFunctionAnalysisVisitor, and various Visit functions in
TrivialFunctionAnalysisVisitor has been updated to use this function instead of
TrivialFunctionAnalysis::isTrivialImpl, which is now a wrapper for the function.
---
.../Checkers/WebKit/PtrTypesSemantics.cpp | 51 +++++++++++--------
.../Checkers/WebKit/uncounted-obj-arg.cpp | 20 +++++++-
2 files changed, 48 insertions(+), 23 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 83a0300c627e8..688b821421109 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -271,6 +271,30 @@ 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(D);
+ if (!IsNew)
+ return true;
+
+ const Stmt *Body = D->getBody();
+ if (!Body)
+ return false;
+
+ bool Result = Visit(Body);
+ if (!Result) // D and its mutually recursive callers are non-trivial.
+ RecursiveFn.clear();
+ else // Check if any of mutually recursive functions were non-trivial.
+ Result = RecursiveFn.contains(D);
+ RecursiveFn.erase(D);
+
+ return Result;
+ }
+
bool VisitStmt(const Stmt *S) {
// All statements are non-trivial unless overriden later.
// Don't even recurse into children by default.
@@ -357,7 +381,7 @@ class TrivialFunctionAnalysisVisitor
Name == "addressof" || Name.find("__builtin") == 0)
return true;
- return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
+ return IsFunctionTrivial(Callee);
}
bool
@@ -392,7 +416,7 @@ class TrivialFunctionAnalysisVisitor
return true;
// Recursively descend into the callee to confirm that it's trivial as well.
- return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
+ return IsFunctionTrivial(Callee);
}
bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) {
@@ -402,7 +426,7 @@ class TrivialFunctionAnalysisVisitor
if (!Callee)
return false;
// Recursively descend into the callee to confirm that it's trivial as well.
- return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
+ return IsFunctionTrivial(Callee);
}
bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E) {
@@ -428,7 +452,7 @@ class TrivialFunctionAnalysisVisitor
}
// Recursively descend into the callee to confirm that it's trivial.
- return TrivialFunctionAnalysis::isTrivialImpl(CE->getConstructor(), Cache);
+ return IsFunctionTrivial(CE->getConstructor());
}
bool VisitCXXNewExpr(const CXXNewExpr *NE) { return VisitChildren(NE); }
@@ -494,28 +518,13 @@ class TrivialFunctionAnalysisVisitor
private:
CacheTy &Cache;
+ llvm::DenseSet<llvm::PointerUnion<const Decl *>> RecursiveFn;
};
bool TrivialFunctionAnalysis::isTrivialImpl(
const Decl *D, TrivialFunctionAnalysis::CacheTy &Cache) {
- // Treat every recursive function as trivial until otherwise proven.
- // This guarantees each function is evaluated at most once.
- auto [It, IsNew] = Cache.insert(std::make_pair(D, true));
- if (!IsNew)
- return It->second;
-
- const Stmt *Body = D->getBody();
- if (!Body) {
- Cache[D] = false;
- return false;
- }
-
TrivialFunctionAnalysisVisitor V(Cache);
- bool Result = V.Visit(Body);
- if (!Result)
- Cache[D] = false;
-
- return Result;
+ return V.IsFunctionTrivial(D);
}
bool TrivialFunctionAnalysis::isTrivialImpl(
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
index 39bc76197d204..e9614ceaeae22 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
@@ -181,8 +181,15 @@ class RefCounted {
void method();
void someFunction();
int otherFunction();
- unsigned recursiveFunction(int n) { return !n ? 1 : recursiveFunction(n - 1); }
+ unsigned recursiveTrivialFunction(int n) { return !n ? 1 : recursiveTrivialFunction(n - 1); }
unsigned recursiveComplexFunction(int n) { return !n ? otherFunction() : recursiveComplexFunction(n - 1); }
+ unsigned mutuallyRecursiveFunction1(int n) { return n < 0 ? 1 : (n % 2 ? mutuallyRecursiveFunction2(n - 2) : mutuallyRecursiveFunction1(n - 1)); }
+ unsigned mutuallyRecursiveFunction2(int n) { return n < 0 ? 1 : (n % 3 ? mutuallyRecursiveFunction2(n - 3) : mutuallyRecursiveFunction1(n - 2)); }
+ unsigned mutuallyRecursiveFunction3(int n) { return n < 0 ? 1 : (n % 5 ? mutuallyRecursiveFunction3(n - 5) : mutuallyRecursiveFunction4(n - 3)); }
+ unsigned mutuallyRecursiveFunction4(int n) { return n < 0 ? 1 : (n % 7 ? otherFunction() : mutuallyRecursiveFunction3(n - 3)); }
+ unsigned mutuallyRecursiveFunction5(unsigned n) { return n > 100 ? 2 : (n % 2 ? mutuallyRecursiveFunction5(n + 1) : mutuallyRecursiveFunction6(n + 2)); }
+ unsigned mutuallyRecursiveFunction6(unsigned n) { return n > 100 ? 3 : (n % 2 ? mutuallyRecursiveFunction6(n % 7) : mutuallyRecursiveFunction7(n % 5)); }
+ unsigned mutuallyRecursiveFunction7(unsigned n) { return n > 100 ? 5 : mutuallyRecursiveFunction7(n * 5); }
int trivial1() { return 123; }
float trivial2() { return 0.3; }
@@ -419,9 +426,18 @@ class UnrelatedClass {
RefCounted::singleton().trivial18(); // no-warning
RefCounted::singleton().someFunction(); // no-warning
- getFieldTrivial().recursiveFunction(7); // no-warning
+ getFieldTrivial().recursiveTrivialFunction(7); // no-warning
getFieldTrivial().recursiveComplexFunction(9);
// expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+ getFieldTrivial().mutuallyRecursiveFunction1(11); // no-warning
+ getFieldTrivial().mutuallyRecursiveFunction2(13); // no-warning
+ getFieldTrivial().mutuallyRecursiveFunction3(17);
+ // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+ getFieldTrivial().mutuallyRecursiveFunction4(19);
+ // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+ getFieldTrivial().mutuallyRecursiveFunction5(23); // no-warning
+ getFieldTrivial().mutuallyRecursiveFunction6(29); // no-warning
+ getFieldTrivial().mutuallyRecursiveFunction7(31); // no-warning
getFieldTrivial().someFunction();
// expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
More information about the cfe-commits
mailing list