This is an archive of the discontinued LLVM Phabricator instance.

[CFG] [analyzer] Explain copy elision through construction contexts.
ClosedPublic

Authored by NoQ on May 31 2018, 7:00 PM.

Details

Summary

Copy elision occurs when

  1. a temporary object (with its corresponding temporary object construction context) is constructed and
  2. it is immediately copied or moved into any other object (with its own construction context, which can be any construction context).

Constructor on step 2 would then be called "elidable".

Copy elision consists in skipping ("eliding") the constructor on step 2 and performing the constructor on step 1 as if it was constructing the object of step 2. Additionally, destructors are skipped for the temporary object constructed on step 1.

The current patch modifies the temporary object construction context for constructor on step 1 to include information about the upcoming step 2. Only temporary object construction contexts would need to be updated because step 1 always has a temporary object construction context.

The updated context will include both a reference to the elidable construct-expression of step 2 and the construction context of step 2. This will allow the analyzer to both skip the elidable constructor itself when it's encountered and compute the correct target region associated with step 2 while modeling step 1.

TOTHINK: whether "temporary object construction context with elision" should be a separate construction context sub-class.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.May 31 2018, 7:00 PM
george.karpenkov requested changes to this revision.Jun 1 2018, 10:59 AM

There's quite a lot of code duplication here, I think we could do better with that. Great job modeling semantics though!

lib/Analysis/CFG.cpp
1320 ↗(On Diff #149389)

There are three repeated calls to findConstructionContexts, with only the second argument changing. Perhaps a helper lambda closure would help?

4959 ↗(On Diff #149389)

begs for a comment differentiating CC from CC1

5014 ↗(On Diff #149389)

three blocks below could really benefit from a helper function.

lib/Analysis/ConstructionContext.cpp
140 ↗(On Diff #149389)

seems this chunk has a fair bit of code duplication vs. lines 79-99

This revision now requires changes to proceed.Jun 1 2018, 10:59 AM
NoQ updated this revision to Diff 149566.EditedJun 1 2018, 3:05 PM

Fix comments in the callback test. The test itself is pretty pointless now, because the behavior that it was supposed to test was a workaround for lack of lifetime extension support.

NoQ added a comment.Jun 1 2018, 3:05 PM

Whoops wrong patch.

NoQ updated this revision to Diff 149568.Jun 1 2018, 3:09 PM
NoQ marked 2 inline comments as done.

Actually let's do a separate context kind. The difference in semantics they represent is so drastically different that i think it's worth it.

lib/Analysis/CFG.cpp
1320 ↗(On Diff #149389)

I'll look into this later for the whole function.

4959 ↗(On Diff #149389)

This variable was in fact unused.

NoQ updated this revision to Diff 149571.Jun 1 2018, 3:24 PM

Actually let's use lambdas as well.

lib/Analysis/CFG.cpp
1320 ↗(On Diff #149389)

Aha, we are changing the whole function anyway, ok then.

MTC added a subscriber: MTC.Jun 3 2018, 11:40 PM
MTC added inline comments.
include/clang/Analysis/ConstructionContext.h
351 ↗(On Diff #149571)

explicit useless?

377 ↗(On Diff #149571)

Same useless here?

MTC added inline comments.Jun 12 2018, 12:00 AM
include/clang/Analysis/ConstructionContext.h
351 ↗(On Diff #149571)

I'm wrong here, maybe there is some use of SimpleTemporaryObjectConstructionContext({ , }), sorry to bother!

377 ↗(On Diff #149571)

Also I'm wrong here!

NoQ updated this revision to Diff 151277.Jun 13 2018, 5:44 PM

Add a flag to disable copy elision. It's convenient to have such flag in the CFG because this way all clients will be able to transparently handle it. When the option is turned off, elided construction contexts will be replaced with simple temporary object construction contexts which need to be handled anyway. When construction contexts are disabled entirely, the option has no effect.

include/clang/Analysis/ConstructionContext.h
351 ↗(On Diff #149571)

Well, i really doubt it'd ever matter (given that the constructor is also private, we can pretty much control all constructions by hand) but i think it's a good practice to add the restriction unless implicit construction is actually important.

This revision is now accepted and ready to land.Jun 26 2018, 5:22 PM
This revision was automatically updated to reflect the committed changes.