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

[Krzysztof Wiśniewski via cfe-commits]

k-wisniewski added a comment.

@a.sidorin 
Thank you for your review! I'll upload the new patch this evening that will include fixes for the things you pointed out. Can I also add you as a reviewer? Also: Can you have a look at my other patch that I have linked in the description? Thanks in advance!



================
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.
+//
----------------
a.sidorin wrote:
> This description is for other checker, could you update it?
Sure, it was easier for me to use some other checker as a template and I forgot about it. Should I add you as a reviewer once I upload the updated patch?


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


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


https://reviews.llvm.org/D26589