[lld-macho] Set FinalDefinitionInLinkageUnit on most LTO externs

[int3]

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.

[modimo]

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

[int3]

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]

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

[modimo]

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

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]

update

[int3]

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]

Looks great!

In D119506#3383201, @int3 wrote:

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)

lld/MachO/LTO.cpp

for (const lto::InputFile::Symbol &objSym : objSyms) {
 
     // Ideally we shouldn't check for SF_Undefined but currently IRObjectFile
     // reports two symbols for module ASM defined. Without this check, lld
     // flags an undefined in IR with a definition in ASM as prevailing.
     // Once IRObjectFile is fixed to report only one symbol this hack can
     // be removed.
     r.Prevailing = !objSym.isUndefined() && sym->getFile() == &f;
 
-    if (const auto *defined = dyn_cast<Defined>(sym))
+    if (const auto *defined = dyn_cast<Defined>(sym)) {
       r.ExportDynamic =
           defined->isExternal() && !defined->privateExtern && exportDynamic;
-    else if (const auto *common = dyn_cast<CommonSymbol>(sym))
+      r.FinalDefinitionInLinkageUnit =
+          !defined->isExternalWeakDef() && !defined->interposable;
+    } else if (const auto *common = dyn_cast<CommonSymbol>(sym)) {
       r.ExportDynamic = !common->privateExtern && exportDynamic;
+      r.FinalDefinitionInLinkageUnit = true;
+    }
 
     r.VisibleToRegularObj =
         sym->isUsedInRegularObj || (r.Prevailing && r.ExportDynamic);
 
     // Un-define the symbol so that we don't get duplicate symbol errors when we
     // load the ObjFile emitted by LTO compilation.
     if (r.Prevailing)
       replaceSymbol<Undefined>(sym, sym->getName(), sym->getFile(),

lld/test/MachO/lto-final-definition.ll

+; REQUIRES: x86
+; RUN: rm -rf %t; mkdir %t
+; RUN: llvm-as %s -o %t/test.o
+; RUN: %lld -lSystem -dylib %t/test.o -o %t/test -save-temps
+; RUN: llvm-dis %t/test.0.2.internalize.bc -o - | FileCheck %s
+; RUN: %lld -lSystem -dylib %t/test.o -o %t/flat-namespace.dylib -save-temps \
+; RUN:   -flat_namespace
+; RUN: llvm-dis %t/flat-namespace.dylib.0.2.internalize.bc -o - | FileCheck %s \
+; RUN:   --check-prefix=NO-DSO-LOCAL
+
+;; f() is never dso_local since it is a weak external.
+; CHECK:        define weak_odr void @f()
+; CHECK:        define dso_local void @main()
+
+; NO-DSO-LOCAL: define weak_odr void @f()
+; NO-DSO-LOCAL: define void @main()
+
+target triple = "x86_64-apple-macosx10.15.0"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+define weak_odr void @f() {
+  ret void
+}
+
+define void @main() {
+  ret void
+}

lld/test/MachO/lto-internalize.ll

 ; RUN: llvm-dis < %t/test.0.2.internalize.bc | FileCheck %s
 ; RUN: llvm-objdump --macho --syms %t/test | FileCheck %s --check-prefix=SYMTAB
 
 ; CHECK: @comm = internal global
 ; CHECK: @comm_hide = internal global
 
 ;; Check that main is not internalized. This covers the case of bitcode symbols
 ;; referenced by undefined symbols that don't belong to any InputFile.
-; CHECK: define void @main()
+; CHECK: define dso_local void @main()
 
 ;; Check that the foo and bar functions are correctly internalized.
 ; CHECK: define internal void @bar()
 ; CHECK: define internal void @foo()
 
 ;; Check that a bitcode symbol that is referenced by a regular object file isn't
 ;; internalized.
-; CHECK: define void @used_in_regular_obj()
+; CHECK: define dso_local void @used_in_regular_obj()
 
 ;; Check that a bitcode symbol that is defined in another bitcode file gets
 ;; internalized.
 ; CHECK: define internal void @baz()
 
 ;; Check that all internalized symbols are not emitted to the symtab
 ; SYMTAB-LABEL: SYMBOL TABLE:
 ; SYMTAB-DAG:   g     F __TEXT,__text _main

 
 ; RUN: %lld -lSystem -export_dynamic %t/test.o %t/baz.o %t/regular.o -o %t/test.extdyn -save-temps
 ; RUN: llvm-dis < %t/test.extdyn.0.2.internalize.bc
 ; RUN: llvm-nm -m %t/test.extdyn | FileCheck %s --check-prefix=DYN-SYMS \
 ; RUN:   --implicit-check-not _foo
 
 ;; Note that only foo() gets internalized here; everything else that isn't
 ;; hidden must be exported.
-; DYN: @comm = common global
+; DYN: @comm = common dso_local global
 ; DYN: @comm_hide = internal global
-; DYN: define void @main()
-; DYN: define void @bar()
+; DYN: define dso_local void @main()
+; DYN: define dso_local void @bar()
 ; DYN: define internal void @foo()
-; DYN: define void @used_in_regular_obj()
-; DYN: define void @baz()
+; DYN: define dso_local void @used_in_regular_obj()
+; DYN: define dso_local void @baz()
 
 ; DYN-SYMS-DAG: (__TEXT,__text) external _bar
 ; DYN-SYMS-DAG: (__TEXT,__text) external _baz
 ; DYN-SYMS-DAG: (__DATA,__common) external _comm
 ; DYN-SYMS-DAG: (__TEXT,__text) external _main
 ; DYN-SYMS-DAG: (__TEXT,__text) external _used_in_regular_obj
 
 ;--- test.s