[dllexport] Instantiate default ctor default args for explicit specializations (PR45811)

[hans]

Summary

For dllexported default constructors with default arguments, we export default constructor closures which pass in the default args. (See D8331 for a good explanation.)

For templates, that means those default args must be instantiated even if the function isn't called. That is done by the InstantiateDefaultCtorDefaultArgs() function, but it wasn't done for explicit specializations, causing asserts (see bug).

[hans]

Please take a look.

clang/lib/Sema/SemaDecl.cpp

13996

I was searching for the right place to do this, looking mostly in the sema template code, but ended up here as an explicit specialization really works out a lot like a function definition.

clang/test/CodeGenCXX/dllexport-ctor-closure.cpp

53

This should also work when dllexport is on the member function rather than the class, but it turns out we currently don't export the function in that case; that's a bug I want to handle separately.

[zahiraam]

In D91089#2383433, @hans wrote:

Please take a look.

@hans Thanks for looking at this.
This test case is still crashing with the patch:

ksh-3.2$ cat test.cpp
template <typename>
class __declspec(dllexport) foo {

foo(int x = 0);

};
template <>
foo<int>::foo(int);
ksh-3.2$

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

[hans]

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

Thanks for catching that!

What's happening is that Clang tries to emit the ctor closure even though the ctor is just declared, but not defined.

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

[hans]

Handling the decl only case.

[zahiraam]

In D91089#2385821, @hans wrote:

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

Thanks for catching that!

What's happening is that Clang tries to emit the ctor closure even though the ctor is just declared, but not defined.

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

Ok. That's a bit different than what MSVC is doing. It generates a closure constructor even if there is only a declaration. But I guess we are not aligning specifically with MSVC!
Look good.

[hans]

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

Ok. That's a bit different than what MSVC is doing. It generates a closure constructor even if there is only a declaration. But I guess we are not aligning specifically with MSVC!
Look good.

Right, I noticed that MSVC does this, but I couldn't come up with any reason why it would be necessary for us. Emitting the closure with the ctor definition should be sufficient.

clang/include/clang/Sema/Sema.h

void InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
                         LocalInstantiationScope *OuterMostScope = nullptr);
 
   void
   InstantiateAttrsForDecl(const MultiLevelTemplateArgumentList &TemplateArgs,
                           const Decl *Pattern, Decl *Inst,
                           LateInstantiatedAttrVec *LateAttrs = nullptr,
                           LocalInstantiationScope *OuterMostScope = nullptr);
 
+  void InstantiateDefaultCtorDefaultArgs(CXXConstructorDecl *Ctor);
+
   bool usesPartialOrExplicitSpecialization(
       SourceLocation Loc, ClassTemplateSpecializationDecl *ClassTemplateSpec);
 
   bool
   InstantiateClassTemplateSpecialization(SourceLocation PointOfInstantiation,
                            ClassTemplateSpecializationDecl *ClassTemplateSpec,
                            TemplateSpecializationKind TSK,
                            bool Complain = true);

clang/lib/Sema/SemaDecl.cpp

if (const auto *Attr = FD->getAttr<AliasAttr>()) {
     FD->dropAttr<AliasAttr>();
     FD->setInvalidDecl();
   }
   if (const auto *Attr = FD->getAttr<IFuncAttr>()) {
     Diag(Attr->getLocation(), diag::err_alias_is_definition) << FD << 1;
     FD->dropAttr<IFuncAttr>();
     FD->setInvalidDecl();
   }
 
+  if (auto *Ctor = dyn_cast<CXXConstructorDecl>(FD)) {
+    if (Ctor->getTemplateSpecializationKind() == TSK_ExplicitSpecialization &&
+        Ctor->isDefaultConstructor() &&
+        Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+      // If this is an MS ABI dllexport default constructor, instantiate any
+      // default arguments.
+      InstantiateDefaultCtorDefaultArgs(Ctor);
+    }
+  }
+
   // See if this is a redefinition. If 'will have body' (or similar) is already
   // set, then these checks were already performed when it was set.
   if (!FD->willHaveBody() && !FD->isLateTemplateParsed() &&
       !FD->isThisDeclarationInstantiatedFromAFriendDefinition()) {
     CheckForFunctionRedefinition(FD, nullptr, SkipBody);
 
     // If we're skipping the body, we're done. Don't enter the scope.
     if (SkipBody && SkipBody->ShouldSkip)

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (TmplAttr->isLateParsed() && LateAttrs) {
       Attr *NewAttr = sema::instantiateTemplateAttribute(TmplAttr, Context,
                                                          *this, TemplateArgs);
       if (NewAttr)
         New->addAttr(NewAttr);
     }
   }
 }
 
+/// In the MS ABI, we need to instantiate default arguments of dllexported
+/// default constructors along with the constructor definition. This allows IR
+/// gen to emit a constructor closure which calls the default constructor with
+/// its default arguments.
+void Sema::InstantiateDefaultCtorDefaultArgs(CXXConstructorDecl *Ctor) {
+  assert(Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+         Ctor->isDefaultConstructor());
+  unsigned NumParams = Ctor->getNumParams();
+  if (NumParams == 0)
+    return;
+  DLLExportAttr *Attr = Ctor->getAttr<DLLExportAttr>();
+  if (!Attr)
+    return;
+  for (unsigned I = 0; I != NumParams; ++I) {
+    (void)CheckCXXDefaultArgExpr(Attr->getLocation(), Ctor,
+                                   Ctor->getParamDecl(I));
+    DiscardCleanupsInEvaluationContext();
+  }
+}
+
 /// Get the previous declaration of a declaration for the purposes of template
 /// instantiation. If this finds a previous declaration, then the previous
 /// declaration of the instantiation of D should be an instantiation of the
 /// result of this function.
 template<typename DeclT>
 static DeclT *getPreviousDeclForInstantiation(DeclT *D) {
   DeclT *Result = D->getPreviousDecl();
 
if (Inst.isInvalid())
     return nullptr;
 
   ContextRAII SavedContext(*this, FD);
   MultiLevelTemplateArgumentList MArgs(*Args);
 
   return cast_or_null<FunctionDecl>(SubstDecl(FD, FD->getParent(), MArgs));
 }
 
