[analyzer] Allow recursive functions to be trivial.

[rniwa]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/91876.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+9-9)
• (modified) clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp (+6)

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@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+
     getFieldTrivial().someFunction();
     // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
     getFieldTrivial().nonTrivial1();

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[MitalAshok]

You should add a test for mutually recursive functions. I suspect something like this doesn't work:

    int non_trivial();
    int f(bool b) { return g(!b) + non_trivial(); }
    int g(bool b) { return b ? f(b) : 1; }
    
getFieldTrivial().f(true);  // expected-warning {{...}}
getFieldTrivial().g(true);  // expected-warning {{...}}

Since when analyzing f, f enters the cache as trivial, g is analyzed and sees f in the cache as trivial, so g is marked trivial, but then f is marked as non-trivial (so g should have been marked as non-trivial, but is marked trivial).

If that is an issue, one way to do this "properly" would be to create a graph, and a function is non-trivial if it has a path to a non-trivial function (this might be too expensive to implement directly). Or we could special case simple recursion only, by changing TrivialFunctionAnalysisVisitor to not call isTrivialImpl for the current function only.

[rniwa]

You should add a test for mutually recursive functions. I suspect something like this doesn't work:

    int non_trivial();
    int f(bool b) { return g(!b) + non_trivial(); }
    int g(bool b) { return b ? f(b) : 1; }
    
getFieldTrivial().f(true);  // expected-warning {{...}}
getFieldTrivial().g(true);  // expected-warning {{...}}

Duh, of course. Thanks for pointing out the obvious flaw.

[haoNoQ]

If that is an issue, one way to do this "properly" would be to create a graph

Note that we do have a CallGraph class. The static analyzer uses it to identify top-level entry points. It doesn't look like it has a readily available solution for identifying recursive functions. But it may be a good idea to simply use the class and implement it there.

[haoNoQ]

Ok LGTM then!

[rniwa]

Thanks for the review!