[lld/ELF][X86] Respect outSecOff when checking if GOTPCREL can be relaxed #86334
Conversation
…axed The existing implementation didn't handle when the input text section was some offset from the output section. This resulted in an assert in relaxGot() with an lld built with asserts for some large binaries, or even worse, a silently broken binary with an lld without asserts.
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Arthur Eubanks (aeubanks) ChangesThe existing implementation didn't handle when the input text section was some offset from the output section. This resulted in an assert in relaxGot() with an lld built with asserts for some large binaries, or even worse, a silently broken binary with an lld without asserts. Full diff: https://github.com/llvm/llvm-project/pull/86334.diff 2 Files Affected:
diff --git a/lld/ELF/Arch/X86_64.cpp b/lld/ELF/Arch/X86_64.cpp
index de459013595fed..a85bf3aa0c09d1 100644
--- a/lld/ELF/Arch/X86_64.cpp
+++ b/lld/ELF/Arch/X86_64.cpp
@@ -328,9 +328,10 @@ bool X86_64::relaxOnce(int pass) const {
if (rel.expr != R_RELAX_GOT_PC)
continue;
- uint64_t v = sec->getRelocTargetVA(
- sec->file, rel.type, rel.addend,
- sec->getOutputSection()->addr + rel.offset, *rel.sym, rel.expr);
+ uint64_t v = sec->getRelocTargetVA(sec->file, rel.type, rel.addend,
+ sec->getOutputSection()->addr +
+ sec->outSecOff + rel.offset,
+ *rel.sym, rel.expr);
if (isInt<32>(v))
continue;
if (rel.sym->auxIdx == 0) {
diff --git a/lld/test/ELF/x86-64-gotpc-relax-too-far.s b/lld/test/ELF/x86-64-gotpc-relax-too-far.s
index 74aa6d8f65a0d8..ba41faab67de5c 100644
--- a/lld/test/ELF/x86-64-gotpc-relax-too-far.s
+++ b/lld/test/ELF/x86-64-gotpc-relax-too-far.s
@@ -5,7 +5,10 @@
# RUN: llvm-objdump --no-print-imm-hex -d %t/bin | FileCheck --check-prefix=DISASM %s
# RUN: llvm-readelf -S %t/bin | FileCheck --check-prefixes=GOT %s
# RUN: ld.lld -T %t/lds2 %t/a.o -o %t/bin2
-# RUN: llvm-readelf -S %t/bin2 | FileCheck --check-prefixes=UNNECESSARY-GOT %s
+# RUN: llvm-objdump --no-print-imm-hex -d %t/bin2 | FileCheck --check-prefix=DISASM %s
+# RUN: llvm-readelf -S %t/bin2 | FileCheck --check-prefixes=GOT %s
+# RUN: ld.lld -T %t/lds3 %t/a.o -o %t/bin3
+# RUN: llvm-readelf -S %t/bin3 | FileCheck --check-prefixes=UNNECESSARY-GOT %s
# DISASM: <_foo>:
# DISASM-NEXT: movl 2097146(%rip), %eax
@@ -47,6 +50,13 @@ SECTIONS {
data 0x80200000 : { *(data) }
}
#--- lds2
+SECTIONS {
+ .text.foo 0x100000 : { *(.text.foo) }
+ .text 0x1ff000 : { . = . + 0x1000 ; *(.text) }
+ .got 0x300000 : { *(.got) }
+ data 0x80200000 : { *(data) }
+}
+#--- lds3
SECTIONS {
.text.foo 0x100000 : { *(.text.foo) }
.text 0x200000 : { *(.text) }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
| @@ -47,6 +50,13 @@ SECTIONS { | |||
| data 0x80200000 : { *(data) } | |||
| } | |||
| #--- lds2 | |||
| SECTIONS { | |||
| .text.foo 0x100000 : { *(.text.foo) } | |||
There was a problem hiding this comment.
Do we need any checks to go along with the new sections?
There was a problem hiding this comment.
Using a linker script to achieve the goal is too complex.
Use two input sections instead? The second input contains relocations while the first is for padding.
There was a problem hiding this comment.
Do we need any checks to go along with the new sections?
What do you mean by new sections? This linker script is a copy of the first one, but with the output section address adjusted.
Use two input sections instead? The second input contains relocations while the first is for padding.
if I'm understanding your request, using two input sections (e.g.)
.section .text,"ax"
.space 4096
.section .text,"ax"
.globl _start
.type _start, @function
_start:
movl __stop_data@GOTPCREL(%rip), %eax # out of range
movq __stop_data@GOTPCREL(%rip), %rax # out of range
movq __stop_data@GOTPCREL(%rip), %rax # in range
movl __stop_data@GOTPCREL(%rip), %eax # in range
adjusts rel.offset instead of sec->outSecOff so it doesn't trigger this bug
the bug I found was in the context of a linker script
There was a problem hiding this comment.
Do we need any checks to go along with the new sections?
if you mean .text.foo, perhaps we should remove that part of this test since it's obsoleted by the other part
There was a problem hiding this comment.
We've added new sections. Presumably it affects the output, but we're not checking for anything.
Basically, if we remove the added part, or if it has no intended effect, the test should start failing.
There was a problem hiding this comment.
The lds2 test does fail if . += 0x1000 below is removed or outSecOff from code is removed. So the test seems fine to me.
| @@ -47,6 +50,13 @@ SECTIONS { | |||
| data 0x80200000 : { *(data) } | |||
| } | |||
| #--- lds2 | |||
| SECTIONS { | |||
| .text.foo 0x100000 : { *(.text.foo) } | |||
There was a problem hiding this comment.
The lds2 test does fail if . += 0x1000 below is removed or outSecOff from code is removed. So the test seems fine to me.
…axed (llvm#86334) The existing implementation didn't handle when the input text section was some offset from the output section. This resulted in an assert in relaxGot() with an lld built with asserts for some large binaries, or even worse, a silently broken binary with an lld without asserts. (cherry picked from commit 4804805)
…axed (llvm#86334) The existing implementation didn't handle when the input text section was some offset from the output section. This resulted in an assert in relaxGot() with an lld built with asserts for some large binaries, or even worse, a silently broken binary with an lld without asserts. (cherry picked from commit 4804805)
The existing implementation didn't handle when the input text section was some offset from the output section.
This resulted in an assert in relaxGot() with an lld built with asserts for some large binaries, or even worse, a silently broken binary with an lld without asserts.