This is an archive of the discontinued LLVM Phabricator instance.

MS ABI: Propagate class-level DLL attributes to class template specialization bases (PR11170)
ClosedPublic

Authored by hans on Jun 23 2014, 4:21 PM.

Details

Summary

This implements what I believe is the final piece of PR11170, modulo bugs.

Consider the following code:

template <typename T> class Base {};
class __declspec(dllexport) class Derived : public Base<int> {}

When the base of an exported or imported class is a class template specialization, MSVC will propagate the dll attribute to the base. In the example code, Base<int> becomes a dllexported class.

My patch does this when the base hasn't been instantiated yet (unless that instantiation already has the attribute), and warns otherwise. This is different from MSVC, which allows changing a specialization back and forth between dllimport and dllexport and seems to let the last one win. Changing the dll attribute after instantiation would be hard for us, and doesn't seem to come up in practice, so I think this is a reasonable limitation to have.

MinGW doesn't do this propagation thing.

Diff Detail

Repository
rL LLVM

Event Timeline

hans updated this revision to Diff 10771.Jun 23 2014, 4:21 PM
hans retitled this revision from to MS ABI: Propagate class-level DLL attributes to class template specialization bases (PR11170).
hans updated this object.
hans edited the test plan for this revision. (Show Details)
hans added reviewers: rnk, nrieck.
hans added a subscriber: Unknown Object (MLST).
rnk edited edge metadata.Jun 23 2014, 5:13 PM

Does your code handle explicit template specializations with inconsistent DLL attributes? I'm thinking of this test case:

template <typename T> struct Foo { T get() { return {}; } };
template <> struct __declspec(dllimport) Foo<int> { int get() { return 42; } };
struct __declspec(dllexport) Bar : Foo<int> {
  int baz() { return Foo<int>::get(); }
};
Bar b;

MSVC appears to import Foo<int>::get(), but if I understand your code correctly, it will diagnose this.

majnemer added inline comments.
lib/Sema/SemaDeclCXX.cpp
1299 ↗(On Diff #10771)

Can this function get some comments? I don't understand why it raises diagnostics when it does or why it is checking for TSK_Undeclared.

hans updated this revision to Diff 10811.Jun 24 2014, 3:38 PM
hans updated this object.
hans edited edge metadata.

New patch that takes explicit specializations and instantiations into account.

rnk accepted this revision.Jun 25 2014, 11:00 AM
rnk edited edge metadata.

lgtm

lib/Sema/SemaDeclCXX.cpp
1310 ↗(On Diff #10811)

Can you return early if getSpecializationKind() == TSK_Undeclared to reduce indentation?

1326–1328 ↗(On Diff #10811)

I think one of the 4 cases you have here is impossible. An explicitly specialized template will never have a different dll attribute, because it can only have an explicit, non-inherited attribute, in which case we won't try to change the dll attribute. What do you think of this for the first sentence:

The template was previously instantiated or explicitly specialized without a dll attribute, or the template was previously instantiated with a different inherited dll attribute.

This revision is now accepted and ready to land.Jun 25 2014, 11:00 AM
hans closed this revision.Jun 25 2014, 11:34 AM
hans updated this revision to Diff 10843.

Closed by commit rL211725 (authored by @hans).

hans added inline comments.Jun 25 2014, 11:37 AM
lib/Sema/SemaDeclCXX.cpp
1310 ↗(On Diff #10811)

Done.

1326–1328 ↗(On Diff #10811)

Sounds good to me. Done.