This is an archive of the discontinued LLVM Phabricator instance.

clang-cl: Treat dllimport explicit template instantiation definitions as declarations (PR27810, PR27811)
ClosedPublic

Authored by hans on May 24 2016, 6:06 PM.

Diff Detail

Event Timeline

hans updated this revision to Diff 58365.May 24 2016, 6:06 PM
hans retitled this revision from to clang-cl: Treat dllimport explicit template instantiation definitions as declarations (PR27810, PR27811).
hans updated this object.
hans added reviewers: rnk, thakis.
hans added a subscriber: cfe-commits.

Does this change our behavior for mingw?

hans added a comment.May 25 2016, 9:35 AM

Does this change our behavior for mingw?

No, this only affects the MS ABI.

MinGW doesn't seem to do this trick. For example, in this code, they will emit a definition for S<int>::Inner::f:

template <typename T> struct S {
  struct Inner {
    void f() {}
  };
};
template struct __declspec(dllimport) S<int>;
thakis edited edge metadata.May 25 2016, 10:21 AM

Looks great, thanks! A few minor questions below.

I verified that this has the same effect as my brute-force patch I tried locally.

Do we have test coverage for template class __declspec(dllexport) codecvt<char>; somewhere already?

lib/Sema/SemaTemplate.cpp
7382

If there are multiple dllexports and dllimports on an explicit instantiation, cl.exe just silently picks the last one?

7467

HasNoEffect is read two times before it's updated here. Is it intentional that you only change it after it's been read twice? If so, maybe add a comment why, since it looks not intentional else. (And make sure there's test coverage for setting it at the right time)

hans added a comment.May 25 2016, 11:25 AM

Thanks for the review!

Do we have test coverage for template class __declspec(dllexport) codecvt<char>; somewhere already?

Yes, that's covered by tests in CodeGenCXX/dllexport.cpp

lib/Sema/SemaTemplate.cpp
7382

The intention here was really just to reset the DLLImport value if it were set on line 7376, i.e. the attribute on the instantiation should override any attribute on the template.

I didn't even consider putting both dllimport and dllexport on the specialization. What does MSVC do? Well..

template <typename T> struct S { void f() {} };
template struct __declspec(dllimport) __declspec(dllexport) S<int>;
d:\src\tmp\a.cc(2) : warning C4141: 'dllexport' : used more than once

That diagnostic seems a little bit confused, but the effect on codegen is even more so. S<int>::f() does not get defined here, which would suggest the instantiation def is treated as an instantiation decl on account of the dllimport. On the other hand, if I reference S<int>::f, it gets defined and *exported*. We probably don't want to reproduce this.

Clang will generally let dllexport take precedence over dllimport when they're on the same declaration, and has a nice warning for it, so let's do that here too.

7467

I wanted to do this after PrevDecl_TSK has been defined, but we can move this a little earlier. I'll do that.

hans updated this revision to Diff 58461.May 25 2016, 11:25 AM
hans edited edge metadata.

tweaks

thakis accepted this revision.May 25 2016, 12:43 PM
thakis edited edge metadata.
thakis added inline comments.
lib/Sema/SemaTemplate.cpp
7382

trumps _dllimport_ here

(test?)

7448

does moving this here change behavior? if so, maybe add a test for that too.

This revision is now accepted and ready to land.May 25 2016, 12:43 PM
hans added inline comments.May 25 2016, 1:05 PM
lib/Sema/SemaTemplate.cpp
7382

Done. Adding a test to dllexport.cpp.

7448

Nope, this doesn't change anything.

hans updated this revision to Diff 58484.May 25 2016, 1:05 PM
hans edited edge metadata.

Fix the "dllexport trumps" comment, and add a test.

This revision was automatically updated to reflect the committed changes.