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

[Aleksei Sidorin via cfe-commits]

a.sidorin added a comment.

Wow, thank you for such a contribution! Did you evaluate this checker on some real code?

I have some minor inline comments below.



================
Comment at: lib/StaticAnalyzer/Checkers/CMakeLists.txt:41
   IdenticalExprChecker.cpp
+        RecursionChecker.cpp
   IvarInvalidationChecker.cpp
----------------
Please fix indentation here


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:12
+// This defines TestAfterDivZeroChecker, a builtin check that performs checks
+//  for division by zero where the division occurs before comparison with zero.
+//
----------------
This description is for other checker, could you update it?


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:21
+
+REGISTER_SET_WITH_PROGRAMSTATE(DirtyStackFrames, const clang::StackFrameContext *)
+
----------------
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?


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:49
+  void checkPostCall(const CallEvent &Call,
+                                       CheckerContext &C) const;
+};
----------------
Incorrect indentation.


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:68
+
+    if (ParentLC->getKind() != LocationContext::StackFrame)
+      continue;
----------------
We may just use `getParent()->getCurrentStackFrame()` in the loop increment to make the code cleaner. What do you think?


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:79
+    const FunctionDecl *PrevFuncDecl =
+        (const FunctionDecl *)PrevStackFrameCtx->getDecl();
+    PrevFuncDecl = PrevFuncDecl->getCanonicalDecl();
----------------
We usually prefer LLVM `[dyn_]cast<>` where possible.


================
Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:89
+      SVal PrevArg = State->getArgSVal(PrevStackFrameCtx, i);
+      SameArgs = SameArgs && compareArgs(C, State, CurArg, PrevArg);
+    }
----------------
Maybe we can use early return here?


================
Comment at: test/Analysis/recursion.cpp:27
+
+void f2();
+void f3();
----------------
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.


https://reviews.llvm.org/D26589