[PATCH] D91993: [ELF] Don't relax R_X86_64_GOTPCRELX if addend != -4

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D91993#2414393 <https://reviews.llvm.org/D91993#2414393>, @MaskRay wrote:

> In D91993#2414225 <https://reviews.llvm.org/D91993#2414225>, @MaskRay wrote:
>
>> In D91993#2413016 <https://reviews.llvm.org/D91993#2413016>, @grimar wrote:
>>
>>> My understanding is that R_X86_64_*RELX relocation should never be emitted when a relaxation is not possible.
>>> ABI doesn't say anything about addends I think ("B.2 Optimize GOTPCRELX Relocations", p151, https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf).
>>> So, why should this be handled on linker side and not on the compiler side?
>>
>> There are two questions:
>>
>> - Whether `movl x at GOTPCREL+4(%rip), %eax` is valid?
>> - Whether the expression should assemble to `R_X86_64_GOTPCRELX`?
>>
>> For the first, I tend to think it is valid - this allows the compiler generates an instruction to load the half part of the GOT entry.
>> For the second, my understanding is that `R_X86_64_GOTPCREL` can be entirely replaced by `R_X86_64_[REX_]GOTPCRELX`. If so, it is certainly reasonable to assemble to `R_X86_64_GOTPCRELX`. I can send a clarification on binutils, though.
>
> Asked on binutils and HJ Lu said  `movl x at GOTPCREL+4(%rip), %eax` is valid (https://sourceware.org/pipermail/binutils/2020-November/114264.html) and `R_X86_64_GOTPCRELX` is correct.
> So I think we should proceed with this patch. I also created a gold bug report: https://sourceware.org/bugzilla/show_bug.cgi?id=26939

OK. Thanks for the information. I still don't understand why the direction is to add job for the linker instead of not emitting a potentially relaxable relocation from the begining on the compiler side.
But since it is what will be added to x86-64-ABI, I have no arguments from not doing this in LLD.

I'd add the `TargetInfo::adjustGotPcExpr` in a separate patch though to avoid having related PPC64 changes.



================
Comment at: lld/ELF/Arch/PPC64.cpp:1395
 
 RelExpr PPC64::adjustRelaxExpr(RelType type, const uint8_t *data,
                                RelExpr expr) const {
----------------
It feels that splitting the `adjustRelaxExpr` to `adjustGotPcExpr` and renaming the first one to `adjustTlsExpr` should be done separatelly
and then this diff will only contain X86_64 related code?


================
Comment at: lld/test/ELF/x86-64-gotpc-offset.s:1
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
----------------
We have `x86-64-gotpc-relax.s` test case for testing relaxations. I think this test should live there?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91993/new/

https://reviews.llvm.org/D91993