-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[lld][macho] Support 1-byte branch relocs for x86_64 #164439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-lld-macho @llvm/pr-subscribers-lld Author: Jez Ng (int3) ChangesFixes #160894. Full diff: https://github.com/llvm/llvm-project/pull/164439.diff 5 Files Affected:
diff --git a/lld/MachO/Arch/X86_64.cpp b/lld/MachO/Arch/X86_64.cpp
index 0a950f28f02ae..d3d7341aa6799 100644
--- a/lld/MachO/Arch/X86_64.cpp
+++ b/lld/MachO/Arch/X86_64.cpp
@@ -51,10 +51,10 @@ struct X86_64 : TargetInfo {
static constexpr std::array<RelocAttrs, 10> relocAttrsArray{{
#define B(x) RelocAttrBits::x
- {"UNSIGNED",
- B(UNSIGNED) | B(ABSOLUTE) | B(EXTERN) | B(LOCAL) | B(BYTE4) | B(BYTE8)},
+ {"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(BYTE4)},
+ {"BRANCH", B(PCREL) | B(EXTERN) | 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)},
@@ -84,6 +84,8 @@ int64_t X86_64::getEmbeddedAddend(MemoryBufferRef mb, uint64_t offset,
const uint8_t *loc = buf + offset + rel.r_address;
switch (rel.r_length) {
+ case 0:
+ return *loc + pcrelOffset(rel.r_type);
case 2:
return static_cast<int32_t>(read32le(loc)) + pcrelOffset(rel.r_type);
case 3:
@@ -96,11 +98,18 @@ int64_t X86_64::getEmbeddedAddend(MemoryBufferRef mb, uint64_t offset,
void X86_64::relocateOne(uint8_t *loc, const Reloc &r, uint64_t value,
uint64_t relocVA) const {
if (r.pcrel) {
- uint64_t pc = relocVA + 4 + pcrelOffset(r.type);
+ uint64_t pc = relocVA + (1 << r.length) + pcrelOffset(r.type);
value -= pc;
}
switch (r.length) {
+ case 0:
+ if (r.type == X86_64_RELOC_UNSIGNED)
+ checkUInt(loc, r, value, 8);
+ else
+ checkInt(loc, r, value, 8);
+ *loc = value;
+ break;
case 2:
if (r.type == X86_64_RELOC_UNSIGNED)
checkUInt(loc, r, value, 32);
diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index 442fc608865d2..20e4a1d755229 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -516,12 +516,8 @@ static bool validateRelocationInfo(InputFile *file, const SectionHeader &sec,
if (isThreadLocalVariables(sec.flags) &&
!relocAttrs.hasAttr(RelocAttrBits::UNSIGNED))
error(message("not allowed in thread-local section, must be UNSIGNED"));
- if (rel.r_length < 2 || rel.r_length > 3 ||
- !relocAttrs.hasAttr(static_cast<RelocAttrBits>(1 << rel.r_length))) {
- static SmallVector<StringRef, 4> widths{"0", "4", "8", "4 or 8"};
- error(message("has width " + std::to_string(1 << rel.r_length) +
- " bytes, but must be " +
- widths[(static_cast<int>(relocAttrs.bits) >> 2) & 3] +
+ if (!relocAttrs.hasAttr(static_cast<RelocAttrBits>(1 << rel.r_length))) {
+ error(message("has invalid width of " + std::to_string(1 << rel.r_length) +
" bytes"));
}
return valid;
diff --git a/lld/MachO/Relocations.h b/lld/MachO/Relocations.h
index b2f621451349e..d3347de36781b 100644
--- a/lld/MachO/Relocations.h
+++ b/lld/MachO/Relocations.h
@@ -25,21 +25,23 @@ class InputSection;
enum class RelocAttrBits {
_0 = 0, // invalid
- PCREL = 1 << 0, // Value is PC-relative offset
- ABSOLUTE = 1 << 1, // Value is an absolute address or fixed offset
+ BYTE1 = 1 << 0, // 1 byte datum
+ BYTE2 = 1 << 1, // 2 byte datum
BYTE4 = 1 << 2, // 4 byte datum
BYTE8 = 1 << 3, // 8 byte datum
- EXTERN = 1 << 4, // Can have an external symbol
- LOCAL = 1 << 5, // Can have a local symbol
- ADDEND = 1 << 6, // *_ADDEND paired prefix reloc
- SUBTRAHEND = 1 << 7, // *_SUBTRACTOR paired prefix reloc
- BRANCH = 1 << 8, // Value is branch target
- GOT = 1 << 9, // References a symbol in the Global Offset Table
- TLV = 1 << 10, // References a thread-local symbol
- LOAD = 1 << 11, // Relaxable indirect load
- POINTER = 1 << 12, // Non-relaxable indirect load (pointer is taken)
- UNSIGNED = 1 << 13, // *_UNSIGNED relocs
- LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue*/ (1 << 14) - 1),
+ PCREL = 1 << 4, // Value is PC-relative offset
+ ABSOLUTE = 1 << 5, // Value is an absolute address or fixed offset
+ EXTERN = 1 << 6, // Can have an external symbol
+ LOCAL = 1 << 7, // Can have a local symbol
+ ADDEND = 1 << 8, // *_ADDEND paired prefix reloc
+ SUBTRAHEND = 1 << 9, // *_SUBTRACTOR paired prefix reloc
+ BRANCH = 1 << 10, // Value is branch target
+ GOT = 1 << 11, // References a symbol in the Global Offset Table
+ TLV = 1 << 12, // References a thread-local symbol
+ LOAD = 1 << 13, // Relaxable indirect load
+ POINTER = 1 << 14, // Non-relaxable indirect load (pointer is taken)
+ UNSIGNED = 1 << 15, // *_UNSIGNED relocs
+ LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue*/ (1 << 16) - 1),
};
// Note: SUBTRACTOR always pairs with UNSIGNED (a delta between two symbols).
diff --git a/lld/test/MachO/invalid/invalid-relocation-length.yaml b/lld/test/MachO/invalid/invalid-relocation-length.yaml
index ff8759b417478..2661e4ea165ec 100644
--- a/lld/test/MachO/invalid/invalid-relocation-length.yaml
+++ b/lld/test/MachO/invalid/invalid-relocation-length.yaml
@@ -2,7 +2,7 @@
# RUN: yaml2obj %s -o %t.o
# RUN: not %lld -o %t %t.o 2>&1 | FileCheck %s -DFILE=%t.o
#
-# CHECK: error: UNSIGNED relocation has width 2 bytes, but must be 4 or 8 bytes at offset 1 of __TEXT,__text in [[FILE]]
+# CHECK: error: UNSIGNED relocation has invalid width of 2 bytes at offset 1 of __TEXT,__text in [[FILE]]
!mach-o
FileHeader:
diff --git a/lld/test/MachO/x86-64-relocs.s b/lld/test/MachO/x86-64-relocs.s
index cdeee96c182f0..422808df81357 100644
--- a/lld/test/MachO/x86-64-relocs.s
+++ b/lld/test/MachO/x86-64-relocs.s
@@ -12,6 +12,7 @@
# CHECK-LABEL: <_main>:
## Test X86_64_RELOC_BRANCH
# CHECK: callq 0x[[#%x, F_ADDR]] <_f>
+# CHECK: jrcxz 0x[[#%x, F_ADDR]] <_f>
## Test extern (symbol) X86_64_RELOC_SIGNED
# CHECK: leaq [[#%u, LOCAL_OFF:]](%rip), %rsi
# CHECK-NEXT: [[#%x, DATA_ADDR - LOCAL_OFF]]
@@ -26,7 +27,8 @@
.section __TEXT,__text
.globl _main, _f
_main:
- callq _f # X86_64_RELOC_BRANCH
+ callq _f # X86_64_RELOC_BRANCH with r_length=2
+ jrcxz _f # X86_64_RELOC_BRANCH with r_length=0
mov $0, %rax
ret
|
| @@ -96,11 +98,18 @@ int64_t X86_64::getEmbeddedAddend(MemoryBufferRef mb, uint64_t offset, | |||
| void X86_64::relocateOne(uint8_t *loc, const Reloc &r, uint64_t value, | |||
| uint64_t relocVA) const { | |||
| if (r.pcrel) { | |||
| uint64_t pc = relocVA + 4 + pcrelOffset(r.type); | |||
| uint64_t pc = relocVA + (1 << r.length) + pcrelOffset(r.type); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, could we factor out 1 << r.length into its own function? Then I could use it to fix the bug in BPSectionOrderer.cpp and hopefully prevent future bugs like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a function just for one binary op?? seems like overkill haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also used in another file :) But yeah, it's not a hard requirement for me. Although I wish it was at least named r.loglength or something similar.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ellishg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I want to wait a few days to give others a chance to review.
|
Thanks for the review! As I've not contributed to LLVM in a while, my commit access has expired. Could you commit it for me please? |
Sure I can land it Monday! |
… 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.
Fixes #160894.