Yet, it does work. And I don't think it's "by chance". The reason that adding
the values helps, is because I am not *re*adding the values to the environment.
The problem is that values were never put there in the first place. The current
code for processBranch does not need them, because it uses some magic to find
the correct atomic sub-expression (ResolveCondition()) that is used to decide
the value of the condition at this point. This, of course, fails when we run
through the conditions the second time, because it cannot find the right
"clues" in the previous elements of the CFGBlock.
That's why I thought the cleanest solution would be to start storing the
values of the conditional expressions explicitly. It also turned out to be the
easiest solution, since the liveness analysis already supports this quite well.
The proof that this works is in the way the analysis works: it goes through the
CFG backwards, and when it sees that a subexpression is needed as an input, the
subexpression is marked as live. Then the expression stays live until we
encounter it as a full expression in the CFG. Here, we mark it as dead (and
it's sub-expressions again as live, etc.). In this setting it does not matter
that the expression will be marked as "live" twice, it will just be live
longer, which is exactly what we want. The only potential problem with this is
that the liveness algorithm uses Stmt* as an identifier of the position in the
CFG, which is no longer unique. A better choice would be something like the
(CFGBlock *, position-in-the-block) pair, and I think that in!
the long
run it would be a good idea to switch to something like this. If you feel
that is necessary, I am ready to do that. However, for now it should work like
this, because even though the live set will be computed twice, it should be the
same, as there are only destructor invocations between the two computations.
That said, I don't think this patch is complete yet. The problem with it is
that it actually doesn't add enough values to the environment. E.g., in the
expression ((A && B) && C) || D, if B is false then I it's not sufficient to
bind (A && B) to 0 (which I do already), I also need to mark ((A && B) && C) as
false, because this is what the destructor test will be checking. I will start
looking into how to do this now.
PS: as a practical proof that the symbol reaper recognizes the conditional
subexpressions as live, I have rigged the analyzer with some debugging output,
and this is what it produced when run on the following program:
$ cat ~/tmp/a.cc
struct A {
A();
~A();
operator bool() const;
};
bool u();
void f() {
if ((u() || u()) && A()) {
} else {
}
}
$ bin/clang --analyze ~/tmp/a.cc
----- Position: 7.0
Store (direct and default bindings), 0x0 :
Ranges are empty.
----- Position: 7.2
Store (direct and default bindings), 0x0 :
Expressions:
(0x6536e20,0x652b1a8) u : &code{u}
(0x6536e20,0x652b1f8) u : &code{u}
Ranges are empty.
Expression 0x652b1f8 is live.
Attempting to use saved value of 0x652b210...
Saving condition value of 0x652b2f8 (1)
----- Position: 6.0
Store (direct and default bindings), 0x6537d58 :
(GlobalInternalSpaceRegion,0,default) : conj_$0{int}
(GlobalSystemSpaceRegion,0,default) : conj_$1{int}
Expressions:
(0x6536e20,0x652b1f8) u : &code{u}
(0x6536e20,0x652b210) u() : conj_$2{_Bool}
Ranges of symbol values:
conj_$2{_Bool} : { [0, 0] }
----- Position: 6.2
Store (direct and default bindings), 0x6537d58 :
(GlobalInternalSpaceRegion,0,default) : conj_$0{int}
(GlobalSystemSpaceRegion,0,default) : conj_$1{int}
Expressions:
(0x6536e20,0x652b290) u : &code{u}
(0x6536e20,0x652b2b8) u : &code{u}
Ranges are empty.
Expression 0x652b2b8 is live.
Attempting to use saved value of 0x652b2f8... failed
Saving condition value of 0x652b2f8 (1)
Saving condition value of 0x652b2f8 (0)
----- Position: 4.0
Store (direct and default bindings), 0x653eb50 :
(GlobalInternalSpaceRegion,0,default) : conj_$3{int}
(GlobalSystemSpaceRegion,0,default) : conj_$4{int}
Expressions:
(0x6536e20,0x652b2b8) u : &code{u}
(0x6536e20,0x652b2d0) u() : conj_$5{_Bool}
(0x6536e20,0x652b2f8) u() || u() : 0 S8b
Ranges of symbol values:
conj_$5{_Bool} : { [0, 0] }
Expression 0x652b2f8 is live.
Attempting to use saved value of 0x652b2f8...
Saving condition value of 0x652b670 (0)
Attempting to use saved value of 0x652b698...
----- Position: 0.0
Store (direct and default bindings), 0x653eb50 :
(GlobalInternalSpaceRegion,0,default) : conj_$3{int}
(GlobalSystemSpaceRegion,0,default) : conj_$4{int}
Expressions:
(0x6536e20,0x652b2f8) u() || u() : 0 S8b
(0x6536e20,0x652b670) (u() || u()) && struct A().operator _Bool() : 0 S8b
Ranges are empty.
----- Position: 5.0
Store (direct and default bindings), 0x653eb50 :
(GlobalInternalSpaceRegion,0,default) : conj_$3{int}
(GlobalSystemSpaceRegion,0,default) : conj_$4{int}
Expressions:
(0x6536e20,0x652b2b8) u : &code{u}
(0x6536e20,0x652b2d0) u() : conj_$5{_Bool}
(0x6536e20,0x652b2f8) u() || u() : 1 S8b
Ranges of symbol values:
conj_$5{_Bool} : { [1, 255] }
Expression 0x652b2f8 is live.
----- Position: 5.4
Store (direct and default bindings), 0x653fed0 :
(GlobalInternalSpaceRegion,0,default) : conj_$7{int}
(GlobalSystemSpaceRegion,0,default) : conj_$8{int}
(temp_object{struct A,0x652b520},0,default) : conj_$6{int}
Expressions:
(0x6536e20,0x652b2f8) u() || u() : 1 S8b
(0x6536e20,0x652b600) struct A().operator _Bool : &code{operator _Bool}
Ranges are empty.
Expression 0x652b2f8 is live.
Expression 0x652b600 is live.
----- Position: 4.0
Store (direct and default bindings), 0x653fd10 :
(GlobalInternalSpaceRegion,0,default) : conj_$9{int}
(GlobalSystemSpaceRegion,0,default) : conj_$10{int}
Expressions:
(0x6536e20,0x652b2f8) u() || u() : 1 S8b
(0x6536e20,0x652b600) struct A().operator _Bool : &code{operator _Bool}
(0x6536e20,0x652b630) struct A().operator _Bool() : conj_$11{_Bool}
(0x6536e20,0x652b658) struct A().operator _Bool() : conj_$11{_Bool}
Ranges are empty.
Expression 0x652b2f8 is live.
Expression 0x652b658 is live.
Attempting to use saved value of 0x652b2f8...
Attempting to use saved value of 0x652b698...
Block 4 is the one where we decide whether we want to run the destructor of
A. As you can see, the value we bound earlier to 0x652b2f8, has survived this
far, and is still considered live, which is why we succeed in using it two
lines below.
Sorry for such a long essay, but I wanted to make it clear about what I think
about this problem. (I also found that it helps me to clear some things up
internally :) ).
http://llvm-reviews.chandlerc.com/D1259
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
