This is an archive of the discontinued LLVM Phabricator instance.

An attempt at fixing lifetime extended temporaries in the CFG.
Needs ReviewPublic

Authored by klimek on Jul 29 2014, 8:55 AM.

Details

Reviewers
jordan_rose
Summary

Fix handling of lifetime extended temporaries by using the
MaterializeTemporaryExpr as indication of whether a temporary was actually
extended. As lifetime extended temporaries do not necessarily have VarDecls,
changes the type of the destructors for those to reuse CFGTemopraryDtor.

Open problems:

  • temporary dtors are not yet inlined, but some tests rely on dtor inlining for lifetime extended temporaries
  • there are still missing cases for when we do not want to continue digging through the AST for the CXXBindTemporaryExpr after hitting a MaterializeTemporaryExpr; we now have 3 different places in the code that encodes this information; some refactoring is needed to clean that up
  • more tests needed

This is still rough, but I wanted to get early feedback about the direction this
is taking.

Diff Detail

Event Timeline

klimek updated this revision to Diff 11983.Jul 29 2014, 8:55 AM
klimek retitled this revision from to An attempt at fixing lifetime extended temporaries in the CFG..
klimek updated this object.
klimek edited the test plan for this revision. (Show Details)
klimek added a reviewer: jordan_rose.
klimek added subscribers: alexmc, Unknown Object (MLST).

Thanks again for smashing on this, Manuel. This rabbit hole just keeps on getting deeper.

I'm in my usual position about not knowing enough about clang to know whether this makes sense at a high-level. Consequently, the following comments might not make sense or might miss problems with the overall approach, so please take them with a grain of salt.

lib/Analysis/CFG.cpp
439

This signature's a bit weird to me: this method is called for lifetime extended temporaries, but LifetimeExtended can be false: what does that mean?

Does this method or its arguments need to be renamed?

1052

Rather than const cast here, should we add or remove const from some other signature so this isn't needed?

1069–1070

Maybe add a comment explaining that we don't need to anything for a bind statement for a lifetime extended temporary because there's no object creation going on?

1119

if (noReturn) { createNoReturnBlock(); } else { autoCreateBlock(); }
This seems to be a common pattern. Maybe worth adding a helper to make this code easier to read?

1262

Maybe add a helper here for calls like this that don't care about CommaLHSs or Adjustments to make this easier to read

klimek updated this revision to Diff 11986.Jul 29 2014, 9:26 AM

Add comment.

lib/Analysis/CFG.cpp
439

Yes, as I wrote, this is still pretty rough - I expect some major refactoring to happen before it lands.

1052

Unfortunately the sinks are non-const, which basically kills us.

1069–1070

Added a comment (object creation is going on, but we handle the descturctor elsewhere).

1119

I actually think this is probably not quite right here yet (I have failing tests with noreturn dtor cases), so I'll do that once all that has settled down :)

1262

I'd actually like to not need to visit this 3 times. Alas, I'm not sure it's possible. You're right, if it's not possible, we should pull something out...

jordan_rose edited edge metadata.Jul 29 2014, 8:07 PM

This mostly seems reasonable to me, except I wonder if we'll want to encode the path to the temporaries by something other than the expression where they were created. I can't remember when they're proper regions and when they're just values (LazyCompoundVals) in the analyzer, but we should be careful to be consistent about the region in which the constructor is run and the region in which the destructor is run.

klimek updated this revision to Diff 12063.Jul 31 2014, 6:55 AM
klimek edited edge metadata.

Adapt now working cfg tests.
Handle more cases for lifetime extension.
Fix bug in materialize-temporary handling.