[PATCH] WIP Fix for temporary destructors in conditionals

[Pavel Labath]

On 6 August 2013 02:31, Jordan Rose <jordan_rose at apple.com> wrote:

>
>   I downloaded this patch and rebuilt, and this already doesn't pass the
> destructors::testConsistency test in temporaries.cpp. That might match your
> "I don't think the patch is complete yet".
>
>   More importantly, however, your test case doesn't actually trigger the
> environment cleanup. Try adding a trivial inlined function ("return true")
> and I think you'll see there's a problem. Liveness isn't cumulative as you
> go backwards; it's more like a flashlight, where a small set of live things
> are true. We're not currently taking so much advantage of this, but it's
> convenient to know the difference between an expression that //happens//
> not to have a value and one that //should// not have a value.
>
>   ```if (isa<Expr>(S)) {
>     val.liveStmts = LV.SSetFact.remove(val.liveStmts, S);
>   }```
>
>   I'd be happy if your conjecture does turn out to be true (and admit that
> LiveVariables was written a long time ago and so my mental model is rusty),
> but I think it's going to turn out to be more work than that.
>
> http://llvm-reviews.chandlerc.com/D1259
>

Ok, the destructor problem solved. It wasn't that hard, I just panicked. :)
(But the question still remains whether to run the array destructor before,
or after the elements'. Or it does not matter.)

Now, back to the liveness issue. I re-ran the analyzer with debug output on
the following program:
struct A {
  A();
  ~A();
  operator bool() const;
};

bool u();

bool f() { return false; }

void ff() {
  if (((f(), u()) || (f(), u())) && (f(), A())) {
  } else {
  }
}

And I still get the same thing:
----- Position: 4.0
Store (direct and default bindings), 0x43c5508 :
 (GlobalInternalSpaceRegion,0,default) : conj_$3{int}

 (GlobalSystemSpaceRegion,0,default) : conj_$4{int}



Expressions:
 (0x43b7b10,0x43ac188) f() : 0 S8b
 (0x43b7b10,0x43ac230) u : &code{u}
 (0x43b7b10,0x43ac248) u() : conj_$5{_Bool}
 (0x43b7b10,0x43ac270) f() , u() : conj_$5{_Bool}
 (0x43b7b10,0x43ac2b8) (f() , u()) || (f() , u()) : 0 S8b
Ranges of symbol values:
 conj_$5{_Bool} : { [0, 0] }
Expression 0x43ac2b8 is live.
Attempting to use saved value of 0x43ac2b8...
Saving condition value of 0x43ac738 (0)
Attempting to use saved value of 0x43ac760...


And I am pretty sure I am triggering the environment cleanup, because the
"Expression ... is live." line is actually output from within the
removeDeadBindings() function:
--- a/lib/StaticAnalyzer/Core/Environment.cpp
+++ b/lib/StaticAnalyzer/Core/Environment.cpp
@@ -166,6 +166,7 @@ EnvironmentManager::removeDeadBindings(Environment Env,
     const SVal &X = I.getData();

     if (SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext()))
{
+      llvm::errs() << "Expression " << BlkExpr.getStmt() << " is live.\n";
       // Copy the binding to the new map.
       EBMapRef = EBMapRef.add(BlkExpr, X);


The code you quoted above does not contradict any of this. It is
responsible for removing the expression from the live set once we encounter
that expression in the cfg. Because the liveness analysis runs backwards,
the expression, once marked as live will stay live until encountering the
said expression in the cfg. Putting that in the normal direction, it will
be live from the moment it is produced, until we reach the last consumer of
the expression.

cheers,
pl
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130806/723ef710/attachment.html>