Page MenuHomePhabricator

[lld-macho] Set FinalDefinitionInLinkageUnit on most LTO externs
ClosedPublic

Authored by int3 on Feb 10 2022, 5:50 PM.

Details

Summary

Since Mach-O has a two-level namespace (unlike ELF), we can usually set
this property to true.

(I believe this setting is only available in the new LTO backend, so I
can't really use ld64 / libLTO's behavior as a reference here... I'm
just doing what I think is correct.)

See D119294: [lld-macho] -flat_namespace for dylibs should make all externs interposable for the work done to calculate the interposable used in
this diff.

Diff Detail

Event Timeline

int3 created this revision.Feb 10 2022, 5:50 PM
int3 requested review of this revision.Feb 10 2022, 5:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2022, 5:50 PM
modimo added a subscriber: modimo.Mar 9 2022, 1:05 PM

Looking through the ELF test cases and changes:

  1. ELF/lto/internalize-basic.ll and ELF/lto/internalize-exportdyn.ll are simple test cases that should work identically on MachO to test this case.
  2. For the ELF definition of FinalDefinitionaInLinkageUnit:
r.FinalDefinitionInLinkageUnit =
    (isExec || sym->visibility != STV_DEFAULT) && dr &&
    // Skip absolute symbols from ELF objects, otherwise PC-rel relocations
    // will be generated by for them, triggering linker errors.
    // Symbol section is always null for bitcode symbols, hence the check
    // for isElf(). Skip linker script defined symbols as well: they have
    // no File defined.
    !(dr->section == nullptr && (!sym->file || sym->file->isElf()));
  • isExec maps towards the output file type. Looking at MachO/driver.cpp it seems the current supported types are MH_BUNDLE, MH_DYLIB and MH_EXECUTE. MH_EXECUTE looks to be the equivalent check.
  • sym->visibility != STV_DEFAULT all non-STV_DEFAULT symbols cannot be pre-empted and so are definitely dso_local. Removing this condition in the OR and running ninja check-all doesn't show any failure so it's not tested. I think the intent here is to capture cases where in .so and .dylib's some symbols cannot be overriden externally and so can be made dso_local. Skimming through how visibility is done on MachO I see the n_type of N_PEXT is non-preemptible visibility and maps to the isExternal property of the symbol.
  • dr checks if its a Defined which is already done here, so nothing too interesting.
  • !(dr->section == nullptr && (!sym->file || sym->file->isElf())) is purely to deal with cases like in the test test/ELF/lto/abs-resol.ll (D42977 + D43051). That test looks like it ports over well to MachO, though the second half using linker script may not exist yet

For the current set of checks:

  1. !defined->isExternalWeakDef() do these exist in MH_EXECUTE filetype assuming that's the check we go for? If so that logic will fold into the MH_EXECUTE check without specifically calling out this case
  2. !defined->interposable similar to above, does flat_namespace work on MH_EXECUTE filetypes?

Taken together, I see the check looking like:

r.FinalDefinitionInLinkageUnit = (config->outputType == MH_EXECUTE || !defined->isExternal()) \\ Check for visibility
                                 && !(defined->section == nullptr && (!sym->file || sym->file->Kind() == InputFile::Kind::ObjKind)) \\ Check to see if the symbol is defined in object file
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 1:05 PM
int3 planned changes to this revision.Mar 9 2022, 8:44 PM
int3 added a comment.Mar 10 2022, 4:37 PM

ELF/lto/internalize-basic.ll and ELF/lto/internalize-exportdyn.ll are simple test cases that should work identically on MachO to test this case.

The cases in internalize-basic.ll are covered by our lto-internalize.ll. internalize-exportdyn.ll is mostly covered by our lto-internalize-unnamed-addr.ll. (We use dylibs instead of executables linked with the -export_dynamic, but the code paths tested are basically identical, since that flag makes executable symbols get treated like dylib ones.) I did notice two missing cases, however: we missed out testing the case where a symbol is sometimes linkonce_odr and sometimes weak_odr, and we were only checking the visibility of the symbols in the final output binary, whereas the ELF tests were checking the visibility of the symbols at the IR level (after the internalize stage of LTO). I've put up D121428: [lld-macho] Extend lto-internalize-unnamed-addr.ll to cover those cases.

isExec maps towards the output file type. Looking at MachO/driver.cpp it seems the current supported types are MH_BUNDLE, MH_DYLIB and MH_EXECUTE. MH_EXECUTE looks to be the equivalent check.

