[PATCH] D12358: [Analyzer] Handling constant bound loops

[Sean Eveson via cfe-commits]

seaneveson added a comment.

My initial approach was for the analyzer to have as much information as possible after the loop. This means there are cases where the information is incorrect. Future work would be to reduce these cases.

I believe your preferred approach is to have no inaccuracies after the loop, which can initially be achieved by providing less information. Further work would add more (guaranteed accurate) information. In this way the analyzer is naturally conservative when it isn't sure of something. I now agree that this is a better approach to take.

What do people think about me creating a new patch based on your feedback? The aim would be to widen any non-exiting loops by invalidation. The initial patch would be behind a flag and would use the TK_EntireMemSpace trait to conservatively invalidate 'everything' when a loop does not exit. It would then run through the loop body in the invalidated state, resulting in the analysis of all possible paths. The loop would then exit from the (now possibly false) loop condition, or a (now reachable) break statement. Loops that never exit regardless of any variables would be an exception; see my comment below for thoughts on handling infinite loops.

Future improvements would prevent unnecessary invalidation and calculate the values of modified variables (after the loop).


================
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:10
@@ +9,3 @@
+///
+/// This file contains functions which are used to widen constant bound loops.
+/// A loop may be widened to approximate the exit state(s), without analysing
----------------
zaks.anna wrote:
> This would allow to widen not just the constant bound loops!
> I think we should widen any loop that we know we did not fully execute.
I agree that the goal should be to widen any loop that isn't fully executed, but we need to consider infinite loops, which might be falsely exited after widening. The way I see it, assuming the widening will be done by invalidating variables/memory, we could either:

  # Only widen loops which definitely exit.
  # Widen all loops unless they are definitely infinite (or widen them in a way which minimizes the number of infinite loops which then exit).
  # Come up with a method which decides if a loops is infinite or not (but tolerate mistakes either way) and widen when we "think" the loop will exit. This is similar to the current approach for constant bound loops.

My current preference would be option 2. This is based on the assumption that loops are generally written to exit at some point, and if they aren't, they are unlikely to have code after them anyway. When it does not know which branch the program will go down, the analyzer's approach is to go down both. Similarly if the analyzer does not know whether the loop exits, it should optimistically go down the exit branch (IMO).

================
Comment at: test/Analysis/constant-bound-loops.c:174
@@ +173,3 @@
+    
+    clang_analyzer_eval(g_global); // expected-warning {{UNKNOWN}}
+    clang_analyzer_eval(s_arg); // expected-warning {{UNKNOWN}}
----------------
zaks.anna wrote:
> seaneveson wrote:
> > zaks.anna wrote:
> > > I think we should extend this test case.
> > > Ex: what about heap, what about variables touched by the loop variables declared in the loop's scope?
> > Good point. I actually encountered a false positive while improving this case.
> > 
> > 
> > ```
> > int *h_ptr = malloc(sizeof(int));
> > *h_ptr = 3;
> > for (int i = 0; i < 10; ++i) {} // WARNING: Potential leak of memory pointed to by 'h_ptr'
> > ```
> > 
> > The value of h_ptr is invalidated, but I'm not sure about *h_ptr. Is it likely this warning is produced because the pointer is invalidated, but not the memory?
> Could you provide the full example? There is leak in the code segment above. 
Sorry, I meant to say the warning is there regardless of what comes after the loop. Here is a full test case:
```
typedef __typeof(sizeof(int)) size_t;
void *malloc(size_t);
void free(void *);

void test() {
    int *h_ptr = (int *) malloc(sizeof(int));
    for (int i = 0; i < 2; ++i) {} // No warning.
    for (int i = 0; i < 10; ++i) {}
    free(h_ptr);
}
```
It produces the following for me:
```
clang.exe -cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-config widen-constant-bound-loops=true test.cpp
test.cpp:8:31: warning: Potential leak of memory pointed to by 'h_ptr'
    for (int i = 0; i < 10; ++i) {}
                              ^
1 warning generated.
```


http://reviews.llvm.org/D12358