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

[hans]

Summary

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)

[hans]

+ehsan fyi

[rsmith]

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]

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 :)

[hans]

Ping?

[hans]

Ping?

[majnemer]

LGTM with nits.

lib/CodeGen/CodeGenModule.cpp

1105–1108 ↗

(On Diff #17478)

Maybe:

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);

[rsmith]

LGTM

lib/CodeGen/CodeGenModule.h

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]

Thanks for the reviews!

lib/CodeGen/CodeGenModule.cpp

1105–1108 ↗

(On Diff #17478)

Done.

1389 ↗

(On Diff #17478)

Done.

lib/CodeGen/CodeGenModule.h

1198–1199 ↗

(On Diff #17478)

Thanks! That's much clearer.

lib/Sema/SemaDeclCXX.cpp

for (Decl *Member : Class->decls()) {
     if (!getDLLAttr(Member)) {
       auto *NewAttr =
           cast<InheritableAttr>(ClassAttr->clone(S.getASTContext()));
       NewAttr->setInherited(true);
       Member->addAttr(NewAttr);
     }
 
     if (MD && ClassExported) {
+      // Instantiations of constexpr template member functions are eagerly
+      // emitted by MarkFunctionReferenced, and also emitted by
+      // InstantiateClassMembers. To avoid emitting the function twice, pass
+      // OdrUse = false to MarkFunctionReferenced and don't pass them to the
+      // consumer (PR21718).
+      bool OdrUse = !MD->isConstexpr() ||
+                    TSK != TSK_ExplicitInstantiationDefinition ||
+                    !MD->getInstantiatedFromMemberFunction();
+
       if (MD->isUserProvided()) {
         // Instantiate non-default class member functions ...
 
         // .. except for certain kinds of template specializations.
         if (TSK == TSK_ExplicitInstantiationDeclaration)
           continue;
         if (TSK == TSK_ImplicitInstantiation && !ClassAttr->isInherited())
           continue;
 
-        S.MarkFunctionReferenced(Class->getLocation(), MD);
+        S.MarkFunctionReferenced(Class->getLocation(), MD, OdrUse);
 
         // The function will be passed to the consumer when its definition is
         // encountered.
       } else if (!MD->isTrivial() || MD->isExplicitlyDefaulted() ||
                  MD->isCopyAssignmentOperator() ||
                  MD->isMoveAssignmentOperator()) {
         // Synthesize and instantiate non-trivial implicit methods, explicitly
         // defaulted methods, and the copy and move assignment operators. The
         // latter are exported even if they are trivial, because the address of
         // an operator can be taken and should compare equal accross libraries.
-        S.MarkFunctionReferenced(Class->getLocation(), MD);
+        S.MarkFunctionReferenced(Class->getLocation(), MD, OdrUse);
 
         // There is no later point when we will see the definition of this
         // function, so pass it to the consumer now.
+        if (OdrUse)
           S.Consumer.HandleTopLevelDecl(DeclGroupRef(MD));
       }
     }
   }
 }
 
 /// \brief Perform semantic checks on a class definition that has been
 /// completing, introducing implicitly-declared members, checking for
 /// abstract types, etc.

test/CodeGenCXX/dllexport.cpp

 struct NonExportedBaseClass {
   virtual ~NonExportedBaseClass();
 };
 NonExportedBaseClass::~NonExportedBaseClass() {}
 
 struct __declspec(dllexport) ExportedDerivedClass : NonExportedBaseClass {};
 // M32-DAG: weak_odr dllexport x86_thiscallcc void @"\01??1ExportedDerivedClass@@UAE@XZ"
 
+// Do not assert about generating code for constexpr functions twice during explicit instantiation (PR21718).
+template <typename T> struct ExplicitInstConstexprMembers {
+  // Copy assignment operator
+  // M32-DAG: define weak_odr dllexport x86_thiscallcc dereferenceable(1) %struct.ExplicitInstConstexprMembers* @"\01??4?$ExplicitInstConstexprMembers@X@@QAEAAU0@ABU0@@Z"
+
+  constexpr ExplicitInstConstexprMembers() {}
+  // M32-DAG: define weak_odr dllexport x86_thiscallcc %struct.ExplicitInstConstexprMembers* @"\01??0?$ExplicitInstConstexprMembers@X@@QAE@XZ"
+
+  ExplicitInstConstexprMembers(const ExplicitInstConstexprMembers&) = default;
+  // M32-DAG: define weak_odr dllexport x86_thiscallcc %struct.ExplicitInstConstexprMembers* @"\01??0?$ExplicitInstConstexprMembers@X@@QAE@ABU0@@Z"
+
+  constexpr int f() const { return 42; }
+  // M32-DAG: define weak_odr dllexport x86_thiscallcc i32 @"\01?f@?$ExplicitInstConstexprMembers@X@@QBEHXZ"
+};
+template struct __declspec(dllexport) ExplicitInstConstexprMembers<void>;
 
 //===----------------------------------------------------------------------===//
 // Classes with template base classes
 //===----------------------------------------------------------------------===//
 
 template <typename T> struct ClassTemplate { void func() {} };
 template <typename T> struct __declspec(dllexport) ExportedClassTemplate { void func() {} };
 template <typename T> struct __declspec(dllimport) ImportedClassTemplate { void func(); };