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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

nit: Please, use proper punctuation in the comments.


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

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

================
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:171
@@ +170,3 @@
+    
+    // Set the approximate value of the loop variable in the last itteration
+    Loc LVLoc = State->getLValue(LoopVariable, LCtx);
----------------
iteration -> iteration

What do you mean by "approximate"? 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.

================
Comment at: test/Analysis/constant-bound-loops.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-store=region -analyzer-max-loop 4 -analyzer-config widen-constant-bound-loops=true -verify %s
+
----------------
Should we test that the loop is still executed n times before it is widened?

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


http://reviews.llvm.org/D12358