[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 ↗

(On Diff #131783)

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

896–900 ↗

(On Diff #131783)

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

[mstorsjo]

lib/CodeGen/CGVTables.cpp

887–888 ↗

(On Diff #131783)

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 ↗

(On Diff #131783)

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.

cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp

if (const RecordType *RecordTy = dyn_cast<RecordType>(Ty)) {
     if (!RD->isDynamicClass())
       return false;
 
     // FIXME: this may need to be reconsidered if the key function
     // changes.
     // N.B. We must always emit the RTTI data ourselves if there exists a key
     // function.
     bool IsDLLImport = RD->hasAttr<DLLImportAttr>();
+
+    // Don't import the RTTI but emit it locally.
+    if (CGM.getTriple().isWindowsGNUEnvironment() && IsDLLImport)
+      return false;
+
     if (CGM.getVTables().isVTableExternal(RD))
       return IsDLLImport && !CGM.getTriple().isWindowsItaniumEnvironment()
                  ? false
                  : true;
 
     if (IsDLLImport)
       return true;
   }

cfe/trunk/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 = available_externally dllimport
+// GNU-DAG: @_ZTS11QObjectData = linkonce_odr
+// GNU-DAG: @_ZTI11QObjectData = linkonce_odr

cfe/trunk/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