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

[Rafael Avila de Espindola via llvm-commits]

Peter Collingbourne via Phabricator <reviews at reviews.llvm.org> writes:

> pcc created this revision.
> Herald added a subscriber: mehdi_amini.
>
> 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.

But that directive is a list, so in this regard they seem equivalent.

In PR32334 I see:

------------------------------------------------------------
In COFF, exports are identified by a list of /export flags in the
.drective section, so when a comdat section is discarded, we don't lose
the information that the symbol might be exported.
-----------------------------------------------------------------

Which is the case where the two representations are different. Even if
we drop the last IR @foo, we still need to remember that @foo is to be
exported.

> 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
> https://reviews.llvm.org/D20217).

I agree. dllexport is almost redundant. Using ELF terminology we can say
that COFF systems just default to hidden visibility and have the option
of using protected when they want to export a symbol. The analogy is not
exact in that ELF visibilities are merged by taking the minimum.

> 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.

I would love to use dllimport on ELF to mean "assume this symbol is
external regardless of -fPIC".

> 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.

I love this change :-)


Cheers,
Rafael