Page MenuHomePhabricator

[wip] IR: Move linker options to top-level global metadata and remove dllexport storage class.
AbandonedPublic

Authored by pcc on Mar 22 2017, 4:43 PM.

Details

Summary

dllexport is not actually a global-level attribute. At the object file
level, it is in fact a module-level attribute and is represented with an
/export flag in the directive section. Trying to shoehorn it into being a
global-level attribute creates unnecessary complexity and a burden on the
rest of the compiler and invites bugs.

Independent of whether it is a global- or module-level attribute, it makes
sense to split dllimport from dllexport, as they are somewhat orthogonal. The
meaning of dllexport is similar to ELF symbol visibility, while the meaning
of dllimport is closer to preemptability (see also D20217). With this,
dllimport can potentially receive a meaning in other object formats without
being in the confusing situation where dllexport only has meaning for COFF
and dllimport also has meaning elsewhere.

This patch does three things:

  • Moves the linker options from a module flag to top-level module metadata. Storing it as a module flag only makes it harder to manipulate the linker options.
  • Causes clang to add an /export flag to the linker options metadata if it is emitting a definition that will be dllexported.
  • Removes the dllexport storage class and stores dllimport as a boolean.

Test cases to be updated.

Fixes PR32334.

Event Timeline

pcc created this revision.Mar 22 2017, 4:43 PM
rnk edited edge metadata.Mar 23 2017, 10:26 AM
rnk added subscribers: majnemer, chandlerc.

I'm opposed to getting rid of the dllexport bit on GlobalValue, and so far on the bug, both @majnemer and @rjmccall seem to be opposed to it as well. I also asked @chandlerc about it in person, and he wasn't in favor, although I'm not sure how strong his opinion was.

This change simplifies the job of the COFF linker because it makes the IR more like a COFF object, so I see why it's appealing. I want to rehash what I was saying about LLVM being more than just an object file format, though. LLVM is supposed to be a library. This stands in opposition to the design philosophy of LLD ELF & COFF. LLVM IR is supposed to be an intermediate representation useful for a wide variety of use cases: optimization, symbolic execution, instrumentation, and things we haven't thought of. Maintaining exports in a separate linker flag list makes it hard for these tools to determine which things are exported. Obviously, such tools can never know the true list of exports until link time, but I argue that it is useful for tools to know if *some* things are exported and have workarounds for the rest. You could argue that this is inviting bugs and such a tool needs link time information to be robust, but I'd argue that in practice it's a lot easier to do things at compile time than link time.

As a minor concern, the new representation is less efficient. We would now store mangled symbol names in one more place than we do now before, which might be significant. It seems nice to delay construction of the .drective section until after memory used by ISel has been freed.

Lastly, this is a minor bug in /msvclto, a feature that is only intended to exist until LLD can write PDBs. I feel like it just doesn't justify a representation change. Yes, LLVM IR is not cast in stone and of course should be changed to fix bugs and add features, but there is still a small cost to changing it.


I am in favor of moving "Linker Options" to "llvm.linker.options" if you want to do that separately.

pcc added a comment.Mar 23 2017, 10:41 AM

Maintaining exports in a separate linker flag list makes it hard for these tools to determine which things are exported.

Only barely: such a (hypothetical) tool can easily enough parse the linker options list.

Lastly, this is a minor bug in /msvclto, a feature that is only intended to exist until LLD can write PDBs. I feel like it just doesn't justify a representation change.

The alternatives that were discussed on the bug were also representational changes, and imposed new burdens on passes. That seems less justifiable for the sake of the /msvclto feature.

pcc added a comment.Mar 23 2017, 1:04 PM

It occurred to me that this bug would also affect ThinLTO distributed builds if they work in the same way as they do with gold. The scenario is pretty much the same: distributed backend processes perform LTO and create native object files that are fed into a separate linker process that performs the final link.

rjmccall edited edge metadata.Mar 24 2017, 9:32 AM

This is a very disruptive change — it completely changes the IR model for dllexport, thus breaking every single frontend that targets Windows. LLVM should care about the impact on out-of-tree frontends. And I'm not worried about Swift here, because we've got a very motivated community; I'm worried about the dozens of other random language frontends that maintain a Windows port.

Furthermore, by your account, this isn't even the representation we want to end up with. In the long term, we should completely fold dllimport and dllexport into symbol visibility. So you're talking about breaking every Windows frontend twice, for some corner case that doesn't really matter to the vast majority of frontends.

I'm willing to accept your argument that attaching extra meaning to declarations is also disruptive. Fine. The obvious compromise position that doesn't involve completely changing how every Windows frontend works is to take your global metadata idea and have it be an opt-in feature usable on an arbitrary symbol name. But right now, you are proposing a breaking change to IR that will make extra work for a ton of frontend authors, purely for the implementation convenience of the LTO linker. That is not an appropriate trade-off.

pcc added a comment.Mar 24 2017, 10:50 AM

Furthermore, by your account, this isn't even the representation we want to end up with. In the long term, we should completely fold dllimport and dllexport into symbol visibility.

That isn't really my position though. dllexport is similar to symbol visibility, but does not have the same merging semantics (see discussion with Rafael). dllimport can be seen as orthogonal to visibility, but I imagine that one possible alternative to D20217 would be to create a visibility that means "default but known DSO-local". So I can understand your perspective with regard to dllimport at least.

The obvious compromise position that doesn't involve completely changing how every Windows frontend works is to take your global metadata idea and have it be an opt-in feature usable on an arbitrary symbol name.

I think that would at least move us in the right direction. I will try changing the IRMover to create metadata for each dllexport symbol that is not linked into the target module.

In D31270#710079, @pcc wrote:

Furthermore, by your account, this isn't even the representation we want to end up with. In the long term, we should completely fold dllimport and dllexport into symbol visibility.

That isn't really my position though. dllexport is similar to symbol visibility, but does not have the same merging semantics (see discussion with Rafael).

It doesn't have the same merging semantics *as ELF*. Mach-O uses the maximum visibility, the same as dllexport.

dllimport can be seen as orthogonal to visibility, but I imagine that one possible alternative to D20217 would be to create a visibility that means "default but known DSO-local". So I can understand your perspective with regard to dllimport at least.

FWIW, you can hack around the lack of that visibility today by putting "hidden" on declarations and "default" on definitions. Of course, this trick only works on strong definitions, and it relies on the IR linker discarding declarations without merging visibility, but it does work. It would be better to have that explicit, though.

pcc abandoned this revision.Mar 24 2017, 1:45 PM

Sent D31349 and D31352 which implement what we decided on here.