Yup. This is covered by the initialization of interposable.

sym->visibility != STV_DEFAULT all non-STV_DEFAULT symbols cannot be pre-empted and so are definitely dso_local. Removing this condition in the OR and running ninja check-all doesn't show any failure so it's not tested. I think the intent here is to capture cases where in .so and .dylib's some symbols cannot be overriden externally and so can be made dso_local. Skimming through how visibility is done on MachO I see the n_type of N_PEXT is non-preemptible visibility and maps to the isExternal property of the symbol.

I think Mach-O can be stricter here due to the two-level namespace (which is again covered by the initialization of interposable). We should therefore be able to apply dso_local to non-weak externs too. I think we just have to exclude weak externs since they are effectively handled as flat namespace lookups at runtime. (But maybe we can still hide weak externs that also satisfy canBeOmittedFromSymbolTable()?)

!(dr->section == nullptr && (!sym->file || sym->file->isElf())) is purely to deal with cases like in the test test/ELF/lto/abs-resol.ll (D42977 + D43051). That test looks like it ports over well to MachO, though the second half using linker script may not exist yet

Mach-O doesn't have linker scripts :) But regardless I don't think the bug that the check is working around applies to the Mach-O codepaths. At least, I'm not seeing an error when running LLD (with this diff applied) against the equivalent absolute symbol test: https://gist.github.com/int3/d25a9cb7e1a12084fe1348ed8bee146f. I don't think we need to include a port of the abs-resol.ll test since it seems like an ELF-specific regression check.

int3 requested review of this revision.Mar 10 2022, 4:57 PM
int3 added a subscriber: MaskRay.

But maybe we can still hide weak externs that also satisfy canBeOmittedFromSymbolTable()

Oh, we're actually doing that already. isExternalWeakDef() returns false if privateExtern is true, and the latter is false if canBeOmittedFromSymbolTable() is false.

Surprisingly, however, despite us marking FinalDefinitionInLinkageUnit = false, the LTO backend doesn't seem to add dso_local to these symbols. OTOH the ELF internalize-exportdyn.ll test shows that dso_local gets added for the equivalent inputs. (cc @MaskRay for the dso_local stuff)

Re-requesting review since I don't think any changes are needed for now

ELF/lto/internalize-basic.ll and ELF/lto/internalize-exportdyn.ll are simple test cases that should work identically on MachO to test this case.

The cases in internalize-basic.ll are covered by our lto-internalize.ll. internalize-exportdyn.ll is mostly covered by our lto-internalize-unnamed-addr.ll. (We use dylibs instead of executables linked with the -export_dynamic, but the code paths tested are basically identical, since that flag makes executable symbols get treated like dylib ones.) I did notice two missing cases, however: we missed out testing the case where a symbol is sometimes linkonce_odr and sometimes weak_odr, and we were only checking the visibility of the symbols in the final output binary, whereas the ELF tests were checking the visibility of the symbols at the IR level (after the internalize stage of LTO). I've put up D121428: [lld-macho] Extend lto-internalize-unnamed-addr.ll to cover those cases.

👍

isExec maps towards the output file type. Looking at MachO/driver.cpp it seems the current supported types are MH_BUNDLE, MH_DYLIB and MH_EXECUTE. MH_EXECUTE looks to be the equivalent check.

Yup. This is covered by the initialization of interposable.

sym->visibility != STV_DEFAULT all non-STV_DEFAULT symbols cannot be pre-empted and so are definitely dso_local. Removing this condition in the OR and running ninja check-all doesn't show any failure so it's not tested. I think the intent here is to capture cases where in .so and .dylib's some symbols cannot be overriden externally and so can be made dso_local. Skimming through how visibility is done on MachO I see the n_type of N_PEXT is non-preemptible visibility and maps to the isExternal property of the symbol.

I think Mach-O can be stricter here due to the two-level namespace (which is again covered by the initialization of interposable). We should therefore be able to apply dso_local to non-weak externs too. I think we just have to exclude weak externs since they are effectively handled as flat namespace lookups at runtime.

I see, so use interposable directly instead of the proxy of output type/visibility in ELF. That's clean.

(But maybe we can still hide weak externs that also satisfy canBeOmittedFromSymbolTable()?)

Curious: are weak externs user annotated? Wondering how weak extern + canBeOmittedFromSymbolTable() can exist.

!(dr->section == nullptr && (!sym->file || sym->file->isElf())) is purely to deal with cases like in the test test/ELF/lto/abs-resol.ll (D42977 + D43051). That test looks like it ports over well to MachO, though the second half using linker script may not exist yet

