This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Respect DLL attributes more faithfully
ClosedPublic

Authored by smeenai on Nov 14 2016, 7:15 PM.

Details

Summary

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior for
exporting.

Event Timeline

smeenai updated this revision to Diff 77939.Nov 14 2016, 7:15 PM
smeenai retitled this revision from to [Sema] Respect DLL attributes more faithfully.
smeenai updated this object.
smeenai added reviewers: compnerd, hans.
smeenai added a subscriber: cfe-commits.
compnerd added inline comments.Nov 15 2016, 10:50 AM
lib/Sema/SemaTemplate.cpp
7666

The fragment doesn't make sense to me. Perhaps needs a bit more context?

smeenai added inline comments.Nov 15 2016, 12:44 PM
lib/Sema/SemaTemplate.cpp
7666

Do you mean the comment specifically?

hans edited edge metadata.Nov 15 2016, 5:01 PM

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ. With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

hans added a comment.Nov 16 2016, 10:01 AM
In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

Actually maybe I was being overly cautius. I think https://reviews.llvm.org/rL225570 might just make this work.

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ.

Yeah, dllimport is interesting here. If you turn on -O1, it will call the __imp one. Fun times :-)

With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

Presumably they parse and analyze the whole translation unit before emitting any code, which makes changing things around later easier for them than for us.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

Yeah, I think https://reviews.llvm.org/rL225570 makes this work actually.

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

Dont' we need dllimport to work analogously though? When using libc++, don't these template members need to be dllimported? If not, why are they being exported in the first place?

In D26657#597523, @hans wrote:
In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

Actually maybe I was being overly cautius. I think https://reviews.llvm.org/rL225570 might just make this work.

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ.

Yeah, dllimport is interesting here. If you turn on -O1, it will call the __imp one. Fun times :-)

Huh. Weird stuff.

With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

Presumably they parse and analyze the whole translation unit before emitting any code, which makes changing things around later easier for them than for us.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

Yeah, I think https://reviews.llvm.org/rL225570 makes this work actually.

So is the conclusion that this is probably safe as is, or should I limit it to dllexport specifically?

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

Dont' we need dllimport to work analogously though? When using libc++, don't these template members need to be dllimported? If not, why are they being exported in the first place?

Ah. We put dllimport on the explicit template instantiation declarations in the header (all the instances of _LIBCPP_EXTERN_TEMPLATE2 in that file). Those get paired with the dllexport on the corresponding instantiation definitions. If you're compiling the library, the declarations have no DLL attribute, and the definitions have dllexport. If you're referencing the headers from outside the library, the declarations get the dllimport.

hans added a comment.Nov 16 2016, 1:42 PM
In D26657#597523, @hans wrote:
In D26657#596759, @hans wrote:

On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match this behavior.

This is scary territory, and behaviour I think might be hard for us to match.

What if we already codegenned the implicit instantiation?

Hmm. Under what specific cases would that happen?

Actually maybe I was being overly cautius. I think https://reviews.llvm.org/rL225570 might just make this work.

I can see odd behavior differences occurring for dllimport. For example, for the C++ source in P7936, if I compile with clang (even with this patch applied), the t.f() call in g gets assembled to a call to ?f@?$s@H@@QAEHXZ. If I hoist the dllimport explicit instantiation definition above g, t.f() instead assembles to __imp_?f@?$s@H@@QAEHXZ.

Yeah, dllimport is interesting here. If you turn on -O1, it will call the __imp one. Fun times :-)

Huh. Weird stuff.

With cl 19.00.24210, t.f() always assembles to __imp_?f@?$s@H@@QAEHXZ, regardless of the placement of the explicit instantiation definition.

Presumably they parse and analyze the whole translation unit before emitting any code, which makes changing things around later easier for them than for us.

I'm having a harder time imagining things going awry for dllexport. I can limit this patch to only dllexport, if that's going to be safer.

Yeah, I think https://reviews.llvm.org/rL225570 makes this work actually.

So is the conclusion that this is probably safe as is, or should I limit it to dllexport specifically?

I think we should limit it do dllexport if we can't fix the dllimport case.

To give some context, libc++ uses extern templates extensively, and I ran into an issue with dllexport explicit locale template instantiations. Some of those instantiations are also implicitly instantiated via sizeof calls. Therefore, all instances which make is called on end up having their dllexport attribute ignored.

We could work around this in libc++ by hoisting the affected instantiations to near the top of the file, but it seemed preferable to make clang match MSVC's behavior instead, at least for dllexport. I don't have any particular interest in making dllimport semantics match MSVC for this specific case.

Dont' we need dllimport to work analogously though? When using libc++, don't these template members need to be dllimported? If not, why are they being exported in the first place?

Ah. We put dllimport on the explicit template instantiation declarations in the header (all the instances of _LIBCPP_EXTERN_TEMPLATE2 in that file). Those get paired with the dllexport on the corresponding instantiation definitions. If you're compiling the library, the declarations have no DLL attribute, and the definitions have dllexport. If you're referencing the headers from outside the library, the declarations get the dllimport.

rnk added inline comments.Nov 16 2016, 4:03 PM
lib/Sema/SemaTemplate.cpp
7667–7670

Is this the right logic in the case of Old_TSK == TSK_Implicit && TSK == TSK_Explicit? We don't want a behavior change when DLLImport is not involved. A test for that case would be good.

smeenai added inline comments.Nov 16 2016, 10:15 PM
lib/Sema/SemaTemplate.cpp
7667–7670

I'm actually concerned about the dllexport case and not the dllimport case, after my discussion with @hans. I'm updating this diff to that effect.

