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

[Sean Eveson via cfe-commits]

seaneveson added a comment.

Thank you for your comments.

@zaks.anna wrote:

> What do you mean by "approximate"?


I think @dcoughlin gave a perfect example in the following comment:

@dcoughlin wrote:

> This doesn't seem quite right. Consider:
>
>   int i;
>   for (i = 0; i < 21; i += 3) {}
>   clang_analyzer_eval(i == 23);
>
>
> The value of i should be 21 after the loop, but this code sets it to 23. And what happens if i starts at 1 instead of 0?


The patch only examines the loop condition to avoid having to consider any of the loop body. It approximates an exit value for the variable in the condition by evaluating an iteration with the variable at its maximum/minimum value. This is loosely based on the widening and narrowing described in this paper: http://dl.acm.org/citation.cfm?id=512973

@zaks.anna wrote:

> Note that people rely on the analyzer to track the exact values of variables. They expect it to know what the value is. It can be very confusing if the analyzer states that it knows the value of the variable and it is the wrong value.
>
> I am concerned with changing the value of a single variable of the loop based on a syntactic check. Also, I am not sure there is a strong need for preserving the value of the index variable either.


Since the goal of this patch is to get the analyzer to move past loops, I see no problem with invalidating the index variable. That said, I still think it is worth doing the approximation, as it identifies infinite loops.

Further work could then be done to set the value of the index variable after 'simple' loops, where the analyzer can be sure of the value. In addition to this, the detection of infinite loops could be improved to work for any loop. This would allow the widening to be applied to any non-infinite, non-exiting loop.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1616
@@ +1615,3 @@
+        builder.isFeasible(false) && !StFalse && 
+        BldCtx.blockCount() == AMgr.options.maxBlockVisitOnPath - 1) {
+
----------------
zaks.anna wrote:
> Do we loose precision for loops that need to be executed exactly maxBlockVisitOnPath times?
Yes.

To be precise, the loss is for loops that need to be executed (maxBlockVisitOnPath - 1) times, because processCFGBlockEntrance generates a sink if blockCount >= maxBlockVisitOnPath.

With the default value for maxBlockVisitOnPath, a loop that iterated three times would be fully analyzed with widen=false, but would be unnecessarily widened if widen=true.

This could be fixed by (effectively) incrementing the value of maxBlockVisitOnPath, when the widening was enabled.

e.g. replacing the following line from processCFGBlockEntrance:

```
if (nodeBuilder.getContext().blockCount() >= AMgr.options.maxBlockVisitOnPath) {
...
```
With something like:

```
int blockLimit = AMgr.options.maxBlockVisitOnPath + 
                 (AMgr.options.shouldWidenConstantBoundLoops() ? 1 : 0);
if (nodeBuilder.getContext().blockCount() >= blockLimit) {
...
```

Does that seem resonable?

================
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:
> 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?


http://reviews.llvm.org/D12358