This is an archive of the discontinued LLVM Phabricator instance.

[2/3] [LLD] [COFF] Add support for a new, mingw specific embedded directive -exclude-symbols:
ClosedPublic

Authored by mstorsjo on Jul 19 2022, 1:22 PM.

Details

Summary

This is an entirely new embedded directive - extending the GNU ld
command line option --exclude-symbols to be usable in embedded
directives too.

(GNU ld.bfd doesn't support this option as an embedded directive yet,
but when given an object file with such a directive, GNU ld doesn't
error out - contrary to lld - but only warns about it. I'll send a
patch to binutils to add support for the same option there too; this
patch pretty much needs to be coordinated with whatever review feedback
it gets there too.)

This works as an inverse to the regular embedded dllexport directives,
for cases when autoexport of all eligible symbols is performed.

Naming wise, the plural form "exclude-symbols" feels a bit odd here
as it would probably only ever be used with one symbol at a time (I'm
only adding testcases for one symbol per option here), but I'd rather
have it named the same as the command line option, rather than vaguely
different.

Diff Detail

Event Timeline

mstorsjo created this revision.Jul 19 2022, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 1:22 PM
mstorsjo requested review of this revision.Jul 19 2022, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 1:22 PM

The corresponding binutils patch is up for review at https://sourceware.org/pipermail/binutils/2022-July/121902.html.

mati865 accepted this revision.Jul 20 2022, 8:43 AM

Even though the changes are small I not sure if approval from an outsider (me) should suffice here.

lld/test/COFF/exclude-symbols-embedded.s
25–26

Maybe one of these lines should list multiple symbols to cover that case?

This revision is now accepted and ready to land.Jul 20 2022, 8:43 AM
mstorsjo added inline comments.Jul 22 2022, 4:20 AM
lld/test/COFF/exclude-symbols-embedded.s
25–26

Sure, I can extend the test to cover that too. I was a little on the fence regarding whether we should allow multiple symbols in the object files (there’s probably little reason to ever use it), but the binutils review seems to point towards keeping that aspect too, so let’s formalize it in the test.

mstorsjo updated this revision to Diff 446784.Jul 22 2022, 4:35 AM

Test multiple symbols with one directive

Even though the changes are small I not sure if approval from an outsider (me) should suffice here.

Ping @rnk - what do you think of this feature? (I’ll try to sync this patch with the same feature for binutils, but I want some more eyes on the concept itself.)

FWIW this feature has now been OK'd and applied on the binutils side - which should make this patch a bit simpler to reason about.

rnk accepted this revision.Aug 9 2022, 10:27 AM

I think it's a reasonable feature.

lld/COFF/Driver.h
54–57

It's not great that "excludes" is literally the opposite of "includes" when it is meant to be the opposite of exports, but of course we should use the name that ld.bfd went with.

mati865 accepted this revision.Aug 9 2022, 11:07 AM
mstorsjo added inline comments.Aug 9 2022, 1:03 PM
lld/COFF/Driver.h
54–57

Yeah - also the directive name exclude-symbols feels a bit odd to have as plural, but I preferred to keep it matching the the existing command line option (existing in ld.bfd).

MaskRay accepted this revision.Aug 9 2022, 5:51 PM
MaskRay added a subscriber: MaskRay.
MaskRay added inline comments.
lld/COFF/Driver.h
163

Prefer llvm::DenseSet<StringRef>.

StringSet<> stores strings and is therefore more expensive. The values are backed by saver().

lld/test/COFF/exclude-symbols-embedded.s
6

See my comment in another patch that grep Name: can be removed.

mstorsjo updated this revision to Diff 451430.Aug 10 2022, 5:46 AM

Simplify the test with --implicit-check-not=Name:, use DenseSet<StringRef> as suggested.

MaskRay accepted this revision.Aug 10 2022, 1:28 PM
This revision was landed with ongoing or failed builds.Aug 11 2022, 2:01 AM
This revision was automatically updated to reflect the committed changes.