Delay emitting dllexport explicitly defaulted members until the class is fully parsed (PR40006)

[hans]

Summary

This is similar to r245139, but that only addressed dllexported classes. It was still possible to run into the same problem with dllexported members in an otherwise normal class (see bug). This uses the same strategy to fix: delay defining the method until the whole class has been parsed.

(The easiest way to see the ordering problem is in Parser::ParseCXXMemberSpecification(): it calls ParseLexedMemberInitializers() *after* ActOnFinishCXXMemberDecls(), which was trying to define the dllexport method. Now we delay it to ActOnFinishCXXNonNestedClass() which is called after both of those.)

[rnk]

clang/lib/Sema/SemaDeclCXX.cpp

11545 ↗

(On Diff #212568)

Do we need to do this until fixpoint? Suppose a dllexported implicit special member triggers a template instantiation, and the template has a dllexported defaulted special member, something like:

struct Bar { Bar(); };
template <typename T> struct Foo { __declspec(dllexport) Foo() = default; Bar obj; };
struct Baz {
   ... not sure how to trigger instantiation
};

[hans]

clang/lib/Sema/SemaDeclCXX.cpp

11545 ↗

(On Diff #212568)

I think that should work, and that's why the function is written to be re-entrant by having a local worklist. If it triggers a template instantiation, ActOnFinishCXXNonNestedClass should get called for the newly instantiated class. But I'm also not sure how to write a test case that would trigger it.

[rnk]

lgtm

clang/lib/Sema/SemaDeclCXX.cpp

11545 ↗

(On Diff #212568)

Right, I see. Nevermind then.

[aganea]

clang/lib/Sema/SemaDeclCXX.cpp

11545 ↗

(On Diff #212568)

This works on latest MSVC:

$ cat test.cc
struct Bar { Bar(); };
template <typename T> struct Foo { __declspec(dllexport) Foo() = default; Bar b; };
template class Foo<int>;

$ cl /c /Z7 test.cc   <-- crashes if using clang-cl

$ lib test.obj /def

$ cat test2.cc
struct Bar { Bar() { } };
template <typename T> struct Foo { __declspec(dllimport) Foo() = default; Bar b; };
int main() {
  Foo<int> f;
  return 0;
}

$ cl /c /Z7 test2.cc

$ link test.lib test2.cc /DEBUG

[hans]

Sorry, seems I forgot to submit this comment..

clang/lib/Sema/SemaDeclCXX.cpp

11545 ↗

(On Diff #212568)

Thanks! I believe that's a separate issue, though, and not related to this patch. I've filed https://bugs.llvm.org/show_bug.cgi?id=42857 for it.

cfe/trunk/include/clang/Sema/Sema.h

static bool TooManyArguments(size_t NumParams, size_t NumArgs,
     if (NumArgs > 0 && PartialOverloading)
       return NumArgs + 1 > NumParams; // If so, we view as an extra argument.
     return NumArgs > NumParams;
   }
 
   // Emitting members of dllexported classes is delayed until the class
   // (including field initializers) is fully parsed.
   SmallVector<CXXRecordDecl*, 4> DelayedDllExportClasses;
+  SmallVector<CXXMethodDecl*, 4> DelayedDllExportMemberFunctions;
 
 private:
   class SavePendingParsedClassStateRAII {
   public:
     SavePendingParsedClassStateRAII(Sema &S) : S(S) { swapSavedState(); }
 
     ~SavePendingParsedClassStateRAII() {
       assert(S.DelayedOverridingExceptionSpecChecks.empty() &&

cfe/trunk/lib/Sema/Sema.cpp

void Sema::ActOnEndOfTranslationUnit() {
 
   // All delayed member exception specs should be checked or we end up accepting
   // incompatible declarations.
   assert(DelayedOverridingExceptionSpecChecks.empty());
   assert(DelayedEquivalentExceptionSpecChecks.empty());
 
   // All dllexport classes should have been processed already.
   assert(DelayedDllExportClasses.empty());
+  assert(DelayedDllExportMemberFunctions.empty());
 
   // Remove file scoped decls that turned out to be used.
   UnusedFileScopedDecls.erase(
       std::remove_if(UnusedFileScopedDecls.begin(nullptr, true),
                      UnusedFileScopedDecls.end(),
                      [this](const DeclaratorDecl *DD) {
                        return ShouldRemoveFromUnused(this, DD);
                      }),

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

if (!M->isInvalidDecl() && M->isExplicitlyDefaulted() &&
         M->hasAttr<DLLExportAttr>()) {
       if (getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015) &&
           M->isTrivial() &&
           (CSM == CXXDefaultConstructor || CSM == CXXCopyConstructor ||
            CSM == CXXDestructor))
         M->dropAttr<DLLExportAttr>();
 
       if (M->hasAttr<DLLExportAttr>()) {
-        DefineImplicitSpecialMember(*this, M, M->getLocation());
-        ActOnFinishInlineFunctionDef(M);
+        // Define after any fields with in-class initializers have been parsed.
+        DelayedDllExportMemberFunctions.push_back(M);
       }
     }
   };
 
   bool HasMethodWithOverrideControl = false,
        HasOverridingMethodWithoutOverrideControl = false;
   if (!Record->isDependentType()) {
     // Check the destructor before any other member function. We need to
if (Record->isInvalidDecl()) {
       return;
     }
     checkForMultipleExportedDefaultConstructors(*this, Record);
   }
 }
 
 void Sema::ActOnFinishCXXNonNestedClass(Decl *D) {
   referenceDLLExportedClassMethods();
+
+  if (!DelayedDllExportMemberFunctions.empty()) {
+    SmallVector<CXXMethodDecl*, 4> WorkList;
+    std::swap(DelayedDllExportMemberFunctions, WorkList);
+    for (CXXMethodDecl *M : WorkList) {
+      DefineImplicitSpecialMember(*this, M, M->getLocation());
+      ActOnFinishInlineFunctionDef(M);
+    }
+  }
 }
 
 void Sema::referenceDLLExportedClassMethods() {
   if (!DelayedDllExportClasses.empty()) {
     // Calling ReferenceDllExportedMembers might cause the current function to
     // be called again, so use a local copy of DelayedDllExportClasses.
     SmallVector<CXXRecordDecl *, 4> WorkList;
     std::swap(DelayedDllExportClasses, WorkList);

cfe/trunk/test/CodeGenCXX/dllexport.cpp

 };
 struct __declspec(dllexport) Baz {
   Bar<int> n;
 };
 // After parsing Baz, in ActOnFinishCXXNonNestedClass we would synthesize
 // Baz's operator=, causing instantiation of Foo<int> after which
 // ActOnFinishCXXNonNestedClass is called, and we would bite our own tail.
 // M32-DAG: define weak_odr dso_local dllexport x86_thiscallcc dereferenceable(1) %"struct.InClassInits::Baz"* @"??4Baz@InClassInits@@QAEAAU01@ABU01@@Z"
+
+// Trying to define the explicitly defaulted ctor must be delayed until the
+// in-class initializer for x has been processed.
+struct PR40006 {
+  __declspec(dllexport) PR40006() = default;
+  int x = 42;
+};
+// M32-DAG: define weak_odr dso_local dllexport x86_thiscallcc %"struct.InClassInits::PR40006"* @"??0PR40006@InClassInits@@QAE@XZ"
+
 }
 
 // We had an issue where instantiating A would force emission of B's delayed
 // exported methods.
 namespace pr26490 {
 template <typename T> struct A { };
 struct __declspec(dllexport) B {
   B(int = 0) {}