Page MenuHomePhabricator

Fix asserts about emitting constexpr methods twice during dllexport explicit instantiation (PR21718)

Authored by hans on Dec 15 2014, 7:09 PM.



During explicit instantiation of dllexport class templates, we would end up trying to emit constexpr methods (at least) twice:

  1. MarkFunctionReferenced would cause it to be emitted, and
  2. InstantiateClassMembers would then emit it again

(3. if we had gotten this far, we'd then call HandleTopLevelDecl on it, which would be the third emission attempt)

Diff Detail


Event Timeline

hans updated this revision to Diff 17318.Dec 15 2014, 7:09 PM
hans retitled this revision from to Fix asserts about emitting constexpr methods twice during dllexport explicit instantiation (PR21718).
hans updated this object.
hans edited the test plan for this revision. (Show Details)
hans added reviewers: rsmith, majnemer.
hans added subscribers: hansw, Unknown Object (MLST).
hans added a subscriber: ehsan.

+ehsan fyi

rsmith edited edge metadata.Dec 17 2014, 11:10 AM

After some investigation, it seems that the issue is a more fundamental problem in the ASTConsumer interface; there are various cases where we call HandleTopLevelDecl multiple times on the same declaration. We get away with this if DeclMustBeEmitted is false for the declaration, but not otherwise. Here's another testcase not involving constexpr nor dllexport:

template<typename T> struct S { __attribute__((used)) auto f() { return 0; } };
int k = S<int>().f();
template struct S<int>;

We also observed that we're missing HandleTopLevelDecl (or similar) calls when implicitly defining special member functions (which also acts to mask the issue in a lot of cases). This causes seemingly-wrong code generation for cases like:

struct S { __attribute__((used)) S() = default; } s;

GCC generates and retains a definition of S::S() in this case.

One possible fix:

  • stop calling HandleTopLevelDecl except in cases where we just finished defining the entity (remove the call from explicit instantiation and from the dllexport code and perhaps add calls to DefineImplicit*)
  • add a new ASTConsumer callback for the case where the properties of an existing declaration have changed (for instance, it's switched from being not-dllexport to being dllexport, or it's switched from being an implicit instantiation to being an explicit instantiation)

We might also wish to switch away from using HandleTopLevelDecl for template instantiations (and special member function definitions, if we ever add it there) because they're not really top-level declarations (they're adding a definition to an existing declaration in many cases), and instead call something like HandleInlineMethodDefinition in those cases. However, that appears to be merely a cleanup rather than a correctness fix.

hans updated this revision to Diff 17478.Dec 18 2014, 7:07 PM
hans updated this object.

Richard's suggestion about having a separate function instead of HandleTopLevelDecl that would be used for updating the linkage after explicit instantiation got me thinking about how that would work if the definition had already been emitted.

To be able to change the linkage from weak to strong, it would be nice if the function was not emitted yet. This is what happens with normal implicit template instantiations today: on first use (which is typically what triggered the instantiation), they get moved into deferredDeclsToEmit, and then stay there until the end of the TU.

The only reason this doesn't work with dllexport or attribute(used) functions is that MayDeferGeneration() returns false for those. That's actually a misnomer because deferring is not a problem for them, as long as they are emitted in the end.

This patch tries to solve the problem by reorganizing the logic about deferring generation.

At least this feels a lot less hacky than the previous patch :)

majnemer accepted this revision.Jan 9 2015, 4:23 PM
majnemer edited edge metadata.

LGTM with nits.

1105–1108 ↗(On Diff #17478)


assert(!GV || GV == GetGlobalValue(getMangledName(D)));
if (!GV)
  GV = GetGlobalValue(getMangledName(D));
1389 ↗(On Diff #17478)

You may want to make this a little more clear:

addDeferredDeclToEmit(/*GV=*/nullptr, GD);
This revision is now accepted and ready to land.Jan 9 2015, 4:23 PM
rsmith accepted this revision.Jan 9 2015, 4:24 PM
rsmith edited edge metadata.


1198–1199 ↗(On Diff #17478)

This sounds like "determine whether either (a) the definition must be emitted or (b) the definition can be emitted lazily" whereas I think you mean "determine whether the definition must be emitted; if this returns \c false, the definition can be emitted lazily if it's used".

hans added a comment.Jan 9 2015, 5:20 PM

Thanks for the reviews!

1105–1108 ↗(On Diff #17478)


1389 ↗(On Diff #17478)


1198–1199 ↗(On Diff #17478)

Thanks! That's much clearer.

This revision was automatically updated to reflect the committed changes.