[clang] [analyzer] Allow recursive functions to be trivial. (PR #91876)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Sun May 12 23:49:14 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/6] [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/6] 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/6] 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}}
>From 1b3883864539ebf6006d68a8db031a58b04b3d8c Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at apple.com>
Date: Sun, 12 May 2024 15:26:36 -0700
Subject: [PATCH 4/6] 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 688b821421109..3583a017831ab 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -288,7 +288,7 @@ class TrivialFunctionAnalysisVisitor
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.
+ else // Check if any of mutually recursive functions were non-trivial.
Result = RecursiveFn.contains(D);
RecursiveFn.erase(D);
>From 0e567ace33412f69a090b1a92377dd4f598e76d8 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at apple.com>
Date: Sun, 12 May 2024 23:09:18 -0700
Subject: [PATCH 5/6] Fix a bug that IsFunctionTrivial was not updating the
cache upon completion.
Also fix a bug that IsFunctionTrivial would do a redundant traversal when
a function had been determined to be non-trivial because it's indistinguishable
if a given function had not been traversed or had been found to be non-trivial
because we were using the absense of the function in the hash set to indicate
the non-triviality of a function. Use a hash map of a function to a boolean
instead to explicitly track whether a given function had been visited or not,
and whether a given function had been determined to be non-trivial or not.
---
.../Checkers/WebKit/PtrTypesSemantics.cpp | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 3583a017831ab..774390ce400fc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -277,20 +277,23 @@ class TrivialFunctionAnalysisVisitor
return CacheIt->second;
// Treat a recursive function call to be trivial until proven otherwise.
- auto [RecursiveIt, IsNew] = RecursiveFn.insert(D);
+ auto [RecursiveIt, IsNew] = RecursiveFn.insert(std::make_pair(D, true));
if (!IsNew)
- return true;
+ return RecursiveIt->second;
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);
+ if (!Result) {
+ // D and its mutually recursive callers are all non-trivial.
+ for (auto& It : RecursiveFn)
+ It.second = false;
+ }
+ assert(RecursiveFn[D] == Result);
RecursiveFn.erase(D);
+ Cache[D] = Result;
return Result;
}
@@ -518,7 +521,7 @@ class TrivialFunctionAnalysisVisitor
private:
CacheTy &Cache;
- llvm::DenseSet<llvm::PointerUnion<const Decl *>> RecursiveFn;
+ CacheTy RecursiveFn;
};
bool TrivialFunctionAnalysis::isTrivialImpl(
>From 6d46b0b2519a812be0bec190ce4f231104781449 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at apple.com>
Date: Sun, 12 May 2024 23:48:25 -0700
Subject: [PATCH 6/6] Fix the cache updating logic.
---
.../StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 774390ce400fc..b690afb70863d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -288,11 +288,13 @@ class TrivialFunctionAnalysisVisitor
bool Result = Visit(Body);
if (!Result) {
// D and its mutually recursive callers are all non-trivial.
- for (auto& It : RecursiveFn)
+ for (auto &It : RecursiveFn)
It.second = false;
}
- assert(RecursiveFn[D] == Result);
- RecursiveFn.erase(D);
+ RecursiveIt = RecursiveFn.find(D);
+ assert(RecursiveIt != RecursiveFn.end());
+ Result = RecursiveIt->second;
+ RecursiveFn.erase(RecursiveIt);
Cache[D] = Result;
return Result;
More information about the cfe-commits
mailing list