Page MenuHomePhabricator

Start adding support for dllimport/dllexport on classes (PR11170)
ClosedPublic

Authored by hans on May 22 2014, 10:03 AM.

Details

Summary

This implements the central part of support for dllimport/dllexport on classes: allowing the attribute on class declarations, inheriting it to class members, and forcing emission of exported members. It's based on Nico Rieck's patch from http://reviews.llvm.org/D1099.

This patch doesn't propagate dllexport to bases that are template specializations, which is an interesting problem. It also doesn't look at the rules when redeclaring classes with different attributes, I'd like to do that separately.

Diff Detail

Repository
rL LLVM

Event Timeline

hans updated this revision to Diff 9706.May 22 2014, 10:03 AM
hans retitled this revision from to Start adding support for dllimport/dllexport on classes (PR11170).
hans updated this object.
hans edited the test plan for this revision. (Show Details)
hans added reviewers: rnk, nrieck.
hans added subscribers: Unknown Object (MLST), hansw.
rsmith added a subscriber: rsmith.May 27 2014, 4:51 PM

Looks basically fine. Just a few comments.

lib/CodeGen/CodeGenModule.cpp
1395 ↗(On Diff #9706)

This would make more sense to me in SetFunctionAttributes, where (presumably) the DLL storage class is set.

lib/Sema/SemaDeclCXX.cpp
4394 ↗(On Diff #9706)

What about member classes? Are they implicitly re-exported too?

4404 ↗(On Diff #9706)

ClassExported && MD->isUserProvided() maybe

4407–4408 ↗(On Diff #9706)

Weird! You get an exported trivial copy assignment but not an exported trivial copy constructor? I guess we'll need to update this once we find out what they do with implicit move operations?

4415 ↗(On Diff #9706)

Why do we need this here but not in the non-defaulted case?

hans added inline comments.May 27 2014, 7:19 PM
lib/CodeGen/CodeGenModule.cpp
1395 ↗(On Diff #9706)

I broke the dtor thunk related changes out into r209706.

lib/Sema/SemaDeclCXX.cpp
4394 ↗(On Diff #9706)

Nope.

4404 ↗(On Diff #9706)

Yes, that sounds good.

4407–4408 ↗(On Diff #9706)

Yes. I have no idea why they always export that copy assignment operator, but they do.

4415 ↗(On Diff #9706)

I don't remember exactly, but I never ran into trouble with the non-default methods.

hans updated this revision to Diff 9859.May 27 2014, 7:21 PM
hans updated this object.

Thanks for the review!

I've rebased on top of r209706 and addressed Richard's comments.

ygao added a subscriber: ygao.May 27 2014, 7:29 PM
nrieck edited edge metadata.May 27 2014, 9:16 PM

I also have a few more tests you should use. For example currently sema diagnoses dllimport redeclarations and drops this attribute, but class template member functions would still be imported.

lib/Sema/SemaDeclCXX.cpp
4375 ↗(On Diff #9859)

You picked this from my old patch but I now think that this diagnostic wouldn't buy much. MSVC doesn't do it, and it's perfectly valid to return for example value objects. With this removed I just inlined this whole function into the loop below.

4393 ↗(On Diff #9859)

I wonder what MS exactly means by that since it's valid to inherit from a non-exported class or even an imported class.

4412 ↗(On Diff #9859)

When is this branch actually needed?

4413 ↗(On Diff #9859)

This check is redundant.

4424 ↗(On Diff #9859)

This fails to export defaulted members for the MS abi (Mingw currently does not export them).

hans added a comment.May 28 2014, 9:48 AM

Thanks for the review!

I also have a few more tests you should use. For example currently sema diagnoses dllimport redeclarations and drops this attribute, but class template member functions would still be imported.

I'd like to look into the whole redeclaration business in a separate patch. It seems the rules are different for redeclaring classes and other things are different with respect to these attributes.

lib/Sema/SemaDeclCXX.cpp
4375 ↗(On Diff #9859)

Sounds good to me.

4393 ↗(On Diff #9859)

It's pretty confusing.. Their docs on http://msdn.microsoft.com/en-us/library/81h27t8c.aspx specifically say "All base classes of an exportable class must be exportable. If not, a compiler warning is generated."

4412 ↗(On Diff #9859)

It's needed for this case:

template <typename T> struct declspec(dllexport) U { void foo() {} };
struct
declspec(dllexport) V : public U<int> { };

to make us instantiate U<int>::foo().

4413 ↗(On Diff #9859)

Thanks! Removed.

4424 ↗(On Diff #9859)

Thanks for catching that!

hans updated this revision to Diff 9883.May 28 2014, 9:48 AM
hans edited edge metadata.

Addressing Nico's comments and rebasing.

hans updated this revision to Diff 9884.May 28 2014, 10:38 AM

David said he was using the November CTP of MSVC to observe the move assignment operator getting exported. I've updated the patch so we export it too.

hans updated this revision to Diff 9937.May 29 2014, 4:01 PM

I tested my patch with Nico's tests from https://github.com/gix/clang/commit/ea6cb1d71c6ef8dac2afd5f40219de9460a92b3f, and fixed the two asserts we were hitting when exporting defaulted trivial constructors and destructors.

There were two remaining test failures: 1) we emit RTTI type descriptor, etc. as weak_odr instead of linkonce_odr when exporting the class. Not a big deal since we're exporting the vtable anyway, but I'm not sure why it's happening. 2) for the MinGW ABI, we don't export the vtable for ExportDynamicClass, but we should.

If folks think it's OK, it'd like to get this patch landed, let Nico commit his tests, and then continue addressing the issues above.

(I had to updated Nico's tests with 'nonnull' to make them work, see the commit here: https://github.com/zmodem/clang/commit/c62c92967e608a4ff90d18887b848122c6efbeb8)

rnk accepted this revision.May 29 2014, 5:35 PM
rnk edited edge metadata.

lgtm

There's still work to be done, but Richard thinks this is "basically fine" and IMO it's time to land and iterate.

lib/CodeGen/CGVTables.cpp
663 ↗(On Diff #9937)

"Imported vtables"

lib/CodeGen/MicrosoftCXXABI.cpp
205 ↗(On Diff #9937)

I wonder why thunks are WeakAny. That seems wrong, but it's a separate issue.

920 ↗(On Diff #9937)

This should probably also go in getAddrOfVTable. I wonder if that explains the MinGW issue.

1230 ↗(On Diff #9937)

This should be in getAddrOfVBTable. We might want to avoid emitting available externally vbtables at -O0 and simply import them from the dll.

lib/Sema/SemaDeclCXX.cpp
4385 ↗(On Diff #9937)

We shouldn't be adding these attributes to FieldDecls. I suppose we want to export nested classes, though.

This revision is now accepted and ready to land.May 29 2014, 5:35 PM
nrieck added inline comments.May 29 2014, 5:42 PM
lib/Sema/SemaDeclCXX.cpp
4385 ↗(On Diff #9937)

I'd use a whitelist and only propagate it to methods and static data members. Nested classes aren't automatically exported or imported.

hans added a comment.May 30 2014, 10:05 AM

Thanks everyone for the review!

lib/CodeGen/CGVTables.cpp
663 ↗(On Diff #9937)

Done.

lib/CodeGen/MicrosoftCXXABI.cpp
920 ↗(On Diff #9937)

Done.

I guess for MinGW we'd need to add it to the Itanium ABI's getAddrOfVTable.

1230 ↗(On Diff #9937)

Done.

lib/Sema/SemaDeclCXX.cpp
4385 ↗(On Diff #9937)

Oops, this fell out when I merged the separate function into this loop. Fixed now.

hans closed this revision.May 30 2014, 10:07 AM
hans updated this revision to Diff 9963.

Closed by commit rL209908 (authored by @hans).