This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Support -non_global_symbols_strip_list, -non_global_symbols_no_strip_list, -x
ClosedPublic

Authored by oontvoo on May 20 2022, 12:46 AM.

Details

Reviewers
thevinster
Group Reviewers
Restricted Project
Commits
rGfae6bd7563ca: [lld-macho] Support -non_global_symbols_strip_list…
Summary

PR/55600

Diff Detail

Event Timeline

oontvoo created this revision.May 20 2022, 12:46 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 20 2022, 12:46 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.May 20 2022, 12:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 12:46 AM
oontvoo updated this revision to Diff 430917.May 20 2022, 2:09 AM

more tests

oontvoo updated this revision to Diff 430950.May 20 2022, 5:59 AM

add tests for interaction with -exported_symbol

MaskRay added inline comments.
lld/MachO/Config.h
97

This is for local symbols so the name can be more specific. One idea is

enum class LocalSymbolPolicy {
  All,
  None,
  IncludeSelected,
  ExcludeSelected,
};
lld/MachO/Driver.cpp
1443

FWIW ld.lld uses error: ... and ... may not be used together

lld/MachO/SyntheticSections.cpp
708

Drop formatting

966

It seems simpler to use bool(Symbol *) and let the for loop call addSymbol(localSymbols, sym);

oontvoo marked an inline comment as done.May 21 2022, 4:38 PM
oontvoo added inline comments.
lld/MachO/Driver.cpp
1443

why?

This was worded this way to be consistent with the error message w.r.t exported_symbol (right a few lines above) AND LD64's error messages.

lld/MachO/SyntheticSections.cpp
966

I dont think that'd be simpler because all the callers would have to repeat the same check.

oontvoo updated this revision to Diff 431186.May 21 2022, 4:41 PM

undo formatting changes

hi, friendly 🔔 :)

oontvoo updated this revision to Diff 431525.May 23 2022, 4:49 PM

consolidate exclude/included patterns set into one

thevinster added inline comments.
lld/MachO/Driver.cpp
1421

nit: variable name seems clearer if it's named includeLocal/excludeLocal.

lld/MachO/SyntheticSections.cpp
971

Does this case need to be enumerated if it's already being filtered out below?

lld/test/MachO/local-symbol-output.s
74

Can we also have a test for interactions with -unexported_symbol as well? Specifically, that a global doesn't get included in the symbol table?

oontvoo updated this revision to Diff 431641.May 24 2022, 4:22 AM
oontvoo marked 2 inline comments as done.

rename variables per suggestion

lld/MachO/SyntheticSections.cpp
971

Does this case need to be enumerated if it's already being filtered out below?

Yes, because we don't need to do the check on line 1009 and line 1019. We only do that check on line 991+ is because iterating through all the inputfiles and their symbols is expensive - hence if we can avoid it (ie. when -x is set), then we should

lld/test/MachO/local-symbol-output.s
74

I'm not sure I understand the test you're proposing. Can you give an example?
Specifically, -x and -non_global_symbols_[no_]strip_list options don't have an effect on global symbols. So how would using them with -unexported_symbol be meaningful?

thevinster added inline comments.May 24 2022, 11:45 AM
lld/MachO/Driver.cpp
1416–1417

Just to check here, does ld64 also do the same here? Wondering if this specific to our implementation or inheriting behavior for ld64. I do agree it's confusing and maybe we should disallow it if no one actually does this in practice.

lld/MachO/SyntheticSections.cpp
971

Whoops missed that part. Thanks for clarifying

lld/test/MachO/local-symbol-output.s
74

Right, -x and -non_global_symbols_[no_]strip_list don't matter to global symbols but unexported symbols can make them non-global. For example,

.globl _foo
.globl _bar
_foo:
  ret
_bar:
  ret

What I'm thinking is if _foo in the test case above is unexported with -x, I would to expect to see only _bar in the symbol table since -x would've stripped away _foo.

We can also apply this to -non_global_symbols_strip_list and non_global_symbols_no_strip_list well by checking that the unexported symbol should've been stripped and not stripped from the symbol table, respectively.

oontvoo marked an inline comment as done.May 24 2022, 5:55 PM
oontvoo added inline comments.
lld/MachO/Driver.cpp
1416–1417

Yes, that's from LD64. People may not do it on purpose but perhaps generated link commands do this.

I'd slightly prefer we error-ing out for simplicity but given the behaviour is not too complex to imitate, I don't feel strongly about it, hence the TODO note.

Does anyone else have a strong opinion on this? Otherwise, happy to just delete the TODO and be done with this edge case :)

lld/test/MachO/local-symbol-output.s
74

added tests

oontvoo updated this revision to Diff 431849.May 24 2022, 5:55 PM
oontvoo marked an inline comment as done.

added additional tests

This revision is now accepted and ready to land.May 25 2022, 1:45 AM
This revision was landed with ongoing or failed builds.May 25 2022, 5:27 AM
This revision was automatically updated to reflect the committed changes.
int3 added a subscriber: int3.May 31 2022, 2:47 PM

Sorry for the belated review. Took me a little longer than expected to get back in the swing of things :)

lld/MachO/Driver.cpp
941

I would prefer to avoid naming things "helper" if there is a more descriptive name available. E.g. something like parseSymbolPatternsFile seems like a good fit here

956

nit: insert line break before functions for easy vim { } movement :p

1418–1419

did you have a build that used both together? IMO we should prefer implementation simplicity by default in cases where ld64's behavior is questionable

1429

extra newline

lld/MachO/SyntheticSections.cpp
971

seems worthwhile to explain this in a comment (see below)

990

could state explicitly that localSymbolsHandler will do the right thing regardless, but that this is a perf optimization

1018–1019

I think we lack a test case for this private extern code path

oontvoo marked 5 inline comments as done.Jun 1 2022, 9:57 AM
lld/MachO/Driver.cpp
1418–1419

Yeah - can we keep this for now? (I'll find out from the builds' owner whether the usage was intentional or if it could be cleaned up).