Skip to content
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.

Sign up for GitHub

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

[lld/ELF][X86] Respect outSecOff when checking if GOTPCREL can be relaxed #86334

Merged
merged 1 commit into from
Mar 24, 2024
Merged

[lld/ELF][X86] Respect outSecOff when checking if GOTPCREL can be relaxed #86334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions lld/ELF/Arch/X86_64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 11 additions & 1 deletion lld/test/ELF/x86-64-gotpc-relax-too-far.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -47,6 +50,13 @@ SECTIONS {
data 0x80200000 : { *(data) }
}
#--- lds2
SECTIONS {
.text.foo 0x100000 : { *(.text.foo) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need any checks to go along with the new sections?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lds2 test does fail if . += 0x1000 below is removed or outSecOff from code is removed. So the test seems fine to me.

.text 0x1ff000 : { . = . + 0x1000 ; *(.text) }
.got 0x300000 : { *(.got) }
data 0x80200000 : { *(data) }
}
#--- lds3
SECTIONS {
.text.foo 0x100000 : { *(.text.foo) }
.text 0x200000 : { *(.text) }
Expand Down
Loading