This is an archive of the discontinued LLVM Phabricator instance.

[MinGW] Produce a vtable and RTTI for dllexported classes without a key function
ClosedPublic

Authored by mstorsjo on Dec 14 2018, 2:25 AM.

Details

Summary

This matches what GCC does in these situations.

This fixes compiling Qt in debug mode. In release mode, references to the vtable of this particular class ends up optimized away, but in debug mode, the compiler creates references to the vtable, which is expected to be dllexported from a different DLL. Make sure the dllexported version actually ends up emitted.

Diff Detail

Event Timeline

mstorsjo created this revision.Dec 14 2018, 2:25 AM
mstorsjo updated this revision to Diff 178222.Dec 14 2018, 5:17 AM

Fixed wrapping of the RUN line in the test.

rnk added inline comments.Dec 14 2018, 12:06 PM
lib/Sema/SemaDeclCXX.cpp
5754–5756

This may be too early, you can get into situations like this if you start emitting the vtable (which will emit inline methods) before we get to the end of the outermost class. See this bug for example:
https://bugs.llvm.org/show_bug.cgi?id=40006

Maybe if you have a dllexported nested class with a virtual method that references the constructor of the outer class which has a late-parsed member initializer... you can get things to go wrong as in the bug above.

I think the fix will be to touch the vtable when we process delayed dllexported classes from the list just above this line.

test/CodeGenCXX/dllexport-missing-key.cpp
20

OK, good, I was going to ask if it became weak_odr, but looks like it all works.

mstorsjo marked 2 inline comments as done.Dec 14 2018, 1:34 PM
mstorsjo added inline comments.
lib/Sema/SemaDeclCXX.cpp
5754–5756

Ok, will upload a new version of the patch.

mstorsjo updated this revision to Diff 178275.Dec 14 2018, 1:35 PM

Moved the code to work on DelayedDllExportClasses instead, as suggested, which still works for the testcase. (I've yet to test that approach on a larger codebase though.)

rnk accepted this revision.Dec 14 2018, 3:25 PM

lgtm

This revision is now accepted and ready to land.Dec 14 2018, 3:25 PM
This revision was automatically updated to reflect the committed changes.