-/// In the MS ABI, we need to instantiate default arguments of dllexported
-/// default constructors along with the constructor definition. This allows IR
-/// gen to emit a constructor closure which calls the default constructor with
-/// its default arguments.
-static void InstantiateDefaultCtorDefaultArgs(Sema &S,
-                                              CXXConstructorDecl *Ctor) {
-  assert(S.Context.getTargetInfo().getCXXABI().isMicrosoft() &&
-         Ctor->isDefaultConstructor());
-  unsigned NumParams = Ctor->getNumParams();
-  if (NumParams == 0)
-    return;
-  DLLExportAttr *Attr = Ctor->getAttr<DLLExportAttr>();
-  if (!Attr)
-    return;
-  for (unsigned I = 0; I != NumParams; ++I) {
-    (void)S.CheckCXXDefaultArgExpr(Attr->getLocation(), Ctor,
-                                   Ctor->getParamDecl(I));
-    S.DiscardCleanupsInEvaluationContext();
-  }
-}
-
 /// Instantiate the definition of the given function from its
 /// template.
 ///
 /// \param PointOfInstantiation the point at which the instantiation was
 /// required. Note that this is not precisely a "point of instantiation"
 /// for the function, but it's close.
 ///
 /// \param Function the already-instantiated declaration of a
if (PatternDecl->hasSkippedBody()) {
         // If this is a constructor, instantiate the member initializers.
         InstantiateMemInitializers(Ctor, cast<CXXConstructorDecl>(PatternDecl),
                                    TemplateArgs);
 
         // If this is an MS ABI dllexport default constructor, instantiate any
         // default arguments.
         if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
             Ctor->isDefaultConstructor()) {
-          InstantiateDefaultCtorDefaultArgs(*this, Ctor);
+          InstantiateDefaultCtorDefaultArgs(Ctor);
         }
       }
 
       // Instantiate the function body.
       Body = SubstStmt(Pattern, TemplateArgs);
 
       if (Body.isInvalid())
         Function->setInvalidDecl();

clang/test/CodeGenCXX/dllexport-ctor-closure.cpp

 template struct __declspec(dllexport) TemplateWithClosure<int>;
 
 // CHECK-LABEL: define weak_odr dso_local dllexport x86_thiscallcc void @"??_F?$TemplateWithClosure@D@@QAEXXZ"({{.*}}) {{#[0-9]+}} comdat
 // CHECK:   call {{.*}} @"??0?$TemplateWithClosure@D@@QAE@H@Z"({{.*}}, i32 1)
 
 // CHECK-LABEL: define weak_odr dso_local dllexport x86_thiscallcc void @"??_F?$TemplateWithClosure@H@@QAEXXZ"({{.*}}) {{#[0-9]+}} comdat
 // CHECK:   call {{.*}} @"??0?$TemplateWithClosure@H@@QAE@H@Z"({{.*}}, i32 4)
 
+template <typename T> struct __declspec(dllexport) ExportedTemplateWithClosure {
+  ExportedTemplateWithClosure(int x = sizeof(T)) {}
+};
+template <> ExportedTemplateWithClosure<int>::ExportedTemplateWithClosure(int) {};
+// CHECK-LABEL: define weak_odr dso_local dllexport x86_thiscallcc void @"??_F?$ExportedTemplateWithClosure@H@@QAEXXZ"({{.*}}) {{#[0-9]+}} comdat
+// CHECK:   call {{.*}} @"??0?$ExportedTemplateWithClosure@H@@QAE@H@Z"({{.*}}, i32 4)
+
 struct __declspec(dllexport) NestedOuter {
   DELETE_IMPLICIT_MEMBERS(NestedOuter);
   NestedOuter(void *p = 0) {}
   struct __declspec(dllexport) NestedInner {
     DELETE_IMPLICIT_MEMBERS(NestedInner);
     NestedInner(void *p = 0) {}
   };
 };

clang/test/SemaCXX/dllexport.cpp

 HasDefaults<void>; // expected-note {{in instantiation of member function 'HasDefaults<void>::HasDefaults' requested here}}
 
 template <typename T> struct HasDefaults2 {
   __declspec(dllexport) // expected-note {{in instantiation of default function argument expression for 'HasDefaults2<void>' required here}}
   HasDefaults2(int x = sizeof(T)) {} // expected-error {{invalid application of 'sizeof'}}
 };
 template struct HasDefaults2<void>; // expected-note {{in instantiation of member function 'HasDefaults2<void>::HasDefaults2' requested here}}
 
+template <typename T> struct __declspec(dllexport) HasDefaults3 { // expected-note{{in instantiation of default function argument expression for 'HasDefaults3<void>' required here}}
+  HasDefaults3(int x = sizeof(T)) {} // expected-error {{invalid application of 'sizeof'}}
+};
+template <> HasDefaults3<void>::HasDefaults3(int) {};
+
 #endif
 
 //===----------------------------------------------------------------------===//
 // Class member templates
 //===----------------------------------------------------------------------===//
 
 struct ExportMemberTmpl {
   template<typename T> __declspec(dllexport)               void normalDecl();