This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] implement options -(un)exported_symbol(s_list)
ClosedPublic

Authored by gkm on Mar 8 2021, 4:44 PM.

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rG06c4aadeb654: [lld-macho] implement options -(un)exported_symbol(s_list)
Summary

Implement command-line options to alter a dylib's exported-symbols list:

  • -exported_symbol* options override the default export list. The export list is compiled according to the command-line option(s) only.
  • -unexported_symbol* options hide otherwise public symbols.
  • -*exported_symbol PATTERN options specify a single literal or glob pattern.
  • -*exported_symbols_list FILE options specify a file containing a series of lines containing symbol literals or glob patterns. Whitespace and #-prefix comments are stripped.

Note: This is a simple implementation of the primary use case. ld64 has much more complexity surrounding interactions with other options, many of which are obscure and undocumented. We will start simple and complexity as necessary.

Diff Detail

Event Timeline

gkm created this revision.Mar 8 2021, 4:44 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: dang. · View Herald Transcript
gkm requested review of this revision.Mar 8 2021, 4:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2021, 4:44 PM
gkm planned changes to this revision.Mar 8 2021, 5:11 PM

I need to check vs. ld64 behavior before this is ready to review.

gkm updated this revision to Diff 329173.Mar 8 2021, 5:43 PM
gkm edited the summary of this revision. (Show Details)
  • Make the -export_symbol* options properly override the default public/private status of all symbols.
oontvoo added a subscriber: oontvoo.Mar 8 2021, 6:18 PM
oontvoo added inline comments.
lld/MachO/Driver.cpp
728

nit: for completeness, consider adding ] too?
(if symbolName had ] without other stuff, it's likely malformed and it would be good to error out)

gkm marked an inline comment as done.Mar 9 2021, 12:19 AM
gkm updated this revision to Diff 329231.Mar 9 2021, 12:19 AM
  • revise according to review feedback
int3 accepted this revision.Mar 9 2021, 6:18 AM
int3 added inline comments.
lld/MachO/Config.h
54–55

Hmm, GlobPattern.cpp has a special case for patterns without wildcards, but I guess having a separate literals set is still faster with the O(1) lookup

Since we're optimizing for speed here, I would use CachedHashStringRef for the literals set

lld/MachO/Driver.cpp
741

avoid auto

lld/test/MachO/export-options.s
12–13

the lack of comment character works because split-file means it isn't passed into llvm-mc, but we should still make it a comment for uniformity

17–21

I'm not sure I see much value in these macros... you're not really saving that many characters in exchange for an additional level of indirection

Without macros, you could save on the retqs by doing

_keep_globl:
_hide_globl:
_keep_private:
_show_private:
  retq

just my 2c, keep the macros if you like them

This revision is now accepted and ready to land.Mar 9 2021, 6:18 AM
gkm marked 4 inline comments as done.Mar 9 2021, 4:10 PM
gkm added inline comments.
lld/MachO/Config.h
54–55

Yes, the separate literals set is for performance. I will comment it as such.

lld/MachO/Driver.cpp
741

11 of 37 range-for loops in lld/MachO/Driver.cpp use auto. I will cleanup with a separate [NFC] diff.

lld/test/MachO/export-options.s
17–21

My primary goals were to (a) minimize vertical space, and (b) eliminate the need to type the symbol name twice--once for the scope directive, and again for the label. Placing all of the labels at the same address doesn't help me with the latter.

gkm updated this revision to Diff 329492.Mar 9 2021, 4:13 PM
gkm marked 3 inline comments as done.
  • revise according to review feedback
gkm updated this revision to Diff 329512.Mar 9 2021, 6:45 PM
  • use CachedHashStringRef for SymbolPatterns::literals
This revision was landed with ongoing or failed builds.Mar 9 2021, 6:46 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 10 2021, 6:22 AM
thakis added inline comments.
lld/MachO/SyntheticSections.cpp
611

This looks very branch and nested. How about:

static bool shouldExportSymbol(const Defined& defined) {
  if (!config->exportedSymbols.empty())
    return  config->exportedSymbols.match(defined->getName());

  return defined->privateExtern ||
            config->unexportedSymbols.match(defined->getName());
}

...

     if (!shouldExportSymbol(*defined))
       continue;
...

? This would also help with the issue below:

611

*very branchY and nested

833

You have to make a similar change here like you made in finalizeContents(), else the symbol table (what nm -m) won't match the export trie.

In general, tests that test --exports-trie output (or --bind output) should always also test nm -m output to make sure they're in sync.

gkm marked 3 inline comments as done.Mar 10 2021, 6:36 PM

@thakis, see D98381 [lld-macho] Handle error cases properly for -exported_symbol(s_list)