This is an archive of the discontinued LLVM Phabricator instance.

AST: Mangle reference temporaries reliably
ClosedPublic

Authored by majnemer on Apr 29 2014, 5:26 PM.

Details

Summary

Previously, we would generate a single name for all reference
temporaries and allow LLVM to rename them for us. Instead, number the
reference temporaries as we build them in Sema.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 8947.Apr 29 2014, 5:26 PM
majnemer retitled this revision from to AST: Mangle reference temporaries reliably.
majnemer updated this object.
majnemer added a reviewer: rsmith.
majnemer added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Apr 29 2014, 5:58 PM

Please also propose this mangling scheme on cxx-abi-dev.

lib/AST/ExprCXX.cpp
1458–1459 ↗(On Diff #8947)
auto ES = new (ExtendedBy->getASTContext()) ExtraState;
1464–1465 ↗(On Diff #8947)

Maybe stash the ExtraState* in a variable to avoid repeateding geting it here?

lib/Sema/SemaInit.cpp
5769 ↗(On Diff #8947)

It's not sufficient to start the numbering from 1 each time: we need to number within the topmost InitializedEntity, not just within the subentity for which we're currently building an initializer. Example:

struct S { const int &a, const int &b; };
S s = { 1, 2 };
lib/Serialization/ASTWriterStmt.cpp
1569–1573 ↗(On Diff #8947)

We usually serialize fields in the order they appear in the class (so mangling number would go last). Not a big deal, though.

majnemer updated this revision to Diff 8951.Apr 29 2014, 9:24 PM
majnemer edited edge metadata.

Address Richard's review comments

rsmith added inline comments.Apr 30 2014, 7:02 PM
include/clang/Sema/Initialization.h
424 ↗(On Diff #8951)

Maybe make this allocate a mangling number and return it, rather than returning a reference to the member?

lib/Sema/SemaInit.cpp
5381 ↗(On Diff #8951)

Maybe pass the ExtendingEntity * into here instead of its decl and a reference to its mangling number?

5791–5808 ↗(On Diff #8951)

You could reorder these to first create the MTE and then performReferenceExtension on it, and avoid the special-casing of the ManglingNumber here:

auto *MTE = new (S.Context) MaterializeTemporaryExpr(... /*no extending decl or mangling number*/ ...);
if (auto *ExtendingEntity = getEntityForTemporaryLifetimeExtension(&Entity)) {
  performReferenceExtension(MTE, ExtendingEntity);
  warnOnLifetimeExtension(...);
}
6196–6218 ↗(On Diff #8951)

Likewise here.

majnemer updated this revision to Diff 9004.Apr 30 2014, 8:33 PM

Address Richard's review comments.

rsmith accepted this revision.May 1 2014, 1:45 AM
rsmith edited edge metadata.

Looks great, thanks!

include/clang/Sema/Initialization.h
424 ↗(On Diff #9004)

Please give this a name that sounds more like it's changing the state of the object. allocateManglingNumber or something?

lib/AST/MicrosoftMangle.cpp
120 ↗(On Diff #9004)

This line looks like it's over 80 characters to me.

This revision is now accepted and ready to land.May 1 2014, 1:45 AM
majnemer closed this revision.May 19 2014, 7:23 AM
majnemer updated this revision to Diff 9551.

Closed by commit rL207776 (authored by @majnemer).