[PATCH] WIP Fix for temporary destructors in conditionals

[Pavel Labath]

  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