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

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.
Herald added a project: All.

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 <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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119506