[PATCH] D26589: Add static analyzer checker for finding infinite recursion

[Artem Dergachev via cfe-commits]

NoQ added a comment.

Thank you for working on this! The overall approach is good. Because alpha checkers are disabled by default, false positives can be addressed later in subsequent commits.



================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:2
+// InfiniteRecursionChecker.cpp - Test if function is infinitely
+// recursive--*--//
+//
----------------
Accidental line break :o


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:21
+
+REGISTER_SET_WITH_PROGRAMSTATE(DirtyStackFrames, const clang::StackFrameContext *)
+
----------------
k-wisniewski wrote:
> a.sidorin wrote:
> > The idea of "dirty stack frames" deserves some explanation. As I understand, it describes function calls where some values may change unexpectedly and we cannot consider this calls later. Am I understanding correctly?
> Yes! Right now it considers frame as "dirty" when there was any kind of region change in this frame or in any function that was called from it. As you see below, it then stops the search down the stack once it encounter such a "dirty" frame, because it can no longer be sure that conditions upon which the recursive call depends on did not change in an unpredictable way. It's obviously too broad a definition and in the next iterations I'll try to narrow it down to variables that affect whether the recursive call happens or not. I also consider looking at //the way // it changes to determine if it's meaningful or not.
I think this deserves commenting, at least the very mechanism of how a stack frame is considered dirty (if it is present in the set, or if a parent is present in the set, or something else, and why is the chosen method somehow correct).


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:145
+    BT.reset(
+        new BugType(this, "Infinite recursion detected", "RecursionChecker"));
+
----------------
I think one of the builtin bugtypes should be used, such as `LogicError`. In any case, both of these should be human-friendly (they appear eg. in scan-view).


================
Comment at: test/Analysis/recursion.cpp:27
+
+void f2();
+void f3();
----------------
k-wisniewski wrote:
> a.sidorin wrote:
> > I'd like to see some more informative function names: for me, it is hard to distinguish between f1-7 here :) It is also hard to determine the function where test starts.
> The convention I know is that the bottom-most one gets considered first, but I may add some explicitly marked entry points for the sake of consistency and to make it easier to reason about. 
There's a way to hard-check this through `FileCheck` + `-analyzer-display-progress` (see `inlining/analysis-order.c`). Might be an overkill, but on the other hand may actually improve readability of the test quite a bit.


================
Comment at: test/Analysis/recursion.cpp:37
+void f2() {
+    SampleGlobalVariable = 1;
+    f3();
----------------
This should eventually warn. Even though the variable is accessed, it's not really changed :)

I'd suggest something like:
```
++SampleGlobalVariable;
if (SampleGlobalVariable < 100)
  f3();
```
It'd make sure that the test is "correct" (it's obvious to the reader that this is a false positive we'd never want to warn about). But you could also keep the original test with a FIXME remark ("we should eventually warn about this").

I also think that this group of tests deserves to have the following test:
```
bool bar(); // no definition!
void foo() {
  if (bar()) { // may change the global state
    foo(); // no-warning
  }
}
```
This test case should magically work because calling `bar()` will change the "global memory space" region (invalidate all globals).


https://reviews.llvm.org/D26589