smeenai updated this revision to Diff 78322.Nov 16 2016, 10:19 PM
smeenai edited edge metadata.

Limiting to dllexport after discussion with @hans

majnemer added inline comments.
lib/Sema/SemaTemplate.cpp
7683–7685

Please do not do this. The canonical location for comments in LLVM are the comments themselves.

We have moved the location of the review system and I can easily believe that it will move yet again.

smeenai updated this revision to Diff 78324.Nov 16 2016, 10:48 PM

Moving explanation to comment instead of referencing Phabricator

hans added inline comments.Nov 18 2016, 2:06 PM
lib/Sema/SemaTemplate.cpp
7667–7670

I think what Reid is alluding to is regardless of dll attributes, you're adding another OR-clause to this if condition which will cause us to e.g. do Def->setTemplateSpecializationKind below, so something is changing in how we handle implicit instantiation followed by explicit instantiation def in general, not just for dllexport which might not be what we want?

smeenai added inline comments.Nov 18 2016, 2:15 PM
lib/Sema/SemaTemplate.cpp
7667–7670

That's a fair point. I'll modify this to address that.

smeenai updated this revision to Diff 78620.Nov 18 2016, 9:11 PM
smeenai updated this object.

Addressing comments

smeenai updated this revision to Diff 78621.Nov 18 2016, 9:13 PM

Typo in comment

General coding style question. Over here, I'm creating a local helper function. However, that helper needs to access member functions of Sema, which is why I made it a private member function right now, which also unfortunately makes the change somewhat non-local (since the header file needs to be modified as well). I can think of two alternatives:

  • Define a local helper lambda (which will capture this) instead of a private member function.
  • Define a local static helper function and pass the Sema instance as a parameter so that it can call member functions on it.

Would either of those be preferable to what I currently have? I'm pretty new when it comes to llvm/clang changes, so stylistic feedback is greatly appreciated.

hans added a comment.Nov 29 2016, 3:45 PM

Apologies for the delay. I was out last week.

General coding style question. Over here, I'm creating a local helper function. However, that helper needs to access member functions of Sema, which is why I made it a private member function right now, which also unfortunately makes the change somewhat non-local (since the header file needs to be modified as well). I can think of two alternatives:

  • Define a local helper lambda (which will capture this) instead of a private member function.
  • Define a local static helper function and pass the Sema instance as a parameter so that it can call member functions on it.

Would either of those be preferable to what I currently have? I'm pretty new when it comes to llvm/clang changes, so stylistic feedback is greatly appreciated.

I usually prefer a static helper function and passing the Sema instance if it's possiblem, but that requires the Sema members we need to access to be public. If that's not the case, adding the helper as a member function is fine.

include/clang/Sema/Sema.h
7494 ↗(On Diff #78621)

Nit: I think /// implies \brief, so we don't usually include it.

7495 ↗(On Diff #78621)

I'd suggest making the function name more specific. It obviously doesn't apply to all DLL attributes, but only class templates.

Also, the "ActOn" methods in Sema are used as an interface to be called by the parser, so I would avoid that prefix.

Naming is hard :-/

Maybe checkDllExportedTemplateSpecialization

lib/Sema/SemaTemplate.cpp
7439

This function only applies to dllexport, not dllimport, so it would be good if the name reflected that, and maybe we could also add an assert to check for it.

7712

Why the isWindowsItaniumEnvironment check? I'd expect checking for the MS ABI is sufficient?

hans added inline comments.Nov 29 2016, 3:48 PM
include/clang/Sema/Sema.h
7495 ↗(On Diff #78621)

also it should be in the private: section since it's just a helper. And maybe the "check" part is unnecessary? dllExportClassTemplateSpecialization might work.

smeenai added inline comments.Nov 29 2016, 9:38 PM
include/clang/Sema/Sema.h
7494 ↗(On Diff #78621)

Cool, will drop.

7495 ↗(On Diff #78621)

Good point about the prefix. I'll do the rename. I intended to make it private; if it's in the wrong part of the header, that's an accident on my part :)

lib/Sema/SemaTemplate.cpp
7439

It's called in two places (the refactored original call for explicit instantiation declaration followed by explicit instantiation definition, and my new call for implicit instantiation followed by explicit instantiation definition). The dllexport guarantee only applies to the second one, right? I'll come up with a better name based on your suggestions in the other comment.

7712

windows-itanium in general tries to stick to MSVC semantics for dllexport/import annotations (unlike Cygwin and MinGW which kinda do their own thing). This is consistent with the conditional for the previous case (lines 7691 to 7693 in this diff).

hans added inline comments.Nov 30 2016, 10:24 AM
lib/Sema/SemaTemplate.cpp
7439

You're right, my mistake.

7712

Oh I see, this seems to be a new thing, starting with e.g. r284288.

Seems fine then, but I'm a little worried that we're adding another variable into the matrix here. IIRC, we key dll attribute behaviour off getCXXABI().isMicrosoft() in lots of places.

smeenai updated this revision to Diff 80193.Dec 3 2016, 4:31 PM

Addressing comments

lib/Sema/SemaTemplate.cpp
7712

You're right; the current situation isn't ideal. I can clean that up in a follow-up.

hans accepted this revision.Dec 5 2016, 9:12 AM
hans edited edge metadata.

lgtm

lib/Sema/SemaTemplate.cpp
7712

Sounds good.

This revision is now accepted and ready to land.Dec 5 2016, 9:12 AM
This revision was automatically updated to reflect the committed changes.