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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
