Page MenuHomePhabricator

[lld-macho] Add support for N_INDR symbols
ClosedPublic

Authored by int3 on Sep 13 2022, 6:12 PM.

Details

Summary

This is similar to the -alias CLI option, but it gives finer-grained
control in that it allows the aliased symbols to be treated as private
externs.

While working on this, I realized that our -alias handling did not
cover the cases where the aliased symbol is a common or dylib symbol,
nor the case where we have an undefined that gets treated specially and
converted to a defined later on. My N_INDR handling neglects this too
for now; I've added checks and TODO messages for these.

N_INDR symbols cropped up as part of our attempt to link swift-stdlib.

Diff Detail

Event Timeline

int3 created this revision.Sep 13 2022, 6:12 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
int3 requested review of this revision.Sep 13 2022, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 6:12 PM
int3 added inline comments.Sep 13 2022, 6:15 PM
lld/MachO/Driver.cpp
1204

I very mildly prefer not using continue to break out of just one level of nesting, but happy to change it back if anyone prefers the original

int3 updated this revision to Diff 459947.Sep 13 2022, 6:23 PM

tweak comment

int3 edited the summary of this revision. (Show Details)Sep 13 2022, 6:34 PM
thakis accepted this revision.Sep 13 2022, 7:50 PM
thakis added a subscriber: thakis.
thakis added inline comments.
lld/MachO/Driver.cpp
1216

This feels a bit inelegant. Conceptually, an alias and its target could be two map entries pointing at the same symbol – if it weren't for the fact that the alias can have more restrictive visibility :/

lld/MachO/SymbolTable.h
46

nit: isPrivateExtern for consistency with previous method?

lld/test/MachO/symbol-aliases.s
43 ↗(On Diff #459947)

Do you have a test for an extern alias to a local or a private extern target?

This revision is now accepted and ready to land.Sep 13 2022, 7:50 PM

Oh, maybe worth adding an alias to the dead stripping test (to make sure alias insertion happens before dead stripping).

thevinster accepted this revision.Sep 14 2022, 1:54 AM
thevinster added a subscriber: thevinster.

Overall, lgtm. Thanks!

lld/MachO/Driver.cpp
1216

If we have an object file that uses aliases and we also specify -alias from the command line, the object file alias will replace the one from the command line. Conceptually speaking, should it be the other way around? I haven't tested this explicitly but am interested to know this result.

lld/MachO/SymbolTable.cpp
120

Does this mean that both the alias and aliased symbol can differ in symbol visibility? If so, might be worth adding a test for it if it's expected behavior.

int3 marked an inline comment as done.Sep 14 2022, 2:24 AM
int3 added inline comments.
lld/MachO/Driver.cpp
1216

agreed heh

1216

ooh, good question. let me add that test case

lld/MachO/SymbolTable.cpp
120

I'm not sure I understand the question. Are you asking if they can have visibilities that differ from each other? Yes, but that is already being tested...

lld/MachO/SymbolTable.h
46

sure. I was thinking that isPrivateExtern sounded like it could apply to either the src or target, whereas makePrivateExtern is more clearly something that will be applied to the newly-created symbol. But consistency is good too

lld/test/MachO/symbol-aliases.s
43 ↗(On Diff #459947)

I'll add a test for a private extern target. AFAICT, MC won't generate aliases to locals (it will just refer to the aliased symbol itself), so that case can't / doesn't need to be tested.

thevinster added inline comments.Sep 14 2022, 11:41 AM
lld/MachO/SymbolTable.cpp
120

Whoops, must've missed that. I guess it would be nice to also test a global alias to a private extern symbol too just to make sure we test all combinations. But I see, thakis@ already commented that as well.

int3 marked an inline comment as done.EditedSep 14 2022, 12:32 PM

So, as per Vincent's suggestion, I wrote some tests to see if/when aliases could overwrite other aliases. And I realize that I had gotten it wrong... in fact, alias symbols are always treated as strongly-defined symbols, even if the aliased symbol is weak. I.e. if we have two definitions of _a, either one (or both) an alias to a weak def _b, we will still encounter a dup symbol error for the two _as.

That said, I'm not sure the implementation complexity of emulating this behavior is worth it... we would essentially have to put AliasSymbols into the global SymbolTable. Which means that every addDefined, addLazy etc call will now have to correctly handling the AliasSymbol case. *Or* we could, instead of creating a separate AliasSymbol class, instead make Defineds able to represent aliases. We could have a Defined::isAlias bit, and use that to make a tagged union out of Defined::isec; that same slot can be used to point to a StringRef of the aliased name.

The former is tedious and the latter is frankly kind of ugly. Given that we are merely being more permissible than ld64 with our current "aliases can override" behavior, I'm not sure this extra work is worth it -- any alias-using programs linkable with ld64 should be linkable with LLD under the current semantics.

If @thakis @oontvoo etc don't mind I'm just going to document this as another behavior difference in our docs.

lld/test/MachO/symbol-aliases.s
24–31 ↗(On Diff #459947)

Oh, maybe worth adding an alias to the dead stripping test (to make sure alias insertion happens before dead stripping).

@thakis I think these lines already cover that case

73 ↗(On Diff #459947)

I messed up here. I thought that this second definition of _extern_alias_to_weak was demonstrating that we could have two separate aliases of the same name w/o a dup symbol error, but actually MC doesn't even generate an alias at all here since _weak is defined in the same file -- references to _extern_alias_to_weak are automatically emitted as references to _weak

int3 added a subscriber: oontvoo.Sep 14 2022, 12:36 PM

(hit enter too early last message, please see final edits if you got a truncated comment via email)

int3 added a comment.Sep 14 2022, 4:15 PM

I'm just going to document this as another behavior difference in our docs.

This is done. Will wait a bit for feedback before landing :)

int3 marked an inline comment as done.Sep 14 2022, 4:15 PM

If @thakis @oontvoo etc don't mind I'm just going to document this as another behavior difference in our docs.

I don't have an opinion on this. Seems fine.

This revision was landed with ongoing or failed builds.Sep 15 2022, 5:35 AM
This revision was automatically updated to reflect the committed changes.

As far as I can tell, this breaks tests everywhere (eg http://45.33.8.238/linux/86581/step_11.txt)

Please take a look and revert for now if it takes a while to fix.

(Also, please run tests locally before commiting)

int3 added a comment.Sep 15 2022, 11:27 AM

Sorry, I do run tests locally, just not always with assertions enabled...

I'm not sure I understand the purpose of the assertion tbh -- @thakis you introduced it in D102076. We're triggering it now because alias symbols can be weak + can be defined with a different InputFile than the section they ultimately belong to. Can we just remove the assert?

int3 added a comment.Sep 15 2022, 7:12 PM

I don't think there's a way to preserve the intent of the assert without adding another parameter to addDefined to indicate whether we are creating an alias or not. Will remove the assert