[lld] enable fixup chains by default #79894
Conversation
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-macho Author: Richard Howell (rmaz) ChangesEnable chained fixups in lld when all platform and version criteria are met. This aligns with behavior in ld64: Full diff: https://github.com/llvm/llvm-project/pull/79894.diff 7 Files Affected:
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 411fbcfcf233eb8..796e9e0b351ca79 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -1075,8 +1075,7 @@ static bool shouldEmitChainedFixups(const InputArgList &args) {
return false;
}
- // TODO: Enable by default once stable.
- return isRequested;
+ return true;
}
void SymbolPatterns::clear() {
diff --git a/lld/test/MachO/arm64-objc-stubs-dyn.s b/lld/test/MachO/arm64-objc-stubs-dyn.s
index 9358fc5b31c2ba5..14dd6dd22b9d6f5 100644
--- a/lld/test/MachO/arm64-objc-stubs-dyn.s
+++ b/lld/test/MachO/arm64-objc-stubs-dyn.s
@@ -41,18 +41,18 @@
# CHECK-EMPTY:
# STUB: Contents of (__TEXT,__stubs) section
-# STUB-NEXT: adrp x16, 8 ; 0x100008000
+# STUB-NEXT: adrp x16, 4 ; 0x100004000
# STUB-NEXT: ldr x16, [x16]
# STUB-NEXT: br x16
# SMALL: Contents of (__TEXT,__objc_stubs) section
# SMALL-NEXT: _objc_msgSend$foo:
# SMALL-NEXT: adrp x1, 8 ; 0x100008000
-# SMALL-NEXT: ldr x1, [x1, #0x18]
+# SMALL-NEXT: ldr x1, [x1, #0x10]
# SMALL-NEXT: b
# SMALL-NEXT: _objc_msgSend$length:
# SMALL-NEXT: adrp x1, 8 ; 0x100008000
-# SMALL-NEXT: ldr x1, [x1, #0x20]
+# SMALL-NEXT: ldr x1, [x1, #0x18]
# SMALL-NEXT: b
.section __TEXT,__objc_methname,cstring_literals
diff --git a/lld/test/MachO/arm64-relocs.s b/lld/test/MachO/arm64-relocs.s
index 4bd0f6b8a0452fe..c090596992065e9 100644
--- a/lld/test/MachO/arm64-relocs.s
+++ b/lld/test/MachO/arm64-relocs.s
@@ -22,7 +22,7 @@
# CHECK-NEXT: ret
# CHECK-LABEL: Contents of (__DATA_CONST,__const) section
-# CHECK: [[#PTR_1]] {{0*}}[[#BAZ]] 00000000 00000000 00000000
+# CHECK: [[#PTR_1]] {{0*}}[[#BAZ]] 00200000 00000000 00000000
# CHECK: [[#PTR_2]] {{0*}}[[#BAZ+123]] 00000000 00000000 00000000
.text
diff --git a/lld/test/MachO/arm64-stubs.s b/lld/test/MachO/arm64-stubs.s
index f714e2008489533..6fd94661d32e275 100644
--- a/lld/test/MachO/arm64-stubs.s
+++ b/lld/test/MachO/arm64-stubs.s
@@ -5,7 +5,7 @@
# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin %t/test.s -o %t/test.o
# RUN: %lld -arch arm64 -dylib -install_name @executable_path/libfoo.dylib %t/foo.o -o %t/libfoo.dylib
# RUN: %lld -arch arm64 -dylib -install_name @executable_path/libbar.dylib %t/bar.o -o %t/libbar.dylib
-# RUN: %lld -arch arm64 -lSystem %t/libfoo.dylib %t/libbar.dylib %t/test.o -o %t/test
+# RUN: %lld -arch arm64 -lSystem %t/libfoo.dylib %t/libbar.dylib %t/test.o -o %t/test -no_fixup_chains
# RUN: llvm-objdump --no-print-imm-hex --macho -d --no-show-raw-insn --section="__TEXT,__stubs" --section="__TEXT,__stub_helper" %t/test | FileCheck %s
diff --git a/lld/test/MachO/dyld-stub-binder.s b/lld/test/MachO/dyld-stub-binder.s
index 1d2e556607c79c6..170fe8abd89bd44 100644
--- a/lld/test/MachO/dyld-stub-binder.s
+++ b/lld/test/MachO/dyld-stub-binder.s
@@ -14,27 +14,27 @@
# RUN: %lld -arch arm64 -lSystem -dylib %t/foo.o -o %t/libfoo.dylib
# RUN: llvm-nm -m %t/libfoo.dylib | FileCheck --check-prefix=STUB %s
-
## Dylibs that do lazy dynamic calls do need dyld_stub_binder.
# RUN: not %no-lsystem-lld -arch arm64 -dylib %t/bar.o %t/libfoo.dylib \
-# RUN: -o %t/libbar.dylib 2>&1 | FileCheck --check-prefix=MISSINGSTUB %s
+# RUN: -o %t/libbar.dylib -no_fixup_chains 2>&1 | \
+# RUN: FileCheck --check-prefix=MISSINGSTUB %s
# RUN: %lld -arch arm64 -lSystem -dylib %t/bar.o %t/libfoo.dylib \
-# RUN: -o %t/libbar.dylib
+# RUN: -o %t/libbar.dylib -no_fixup_chains
# RUN: llvm-nm -m %t/libbar.dylib | FileCheck --check-prefix=STUB %s
## As do executables.
# RUN: not %no-lsystem-lld -arch arm64 %t/libfoo.dylib %t/libbar.dylib %t/test.o \
-# RUN: -o %t/test 2>&1 | FileCheck --check-prefix=MISSINGSTUB %s
+# RUN: -o %t/test -no_fixup_chains 2>&1 | FileCheck --check-prefix=MISSINGSTUB %s
# RUN: %lld -arch arm64 -lSystem %t/libfoo.dylib %t/libbar.dylib %t/test.o \
-# RUN: -o %t/test
+# RUN: -o %t/test -no_fixup_chains
# RUN: llvm-nm -m %t/test | FileCheck --check-prefix=STUB %s
## Test dynamic lookup of dyld_stub_binder.
# RUN: %no-lsystem-lld -arch arm64 %t/libfoo.dylib %t/libbar.dylib %t/test.o \
-# RUN: -o %t/test -undefined dynamic_lookup
+# RUN: -o %t/test -undefined dynamic_lookup -no_fixup_chains
# RUN: llvm-nm -m %t/test | FileCheck --check-prefix=DYNSTUB %s
# RUN: %no-lsystem-lld -arch arm64 %t/libfoo.dylib %t/libbar.dylib %t/test.o \
-# RUN: -o %t/test -U dyld_stub_binder
+# RUN: -o %t/test -U dyld_stub_binder -no_fixup_chains
# RUN: llvm-nm -m %t/test | FileCheck --check-prefix=DYNSTUB %s
# MISSINGSTUB: error: undefined symbol: dyld_stub_binder
diff --git a/lld/test/MachO/icf-safe.ll b/lld/test/MachO/icf-safe.ll
index 71c6f9f7ddac874..7f342e12e5c2a69 100644
--- a/lld/test/MachO/icf-safe.ll
+++ b/lld/test/MachO/icf-safe.ll
@@ -3,14 +3,14 @@
; RUN: rm -rf %t; mkdir %t
; RUN: llc -filetype=obj %s -O3 -o %t/icf-obj.o -enable-machine-outliner=never -mtriple arm64-apple-macos -addrsig
-; RUN: %lld -arch arm64 -lSystem --icf=safe -dylib -o %t/icf-safe.dylib %t/icf-obj.o
-; RUN: %lld -arch arm64 -lSystem --icf=all -dylib -o %t/icf-all.dylib %t/icf-obj.o
+; RUN: %lld -arch arm64 -lSystem --icf=safe -dylib -o %t/icf-safe.dylib %t/icf-obj.o -no_fixup_chains
+; RUN: %lld -arch arm64 -lSystem --icf=all -dylib -o %t/icf-all.dylib %t/icf-obj.o -no_fixup_chains
; RUN: llvm-objdump %t/icf-safe.dylib -d --macho | FileCheck %s --check-prefix=ICFSAFE
; RUN: llvm-objdump %t/icf-all.dylib -d --macho | FileCheck %s --check-prefix=ICFALL
; RUN: llvm-as %s -o %t/icf-bitcode.o
-; RUN: %lld -arch arm64 -lSystem --icf=safe -dylib -o %t/icf-safe-bitcode.dylib %t/icf-bitcode.o
-; RUN: %lld -arch arm64 -lSystem --icf=all -dylib -o %t/icf-all-bitcode.dylib %t/icf-bitcode.o
+; RUN: %lld -arch arm64 -lSystem --icf=safe -dylib -o %t/icf-safe-bitcode.dylib %t/icf-bitcode.o -no_fixup_chains
+; RUN: %lld -arch arm64 -lSystem --icf=all -dylib -o %t/icf-all-bitcode.dylib %t/icf-bitcode.o -no_fixup_chains
; RUN: llvm-objdump %t/icf-safe-bitcode.dylib -d --macho | FileCheck %s --check-prefix=ICFSAFE
; RUN: llvm-objdump %t/icf-all-bitcode.dylib -d --macho | FileCheck %s --check-prefix=ICFALL
diff --git a/lld/test/MachO/objc-selrefs.s b/lld/test/MachO/objc-selrefs.s
index 9dff440727ac34b..a906978cc2cf728 100644
--- a/lld/test/MachO/objc-selrefs.s
+++ b/lld/test/MachO/objc-selrefs.s
@@ -6,14 +6,14 @@
# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin %t/implicit-selrefs.s -o %t/implicit-selrefs.o
# RUN: %lld -dylib -arch arm64 -lSystem -o %t/explicit-only-no-icf \
-# RUN: %t/explicit-selrefs-1.o %t/explicit-selrefs-2.o
+# RUN: %t/explicit-selrefs-1.o %t/explicit-selrefs-2.o -no_fixup_chains
# RUN: llvm-otool -vs __DATA __objc_selrefs %t/explicit-only-no-icf | \
# RUN: FileCheck %s --check-prefix=EXPLICIT-NO-ICF
## NOTE: ld64 always dedups the selrefs unconditionally, but we only do it when
## ICF is enabled.
# RUN: %lld -dylib -arch arm64 -lSystem -o %t/explicit-only-with-icf \
-# RUN: %t/explicit-selrefs-1.o %t/explicit-selrefs-2.o
+# RUN: %t/explicit-selrefs-1.o %t/explicit-selrefs-2.o -no_fixup_chains
# RUN: llvm-otool -vs __DATA __objc_selrefs %t/explicit-only-with-icf \
# RUN: | FileCheck %s --check-prefix=EXPLICIT-WITH-ICF
@@ -26,7 +26,8 @@
## We don't yet support dedup'ing implicitly-defined selrefs.
# RUN: %lld -dylib -arch arm64 -lSystem --icf=all -o %t/explicit-and-implicit \
-# RUN: %t/explicit-selrefs-1.o %t/explicit-selrefs-2.o %t/implicit-selrefs.o
+# RUN: %t/explicit-selrefs-1.o %t/explicit-selrefs-2.o %t/implicit-selrefs.o \
+# RUN: -no_fixup_chains
# RUN: llvm-otool -vs __DATA __objc_selrefs %t/explicit-and-implicit \
# RUN: | FileCheck %s --check-prefix=EXPLICIT-AND-IMPLICIT
|
|
@BertalanD I believe you've done extensive testing when implementing this. Do you know if this is stable? |
|
LGTM! Yes, this feature should be ready to go. IIRC, the Chromium folks are already using it in production for their iOS builds, but @nico should be able to confirm that. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
rmaz
force-pushed
the
cfixupsdefault
branch
2 times, most recently
from
f6748d2
to
d22c878
Compare
This commit enables chained fixups by default for supported platforms. This matches the logic used in ld64 907.
|
We're hitting linker crashes/asserts after this. See https://issues.chromium.org/u/1/issues/325133695#comment8 for a reproducer. Do you think this can be fixed quickly, or could we revert it in the meantime? |
While we look at this - passing "-no_fixup_chains" to the linker will make the crash go away. Would that work as a temp work-around ? |
|
Original author of our fixup-chains support here. I had a quick (sorry, school et al.) look at it. It looks like we are trying to find the address of Now, obviously people shouldn't be defining exported symbols inside The relevant "offending" code is here: |
|
cc @nico |
Yes, that should work. Thanks! |
|
We've hit another issue with fixup chains. In this case, lld doesn't crash but appears to create a corrupt binary. See https://crbug.com/325410295 for a reproducer. To prevent others from hitting the same problem, I'll go ahead and revert this now. |
This caused links to fail with: lld/MachO/Symbols.cpp:97: virtual uint64_t lld::macho::Defined::getVA() const: Assertion `target->usesThunks()' failed. or crash when asserts are disabled. See comment on #79894 > Enable chained fixups in lld when all platform and version criteria are > met. This is an attempt at simplifying the logic used in ld 907: > > https://github.com/apple-oss-distributions/ld64/blob/93d74eafc37c0558b4ffb88a8bc15c17bed44a20/src/ld/Options.cpp#L5458-L5549 > > Some changes were made to simplify the logic: > - only enable chained fixups for macOS from 13.0 to avoid the arch check > - only enable chained fixups for iphonesimulator from 16.0 to avoid the > arch check > - don't enable chained fixups for not specifically listed platforms > - don't enable chained fixups for arm64_32 This reverts commit 775c285.
|
@zmodem: it seems, from https://issues.chromium.org/issues/325410295, that the issue was with |
No, the issue did not reproduce with Even if the root cause is that There is also the separate issue of crashing when linking rust code (https://crbug.com/325133695). |
|
Sorry, misread one phrase of the other bug, and I thought |
Enable chained fixups in lld when all platform and version criteria are met. This is an attempt at simplifying the logic used in ld 907:
https://github.com/apple-oss-distributions/ld64/blob/main/src/ld/Options.cpp#L5458-L5549
Some changes were made to simplify the logic: