Page MenuHomePhabricator

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

Authored by mstorsjo on Jan 29 2018, 5:28 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Jan 29 2018, 5:28 AM
mstorsjo updated this revision to Diff 131783.Jan 29 2018, 5:31 AM

Remove a few accidentally leftover parts of the new testcase.

majnemer added inline comments.Jan 29 2018, 8:53 AM
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 added inline comments.Jan 29 2018, 11:59 AM
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 updated this revision to Diff 132045.Jan 30 2018, 1:53 PM

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?

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 added inline comments.Jan 31 2018, 7:27 PM
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 added inline comments.Jan 31 2018, 11:34 PM
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 accepted this revision.Feb 1 2018, 2:06 PM

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.

This revision is now accepted and ready to land.Feb 1 2018, 2:06 PM
This revision was automatically updated to reflect the committed changes.
mstorsjo added a subscriber: hans.Feb 1 2018, 11:35 PM

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

hans added a comment.Feb 2 2018, 6:06 AM

@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 added a comment.Feb 5 2018, 2:01 AM
In D42641#996117, @hans 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.