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

[Anna Zaks via cfe-commits]

zaks.anna added inline comments.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1616
@@ +1615,3 @@
+        builder.isFeasible(false) && !StFalse && 
+        BldCtx.blockCount() == AMgr.options.maxBlockVisitOnPath - 1) {
+
----------------
seaneveson wrote:
> 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?
This would increment the value for all loops...

I think we should first answer the question below. Do we want to widen all loops, not just loops with "constant bounds"?

================
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}}
----------------
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. 


http://reviews.llvm.org/D12358