[MinGW] Emit typeinfo locally for dllimported classes without key functions

[mstorsjo]

Summary

This fixes building Qt as shared libraries with clang in MinGW mode; previously subclasses of the QObjectData class (in other DLLs than the base DLL) failed to find the typeinfo symbols (that neither were emitted in the base DLL or in the subclass component).

If the virtual destructor in the newly added testcase wouldn't be pure (or if there'd be another non-pure virtual method), it'd be a key function and things would work out as intended. Make sure to locally emit the typeinfo for those classes as well.

This matches what GCC does in this specific testcase (although it's unclear whether the generic mechanics are the same or different).

This fixes the root issue that spawned PR35146. The difference to GCC that is described in that bug still is present though.

I admit that this is a kinda hack-and-slash type of fix since I don't really understand how all these concepts fit together. Better suggestions on solving that particular issue are welcome.

[mstorsjo]

Remove a few accidentally leftover parts of the new testcase.

[majnemer]

lib/CodeGen/CGVTables.cpp

887–888

Maybe a comment like "VTables of classes declared as dllimport are always considered to be external." ?

896–900

It would be good to have tests for what would have happened if these paths got hit.

[mstorsjo]

lib/CodeGen/CGVTables.cpp

887–888

Ok, I can add that. I wasn't quite sure what to say, since I only ended up here in trying to get ShouldUseExternalRTTIDescriptor in ItaniumCXXABI to return false. Alternatively I could just add an "if (GNU && DLLImport) return false;" in that function instead, if that'd cause less collateral changes (like the vtable in dllimport.cpp that changes from available_externally to external).

896–900

Ok - can you hint on what kind of constructs and setups are needed to get here?

[mstorsjo]

Adjusted the fix by moving the change into ShouldUseExternalRTTIDescriptor - this causes less changes to other tests. @majnemer - do you think this is better or worse than having the fix in isVTableExternal?

[compnerd]

I think that the new version is better.

lib/CodeGen/ItaniumCXXABI.cpp

2766 ↗

(On Diff #132045)

Can't this be simplified to ignore the IsDLLImport? If it is export, it should be emitted, if it is import, it should emit it locally. If it is static, it should also emit it locally, no?

[mstorsjo]

lib/CodeGen/ItaniumCXXABI.cpp

2766 ↗

(On Diff #132045)

Hmm, maybe - I tried to keep the condition as specific as possible for the one case that doesn't work right now. I think that might give more stray changes to tests though.

[mstorsjo]

lib/CodeGen/ItaniumCXXABI.cpp

2766 ↗

(On Diff #132045)

Doing that change requires changes in CodeGenCXX/vtable-key-function-ios.cpp, where a non-dllimport typeinfo previous was referred to externally but now would be imported. It's probably harmless but produces more typeinfo instances than what actually is required. (Building Qt with that change still succeeds.)

[rnk]

This is actually consistent with what Microsoft does for dllimport classes. They don't have key functions, but they do import vftables when a class is dllimport and the constructor is inline. They never import RTTI and always emit it locally.

In any case, yes, this organization makes the most sense to me. Presumably GCC also exports its vtables so that we can import them and they don't need to be emitted locally, but we do need to emit RTTI locally.

[mstorsjo]

@hans I'd like to have this in 6.0 as well, to allow building Qt for windows as DLLs.

[hans]

In D42641#995845, @mstorsjo wrote:

@hans I'd like to have this in 6.0 as well, to allow building Qt for windows as DLLs.

Okay, let's just have it bake in trunk a little bit longer and then I'll merge.

[hans]

In D42641#996117, @hans wrote:

In D42641#995845, @mstorsjo wrote:

@hans I'd like to have this in 6.0 as well, to allow building Qt for windows as DLLs.

Okay, let's just have it bake in trunk a little bit longer and then I'll merge.

Merged in r324219.

lib/CodeGen/CGVTables.cpp

 bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) {
   assert(RD->isDynamicClass() && "Non-dynamic classes have no VTable.");
 
   // We always synthesize vtables if they are needed in the MS ABI. MSVC doesn't
   // emit them even if there is an explicit template instantiation.
   if (CGM.getTarget().getCXXABI().isMicrosoft())
     return false;
 
+  if (CGM.getTriple().isWindowsGNUEnvironment() && RD->hasAttr<DLLImportAttr>())
+    return true;
+
   // If we have an explicit instantiation declaration (and not a
   // definition), the vtable is defined elsewhere.
   TemplateSpecializationKind TSK = RD->getTemplateSpecializationKind();
   if (TSK == TSK_ExplicitInstantiationDeclaration)
     return true;
 
   // Otherwise, if the class is an instantiated template, the
   // vtable must be defined here.
   if (TSK == TSK_ImplicitInstantiation ||
       TSK == TSK_ExplicitInstantiationDefinition)
     return false;
 
   // Otherwise, if the class doesn't have a key function (possibly
   // anymore), the vtable must be defined here.
   const CXXMethodDecl *keyFunction = CGM.getContext().getCurrentKeyFunction(RD);
   if (!keyFunction)
     return false;
 
   // Otherwise, if we don't have a definition of the key function, the

test/CodeGenCXX/dllimport-missing-key.cpp

+// RUN: %clang_cc1 -triple i686-windows-gnu -emit-llvm -std=c++1y -O0 -o - %s -w | FileCheck --check-prefix=GNU %s
+
+class __declspec(dllimport) QObjectData {
+public:
+    virtual ~QObjectData() = 0;
+    void *ptr;
+
+    int method() const;
+};
+
+class LocalClass : public QObjectData {
+};
+
+void call() {
+    (new LocalClass())->method();
+}
+
+// GNU-DAG: @_ZTV11QObjectData = external dllimport
+// GNU-DAG: @_ZTS11QObjectData = linkonce_odr
+// GNU-DAG: @_ZTI11QObjectData = linkonce_odr

test/CodeGenCXX/dllimport-rtti.cpp

 // RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++1y -fms-extensions -O1 -disable-llvm-passes -o - %s | FileCheck %s --check-prefix=MSVC
 // RUN: %clang_cc1 -triple i686-windows-gnu  -emit-llvm -std=c++1y -fms-extensions -O1 -disable-llvm-passes -o - %s | FileCheck %s --check-prefix=GNU
 
 struct __declspec(dllimport) S {
   virtual void f() {}
 } s;
 // MSVC: [[VF_S:.*]] = private unnamed_addr constant { [2 x i8*] }
 // MSVC-DAG: @"\01??_SS@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ({ [2 x i8*] }, { [2 x i8*] }* [[VF_S]], i32 0, i32 0, i32 1)
 // MSVC-DAG: @"\01??_R0?AUS@@@8" = linkonce_odr
 // MSVC-DAG: @"\01??_R1A@?0A@EA@S@@8" = linkonce_odr
 // MSVC-DAG: @"\01??_R2S@@8" = linkonce_odr
 // MSVC-DAG: @"\01??_R3S@@8" = linkonce_odr
 
 // GNU-DAG: @_ZTV1S = available_externally dllimport
-// GNU-DAG: @_ZTI1S = external dllimport
+// GNU-DAG: @_ZTI1S = linkonce_odr
 
 struct U : S {
 } u;
 
 struct __declspec(dllimport) V {
   virtual void f();
 } v;
 // GNU-DAG: @_ZTV1V = available_externally dllimport

test/CodeGenCXX/dllimport.cpp

 // MO1-DAG: define available_externally dllimport x86_thiscallcc void @"\01?foo@?$U@H@@QAEXXZ"
 struct __declspec(dllimport) V : public U<int> { };
 USEMEMFUNC(V, foo)
 
 struct __declspec(dllimport) W { virtual void foo() {} };
 USECLASS(W)
 // vftable:
 // MO1-DAG: @"\01??_SW@@6B@" = linkonce_odr unnamed_addr constant { [1 x i8*] } { [1 x i8*] [i8* bitcast (void (%struct.W*)* @"\01?foo@W@@UAEXXZ" to i8*)] }
-// GO1-DAG: @_ZTV1W = available_externally dllimport unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.W*)* @_ZN1W3fooEv to i8*)] }
+// GO1-DAG: @_ZTV1W = external dllimport unnamed_addr constant { [3 x i8*] }
 
 struct __declspec(dllimport) KeyFuncClass {
   constexpr KeyFuncClass() {}
   virtual void foo();
 };
 extern constexpr KeyFuncClass keyFuncClassVar = {};
 // G32-DAG: @_ZTV12KeyFuncClass = external dllimport unnamed_addr constant { [3 x i8*] }