Mach-O doesn't have linker scripts :) But regardless I don't think the bug that the check is working around applies to the Mach-O codepaths. At least, I'm not seeing an error when running LLD (with this diff applied) against the equivalent absolute symbol test: https://gist.github.com/int3/d25a9cb7e1a12084fe1348ed8bee146f. I don't think we need to include a port of the abs-resol.ll test since it seems like an ELF-specific regression check.

Testing locally that global is getting dso_local but no crashes on linker fixup. Thanks for checking and linking to your test case.

MaskRay added a comment.EditedMar 11 2022, 4:13 PM

I think Mach-O can be stricter here due to the two-level namespace (which is again covered by the initialization of interposable). We should therefore be able to apply dso_local to non-weak externs too. I think we just have to exclude weak externs since they are effectively handled as flat namespace lookups at runtime.

The ELF condition of (isExec || sym->visibility != STV_DEFAULT) && ... is because:
in ELF, a default visibility non-local symbol is by default preemptible/interposable.

With macOS two-level namespace (https://maskray.me/blog/2021-05-16-elf-interposition-and-bsymbolic#alternative-symbol-search-models),
macOS can be stricter.

Surprisingly, however, despite us marking FinalDefinitionInLinkageUnit = false, the LTO backend doesn't seem to add dso_local to these symbols. OTOH the ELF internalize-exportdyn.ll test shows that dso_local gets added for the equivalent inputs. (cc @MaskRay for the dso_local stuff)

Because FinalDefinitionInLinkageUnit = true, lib/LTO/LTO.cpp:802 sets dso_local on _start.


I agree that !defined->isExternalWeakDef() && !defined->interposable; is sufficient for Mach-O.
There is no linker script absolute symbols. defined->interposable maps to the ELF preemptibility thing.

It seems that !defined->isExternalWeakDef() is not covered by a test.
I think the condition can be removed, but perhaps there is some Mach-O thing that I haven't thought about.

About why hidden undefined weak (not relevant to our Defined discussion here) does not set dso_local: GCC traditionally uses GOT references for hidden undefined weak. Some code may rely on the zero value behavior. ELF says "Unresolved weak symbols have a zero value." and this rule appears to override the visibility.

int3 added a comment.Mar 15 2022, 11:00 AM

Curious: are weak externs user annotated? Wondering how weak extern + canBeOmittedFromSymbolTable() can exist.

All the internalized symbols in D121428: [lld-macho] Extend lto-internalize-unnamed-addr.ll are weak externs that satisfy canBeOmittedFromSymbolTable(). I don't think there's a conflict here...?

Because FinalDefinitionInLinkageUnit = true, lib/LTO/LTO.cpp:802 sets dso_local on _start.

Oops, I mistyped, I meant that we were setting FinalDefinitionInLinkageUnit = true. But I see now that the symbols for which we were setting it to true in lto-internalize-unnamed-addr.ll were getting internalized anyway, so that's probably why they weren't marked dso_local.

It seems that !defined->isExternalWeakDef() is not covered by a test.

Added one in lto-final-definition.ll.

I think the condition can be removed, but perhaps there is some Mach-O thing that I haven't thought about.

Weak symbol lookups in Mach-O essentially happen in a flat namespace, so I think they should never be treated as final definitions. Also, this lookup occurs at runtime regardless of whether we are building an executable or dylib, and regardless of whether the symbol is a Defined or a DylibSymbol. Does ELF behave differently?

MaskRay accepted this revision as: MaskRay.EditedMar 15 2022, 11:16 AM

Looks great!

I think the condition can be removed, but perhaps there is some Mach-O thing that I haven't thought about.

Weak symbol lookups in Mach-O essentially happen in a flat namespace, so I think they should never be treated as final definitions. Also, this lookup occurs at runtime regardless of whether we are building an executable or dylib, and regardless of whether the symbol is a Defined or a DylibSymbol. Does ELF behave differently?

Ah, I see. On ELF, a weak definition is equivalent to a non-weak definition: if the executable defines a weak symbol, references to the symbol will bind to this definition, even if a shared object has a STB_GLOBAL definition.
(This is followed by glibc and musl. Some *BSD systems deviated: https://maskray.me/blog/2021-08-22-freebsd-src-browsing-on-linux-and-my-rtld-contribution#stb_weak-in-symbol-lookup)

This revision is now accepted and ready to land.Mar 15 2022, 11:16 AM