[PATCH] D119506: [lld-macho] Set FinalDefinitionInLinkageUnit on most LTO externs

Fri Mar 11 15:29:53 PST 2022

modimo added a comment.

In D119506#3374156 <https://reviews.llvm.org/D119506#3374156>, @int3 wrote:

>> 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 <https://reviews.llvm.org/D121428> 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 <https://reviews.llvm.org/D42977> + D43051 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119506/new/

https://reviews.llvm.org/D119506