This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Add support for exporting no symbols
ClosedPublic

Authored by keith on Jun 10 2022, 11:51 PM.

Details

Reviewers
int3
oontvoo
Group Reviewers
Restricted Project
Commits
rG272bf0fc41e6: [lld-macho] Add support for exporting no symbols
Summary

As an optimization for ld64 sometimes it can be useful to not export any
symbols for top level binaries that don't need any exports, to do this
you can pass -exported_symbols_list /dev/null, or new with Xcode 14
(ld64 816) there is a -no_exported_symbols flag for the same behavior.
This reproduces this behavior where previously an empty exported symbols
list file would have been ignored.

Diff Detail

Event Timeline

keith created this revision.Jun 10 2022, 11:51 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 10 2022, 11:51 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
keith requested review of this revision.Jun 10 2022, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2022, 11:51 PM
int3 added a subscriber: int3.Jun 12 2022, 6:59 AM
int3 added inline comments.
lld/MachO/Driver.cpp
1412–1413

tangential & existing code, but this seems kind of redundant to me lol. We're already erroring out, doesn't seem like there's a need to tell the user we're ignoring unexports. (Not sure there's a need to clear the unexported symbols list either, but I suppose it might make any subsequent error messages less confusing.)

1416

hmm should we make it an error to use -no_exported_symbols with -exported_symbol as well?

1561

IMO this check is a bit confusing because it conflates a perf optimization with an actual difference in behavior. How about having the Config be something like

struct Config {
  ...
  bool explicitExports = false;
  SymbolPatterns exportPatterns; // (can represent both `exportedSymbols` and `unexportedSymbols`)
};

With exportPatterns interpreted differently based on the value of explicitExports. Then this if check becomes if (config->explicitExports) { ...

That said I know this is existing code, and refactoring it can be rightly said to be out of scope :)

thakis added a subscriber: thakis.Jun 12 2022, 10:26 AM
thakis added inline comments.
lld/MachO/Driver.cpp
1413

(very nit: I find this name a bit hard to read, since empty is also a verb and this reads like "should I empty the exported symbols list" to me)

oontvoo added inline comments.
lld/MachO/Driver.cpp
1561

agreed - perhaps get rid of the two unexportedSymbols and exportedSymbols lists and have just one list then a flag to say what the list means?

keith updated this revision to Diff 436452.Jun 13 2022, 9:49 AM
keith marked 3 inline comments as done.

Add explicit config for exported symbols options

keith added inline comments.Jun 13 2022, 9:54 AM
lld/MachO/Driver.cpp
1412–1413

Removed this, it was silently ignored anyways because of the order of the if statement below. If we find a case where error messages are degraded without it I guess we should add it back with a test showing that case.

1561

So I did the first suggestion here with the explicit exports, I'm not sure if it would be as clear if we joined exportedSymbols and unexportedSymbols but if folks generally prefer that I can change it!

int3 accepted this revision.Jun 14 2022, 2:48 AM

Thanks!

lld/MachO/Driver.cpp
1412–1413

nice. can we remove the ">>> ignoring unexports" string as well?

1412–1413

no need for { } here and line 1416 below

This revision is now accepted and ready to land.Jun 14 2022, 2:48 AM
oontvoo accepted this revision.Jun 14 2022, 6:34 AM

👍

keith updated this revision to Diff 437331.Jun 15 2022, 2:04 PM
keith marked 2 inline comments as done.

Formatting improvements

This revision was landed with ongoing or failed builds.Jun 15 2022, 3:07 PM
This revision was automatically updated to reflect the committed changes.