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