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.

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

release/18.x: [lld/ELF][X86] Respect outSecOff when checking if GOTPCREL can be relaxed (#86334) #86688

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

llvmbot
Copy link

@llvmbot llvmbot commented Mar 26, 2024

Backport 4804805

Requested by: @EugeneZelenko

@llvmbot llvmbot added this to the LLVM 18.X Release milestone Mar 26, 2024
@llvmbot
Copy link
Author

llvmbot commented Mar 26, 2024

@MaskRay What do you think about merging this PR to the release branch?

@llvmbot
Copy link
Author

llvmbot commented Mar 26, 2024

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: None (llvmbot)

Changes

Backport 4804805

Requested by: @EugeneZelenko


Full diff: https://github.com/llvm/llvm-project/pull/86688.diff

2 Files Affected:

  • (modified) lld/ELF/Arch/X86_64.cpp (+4-3)
  • (modified) lld/test/ELF/x86-64-gotpc-relax-too-far.s (+11-1)
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) }

@MaskRay
Copy link
Member

MaskRay commented Mar 26, 2024

The patch in question fixed a regression (assert in certain cases) introduced by https://reviews.llvm.org/D157020

…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)
@tstellar
Copy link
Collaborator

Hi @EugeneZelenko (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix.

@tstellar tstellar merged commit 767b61c into llvm:release/18.x Mar 27, 2024
6 of 7 checks passed
@EugeneZelenko
Copy link
Contributor

@aeubanks: Please answer @tstellar question. I just fixed cherry-pick command in issue.

@aeubanks
Copy link
Contributor

aeubanks commented Apr 5, 2024

I think (not 100% sure) that this fixes a regression that was introduced between 17 and 18, so it shouldn't require release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants