[lld] macho: Support section branch relocations, including the 1-byte form #169062
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… form I noticed that we had a hardcoded value of 4 for the pcrel section relocations, which seems like an issue given that we recently added support for 1-byte branch relocations in llvm#164439. The code included an assert that the relevant relocation had the BYTE4 attribute, but that is actually not enough to use a hardcoded value of 4: we need to assert that the *other* `BYTE<n>` attributes are not set either. However, since we did not support local branch relocations, that doesn't seem to have mattered in practice. That said, local branch relocations can be emitted by compilers, and ld64 does handle the 4-byte version of them, so I've added support for it here. ld64 actually seems to reject 1-byte section relocations, so the questionable code is actually probably fine (minus the incorrect assert). So we have two options: add an equivalent check in LLD, or just support 1-byte local branch relocations. Supporting it actually requires less code, so I've gone with that option here.
Member
|
@llvm/pr-subscribers-lld Author: Jez Ng (int3) ChangesI noticed that we had a hardcoded value of 4 for the pcrel section relocations, which seems like an issue given that we recently added support for 1-byte branch relocations in #164439. The code included an assert that the relevant relocation had the BYTE4 attribute, but that is actually not enough to use a hardcoded value of 4: we need to assert that the other However, since we did not support local branch relocations, that doesn't seem to have mattered in practice. That said, local branch relocations can be emitted by compilers, and ld64 does handle the 4-byte version of them, so I've added support for it here. ld64 actually seems to reject 1-byte section relocations, so the questionable code is actually probably fine (minus the incorrect assert). So we have two options: add an equivalent check in LLD, or just support 1-byte local branch relocations. Supporting it actually requires less code, so I've gone with that option here. Full diff: https://github.com/llvm/llvm-project/pull/169062.diff 3 Files Affected:
diff --git a/lld/MachO/Arch/X86_64.cpp b/lld/MachO/Arch/X86_64.cpp
index 111c4d9846d28..f35139906554f 100644
--- a/lld/MachO/Arch/X86_64.cpp
+++ b/lld/MachO/Arch/X86_64.cpp
@@ -54,7 +54,8 @@ static constexpr std::array<RelocAttrs, 10> relocAttrsArray{{
{"UNSIGNED", B(UNSIGNED) | B(ABSOLUTE) | B(EXTERN) | B(LOCAL) | B(BYTE1) |
B(BYTE4) | B(BYTE8)},
{"SIGNED", B(PCREL) | B(EXTERN) | B(LOCAL) | B(BYTE4)},
- {"BRANCH", B(PCREL) | B(EXTERN) | B(BRANCH) | B(BYTE1) | B(BYTE4)},
+ {"BRANCH",
+ B(PCREL) | B(EXTERN) | B(LOCAL) | B(BRANCH) | B(BYTE1) | B(BYTE4)},
{"GOT_LOAD", B(PCREL) | B(EXTERN) | B(GOT) | B(LOAD) | B(BYTE4)},
{"GOT", B(PCREL) | B(EXTERN) | B(GOT) | B(POINTER) | B(BYTE4)},
{"SUBTRACTOR", B(SUBTRAHEND) | B(EXTERN) | B(BYTE4) | B(BYTE8)},
diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index d0128d03a9eab..efcffc9c53383 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -594,8 +594,8 @@ void ObjFile::parseRelocations(ArrayRef<SectionHeader> sectionHeaders,
// FIXME This logic was written around x86_64 behavior -- ARM64 doesn't
// have pcrel section relocations. We may want to factor this out into
// the arch-specific .cpp file.
- assert(target->hasAttr(r.type, RelocAttrBits::BYTE4));
- referentOffset = sec.addr + relInfo.r_address + 4 + totalAddend -
+ referentOffset = sec.addr + relInfo.r_address +
+ (1ull << relInfo.r_length) + totalAddend -
referentSecHead.addr;
} else {
// The addend for a non-pcrel relocation is its absolute address.
diff --git a/lld/test/MachO/x86-64-relocs.s b/lld/test/MachO/x86-64-relocs.s
index 530394877437e..d29f689671208 100644
--- a/lld/test/MachO/x86-64-relocs.s
+++ b/lld/test/MachO/x86-64-relocs.s
@@ -4,6 +4,7 @@
# RUN: llvm-objdump --no-print-imm-hex --section-headers --syms -d %t | FileCheck %s
# CHECK-LABEL: Sections:
+# CHECK: __branch_target {{[0-9a-z]+}} [[#%x, BRANCH_SECT:]]
# CHECK: __data {{[0-9a-z]+}} [[#%x, DATA_ADDR:]]
# CHECK-LABEL: SYMBOL TABLE:
@@ -12,10 +13,18 @@
# CHECK-LABEL: <_main>:
## Test X86_64_RELOC_BRANCH
+## Test symbol (extern) relocations first (most common case)
# CHECK: callq 0x[[#%x, F_ADDR]] <_f>
# CHECK: jrcxz 0x[[#%x, F_ADDR]] <_f>
# CHECK: callq 0x[[#%x, G_ADDR]] <_g>
# CHECK: jrcxz 0x[[#%x, G_ADDR]] <_g>
+## Test section (local) BRANCH relocations
+# CHECK: callq 0x[[#%x, BRANCH_SECT]]
+## NOTE: ld64 rejects 1-byte local branch relocations as unsupported, but it
+## doesn't take any extra code for us to support it
+# CHECK: jrcxz 0x[[#%x, BRANCH_SECT]]
+
+# CHECK-LABEL: <_f>:
## Test extern (symbol) X86_64_RELOC_SIGNED
# CHECK: leaq [[#%u, LOCAL_OFF:]](%rip), %rsi
# CHECK-NEXT: [[#%x, DATA_ADDR - LOCAL_OFF]]
@@ -33,9 +42,12 @@
_main:
callq _f # X86_64_RELOC_BRANCH with r_length=2
jrcxz _f # X86_64_RELOC_BRANCH with r_length=0
- # test negative addends
+ # Test negative addends
callq _f - 1
jrcxz _f - 1
+ # Test section relocations
+ callq L_.branch_target
+ jrcxz L_.branch_target
mov $0, %rax
ret
@@ -48,6 +60,10 @@ _f:
movq L_.ptr_1(%rip), %rsi
ret
+.section __TEXT,__branch_target
+L_.branch_target:
+ ret
+
.data
## References to this generate a symbol relocation
_local:
|
Member
|
@llvm/pr-subscribers-lld-macho Author: Jez Ng (int3) ChangesI noticed that we had a hardcoded value of 4 for the pcrel section relocations, which seems like an issue given that we recently added support for 1-byte branch relocations in #164439. The code included an assert that the relevant relocation had the BYTE4 attribute, but that is actually not enough to use a hardcoded value of 4: we need to assert that the other However, since we did not support local branch relocations, that doesn't seem to have mattered in practice. That said, local branch relocations can be emitted by compilers, and ld64 does handle the 4-byte version of them, so I've added support for it here. ld64 actually seems to reject 1-byte section relocations, so the questionable code is actually probably fine (minus the incorrect assert). So we have two options: add an equivalent check in LLD, or just support 1-byte local branch relocations. Supporting it actually requires less code, so I've gone with that option here. Full diff: https://github.com/llvm/llvm-project/pull/169062.diff 3 Files Affected:
diff --git a/lld/MachO/Arch/X86_64.cpp b/lld/MachO/Arch/X86_64.cpp
index 111c4d9846d28..f35139906554f 100644
--- a/lld/MachO/Arch/X86_64.cpp
+++ b/lld/MachO/Arch/X86_64.cpp
@@ -54,7 +54,8 @@ static constexpr std::array<RelocAttrs, 10> relocAttrsArray{{
{"UNSIGNED", B(UNSIGNED) | B(ABSOLUTE) | B(EXTERN) | B(LOCAL) | B(BYTE1) |
B(BYTE4) | B(BYTE8)},
{"SIGNED", B(PCREL) | B(EXTERN) | B(LOCAL) | B(BYTE4)},
- {"BRANCH", B(PCREL) | B(EXTERN) | B(BRANCH) | B(BYTE1) | B(BYTE4)},
+ {"BRANCH",
+ B(PCREL) | B(EXTERN) | B(LOCAL) | B(BRANCH) | B(BYTE1) | B(BYTE4)},
{"GOT_LOAD", B(PCREL) | B(EXTERN) | B(GOT) | B(LOAD) | B(BYTE4)},
{"GOT", B(PCREL) | B(EXTERN) | B(GOT) | B(POINTER) | B(BYTE4)},
{"SUBTRACTOR", B(SUBTRAHEND) | B(EXTERN) | B(BYTE4) | B(BYTE8)},
diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index d0128d03a9eab..efcffc9c53383 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -594,8 +594,8 @@ void ObjFile::parseRelocations(ArrayRef<SectionHeader> sectionHeaders,
// FIXME This logic was written around x86_64 behavior -- ARM64 doesn't
// have pcrel section relocations. We may want to factor this out into
// the arch-specific .cpp file.
- assert(target->hasAttr(r.type, RelocAttrBits::BYTE4));
- referentOffset = sec.addr + relInfo.r_address + 4 + totalAddend -
+ referentOffset = sec.addr + relInfo.r_address +
+ (1ull << relInfo.r_length) + totalAddend -
referentSecHead.addr;
} else {
// The addend for a non-pcrel relocation is its absolute address.
diff --git a/lld/test/MachO/x86-64-relocs.s b/lld/test/MachO/x86-64-relocs.s
index 530394877437e..d29f689671208 100644
--- a/lld/test/MachO/x86-64-relocs.s
+++ b/lld/test/MachO/x86-64-relocs.s
@@ -4,6 +4,7 @@
# RUN: llvm-objdump --no-print-imm-hex --section-headers --syms -d %t | FileCheck %s
# CHECK-LABEL: Sections:
+# CHECK: __branch_target {{[0-9a-z]+}} [[#%x, BRANCH_SECT:]]
# CHECK: __data {{[0-9a-z]+}} [[#%x, DATA_ADDR:]]
# CHECK-LABEL: SYMBOL TABLE:
@@ -12,10 +13,18 @@
# CHECK-LABEL: <_main>:
## Test X86_64_RELOC_BRANCH
+## Test symbol (extern) relocations first (most common case)
# CHECK: callq 0x[[#%x, F_ADDR]] <_f>
# CHECK: jrcxz 0x[[#%x, F_ADDR]] <_f>
# CHECK: callq 0x[[#%x, G_ADDR]] <_g>
# CHECK: jrcxz 0x[[#%x, G_ADDR]] <_g>
+## Test section (local) BRANCH relocations
+# CHECK: callq 0x[[#%x, BRANCH_SECT]]
+## NOTE: ld64 rejects 1-byte local branch relocations as unsupported, but it
+## doesn't take any extra code for us to support it
+# CHECK: jrcxz 0x[[#%x, BRANCH_SECT]]
+
+# CHECK-LABEL: <_f>:
## Test extern (symbol) X86_64_RELOC_SIGNED
# CHECK: leaq [[#%u, LOCAL_OFF:]](%rip), %rsi
# CHECK-NEXT: [[#%x, DATA_ADDR - LOCAL_OFF]]
@@ -33,9 +42,12 @@
_main:
callq _f # X86_64_RELOC_BRANCH with r_length=2
jrcxz _f # X86_64_RELOC_BRANCH with r_length=0
- # test negative addends
+ # Test negative addends
callq _f - 1
jrcxz _f - 1
+ # Test section relocations
+ callq L_.branch_target
+ jrcxz L_.branch_target
mov $0, %rax
ret
@@ -48,6 +60,10 @@ _f:
movq L_.ptr_1(%rip), %rsi
ret
+.section __TEXT,__branch_target
+L_.branch_target:
+ ret
+
.data
## References to this generate a symbol relocation
_local:
|
🐧 Linux x64 Test Results
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed that we had a hardcoded value of 4 for the pcrel section relocations, which seems like an issue given that we recently added support for 1-byte branch relocations in #164439. The code included an assert that the relevant relocation had the BYTE4 attribute, but that is actually not enough to use a hardcoded value of 4: we need to assert that the other
BYTE<n>attributes are not set either.However, since we did not support local branch relocations, that doesn't seem to have mattered in practice. That said, local branch relocations can be emitted by compilers, and ld64 does handle the 4-byte version of them, so I've added support for it here.
ld64 actually seems to reject 1-byte section relocations, so the questionable code is actually probably fine (minus the incorrect assert). So we have two options: add an equivalent check in LLD, or just support 1-byte local branch relocations. Supporting it actually requires less code, so I've gone with that option here.