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.

cfe/trunk/lib/CodeGen/CodeGenModule.h

private:
 
   /// Emit the llvm.gcov metadata used to tell LLVM where to emit the .gcno and
   /// .gcda files in a way that persists in .bc files.
   void EmitCoverageFile();
 
   /// Emits the initializer for a uuidof string.
   llvm::Constant *EmitUuidofInitializer(StringRef uuidstr);
 
-  /// Determine if the given decl can be emitted lazily; this is only relevant
-  /// for definitions. The given decl must be either a function or var decl.
-  bool MayDeferGeneration(const ValueDecl *D);
+  /// Determine whether the definition must be emitted; if this returns \c
+  /// false, the definition can be emitted lazily if it's used.
+  bool MustBeEmitted(const ValueDecl *D);
+
+  /// Determine whether the definition can be emitted eagerly, or should be
+  /// delayed until the end of the translation unit. This is relevant for
+  /// definitions whose linkage can change, e.g. implicit function instantions
+  /// which may later be explicitly instantiated.
+  bool MayBeEmittedEagerly(const ValueDecl *D);
 
   /// Check whether we can use a "simpler", more core exceptions personality
   /// function.
   void SimplifyPersonality();
 };
 }  // end namespace CodeGen
 }  // end namespace clang
 
 #endif

cfe/trunk/lib/CodeGen/CodeGenModule.cpp

while (true) {
     // Stop if we're out of both deferred v-tables and deferred declarations.
     if (DeferredDeclsToEmit.empty()) break;
 
     DeferredGlobal &G = DeferredDeclsToEmit.back();
     GlobalDecl D = G.GD;
     llvm::GlobalValue *GV = G.GV;
     DeferredDeclsToEmit.pop_back();
 
-    assert(GV == GetGlobalValue(getMangledName(D)));
+    assert(!GV || GV == GetGlobalValue(getMangledName(D)));
+    if (!GV)
+      GV = GetGlobalValue(getMangledName(D));
+
+
     // Check to see if we've already emitted this.  This is necessary
     // for a couple of reasons: first, decls can end up in the
     // deferred-decls queue multiple times, and second, decls can end
     // up with definitions in unusual ways (e.g. by an extern inline
     // function acquiring a strong function redefinition).  Just
     // ignore these cases.
-    if(!GV->isDeclaration())
+    if (GV && !GV->isDeclaration())
       continue;
 
     // Otherwise, emit the definition and move on to the next one.
     EmitGlobalDefinition(D, GV);
   }
 }
 
 void CodeGenModule::EmitGlobalAnnotations() {
if (Ty->isRecordType()) {
       std::string TypeStr = Ty.getAsString(getContext().getPrintingPolicy());
       if (SanitizerBL.isBlacklistedType(TypeStr, Category))
         return true;
     }
   }
   return false;
 }
 
-bool CodeGenModule::MayDeferGeneration(const ValueDecl *Global) {
+bool CodeGenModule::MustBeEmitted(const ValueDecl *Global) {
   // Never defer when EmitAllDecls is specified.
   if (LangOpts.EmitAllDecls)
+    return true;
+
+  return getContext().DeclMustBeEmitted(Global);
+}
+
+bool CodeGenModule::MayBeEmittedEagerly(const ValueDecl *Global) {
+  if (const auto *FD = dyn_cast<FunctionDecl>(Global))
+    if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
+      // Implicit template instantiations may change linkage if they are later
+      // explicitly instantiated, so they should not be emitted eagerly.
       return false;
 
-  return !getContext().DeclMustBeEmitted(Global);
+  return true;
 }
 
 llvm::Constant *CodeGenModule::GetAddrOfUuidDescriptor(
     const CXXUuidofExpr* E) {
   // Sema has verified that IIDSource has a __declspec(uuid()), and that its
   // well-formed.
   StringRef Uuid = E->getUuidAsStringRef(Context);
   std::string Name = "_GUID_" + Uuid.lower();
if (const auto *FD = dyn_cast<FunctionDecl>(Global)) {
     const auto *VD = cast<VarDecl>(Global);
     assert(VD->isFileVarDecl() && "Cannot emit local var decl as global.");
 
     if (VD->isThisDeclarationADefinition() != VarDecl::Definition &&
         !Context.isMSStaticDataMemberInlineDefinition(VD))
       return;
   }
 
-  // Defer code generation when possible if this is a static definition, inline
-  // function etc.  These we only want to emit if they are used.
-  if (!MayDeferGeneration(Global)) {
+  // Defer code generation to first use when possible, e.g. if this is an inline
+  // function. If the global must always be emitted, do it eagerly if possible
+  // to benefit from cache locality.
+  if (MustBeEmitted(Global) && MayBeEmittedEagerly(Global)) {
     // Emit the definition if it can't be deferred.
     EmitGlobalDefinition(GD);
     return;
   }
 
   // If we're deferring emission of a C++ variable with an
   // initializer, remember the order in which it appeared in the file.
   if (getLangOpts().CPlusPlus && isa<VarDecl>(Global) &&
       cast<VarDecl>(Global)->hasInit()) {
     DelayedCXXInitPosition[Global] = CXXGlobalInits.size();
     CXXGlobalInits.push_back(nullptr);
   }
-  
-  // If the value has already been used, add it directly to the
-  // DeferredDeclsToEmit list.
   StringRef MangledName = getMangledName(GD);
-  if (llvm::GlobalValue *GV = GetGlobalValue(MangledName))
+  if (llvm::GlobalValue *GV = GetGlobalValue(MangledName)) {
+    // The value has already been used and should therefore be emitted.
     addDeferredDeclToEmit(GV, GD);
-  else {
+  } else if (MustBeEmitted(Global)) {
+    // The value must be emitted, but cannot be emitted eagerly.
+    assert(!MayBeEmittedEagerly(Global));
+    addDeferredDeclToEmit(/*GV=*/nullptr, GD);
+  } else {
     // Otherwise, remember that we saw a deferred decl with this name.  The
     // first use of the mangled name will cause it to move into
     // DeferredDeclsToEmit.
     DeferredDecls[MangledName] = GD;
   }
 }
 
 namespace {

 CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
                                      StringRef Name) {
   return GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
 }
 
 void CodeGenModule::EmitTentativeDefinition(const VarDecl *D) {
   assert(!D->getInit() && "Cannot emit definite definitions here!");
 
-  if (MayDeferGeneration(D)) {
+  if (!MustBeEmitted(D)) {
     // If we have not seen a reference to this variable yet, place it
     // into the deferred declarations table to be emitted if needed
     // later.
     StringRef MangledName = getMangledName(D);
     if (!GetGlobalValue(MangledName)) {
       DeferredDecls[MangledName] = D;
       return;
     }

cfe/trunk/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(); };

cfe/trunk/test/CodeGenCXX/explicit-instantiation.cpp

namespace LateInstantiation {
   // CHECK: define linkonce_odr i32 @_ZN17LateInstantiation1SIcE1fEv(
   // CHECK: define linkonce_odr i32 @_ZN17LateInstantiation1fIcEEiv(
   // CHECK-NO-OPT: declare i32 @_ZN17LateInstantiation1SIiE1fEv(
   // CHECK-NO-OPT: declare i32 @_ZN17LateInstantiation1fIiEEiv(
   // CHECK-OPT: define available_externally i32 @_ZN17LateInstantiation1SIiE1fEv(
   // CHECK-OPT: define available_externally i32 @_ZN17LateInstantiation1fIiEEiv(
 }
 
+namespace PR21718 {
+// The linkage of a used constexpr member function can change from linkonce_odr
+// to weak_odr after explicit instantiation without errors about defining the
+// same function twice.
+template <typename T>
+struct S {
+// CHECK-LABEL: define weak_odr i32 @_ZN7PR217181SIiE1fEv
+  __attribute__((used)) constexpr int f() { return 0; }
+};
+int g() { return S<int>().f(); }
+template struct S<int>;
+}
+
 // Check that we emit definitions from explicit instantiations even when they
 // occur prior to the definition itself.
 template <typename T> struct S {
   void f();
   static void g();
   static int i;
   struct S2 {
     void h();