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

Thu Nov 26 01:23:16 PST 2020

jhenderson added a comment.

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

> In D91993#2416238 <https://reviews.llvm.org/D91993#2416238>, @jhenderson wrote:
>
>> FWIW, I'm with @grimar in the sense that I think this is poor and unnecessary behaviour provided for by the psABI. Why emit the relocation type in this context, when the primary purpose (assuming I'm not mistaken) of this relocation type is to enable relaxations? Why not emit the generic GOTPCREL relocation?
>
> MC could emit R_X86_64_GOTPCREL in this case. Then call GNU as' emitting R_X86_64_GOTPCRELX a bug?

I wouldn't call it a bug in the assembler, as my understanding from your comments earlier is that it is conformant with the ABI. I do think I'd consider it a possible bug in the psABI though, but maybe I'm misunderstanding the intent of GOTPCRELX (I thought it indicated that the target place is a candidate for relaxation, which the linker may choose to do).

> Hmm, I think the assembler behavior is fine, because R_X86_64_GOTPCREL in all cases can be replaced by R_X86_64_GOTPCRELX and R_X86_64_REX_GOTPCRELX, if the user does not mind the instructions are relaxed.
>
> Note, R_X86_64_GOTPCRELX already does not guarantee ensured relaxation, e.g. when the symbol is preemptible the linker cannot relax the instruction, when the instruction is not one of call,jmp,adc,add,and,cmp,or,sbb,sub,test,xor, etc.

In D91993#2417791 <https://reviews.llvm.org/D91993#2417791>, @grimar wrote:

> Seems that, e.g., `R_X86_64_GOTPCREL` should give a guarantee that relaxation will not be performed though. As far I understand GNU linkers can relax `R_X86_64_GOTPCREL` too? It looks like a violation of ABI?

I think our downstream platform has previously relaxed GOTPCREL too (although I'd have to go back and check). I don't think it hurts for a linker to perform an optimisation it can see is safe. I wouldn't say it's a violation of the ABI in this case, rather just something not covered by the ABI. Probably in all those cases that it did that, the relocation is now a GOTPCRELX, but I don't think it always was.

> The suggested change (for ABI) about addend then seems only needed to support this behavior of GNU linkers. In LLVM we probably should just not emit a potentially relaxable relocation from the begining.

Strictly, it could be argued that we'd actually be violating the ABI in that case, though I don't disagree with it. The psABI states that in the case of `mov name at GOTPCREL(%rip), %reg` a GOTPCRELX relocation should be emitted. I'm note convinced that pattern specifically states the same for the +4 version you mentioned before, but HJ Lu seems to suggest it does, so if we didn't emit a GOTPCRELX, we'd be ignoring that statement.

Anyway, that's the other patch (D92114 <https://reviews.llvm.org/D92114>). Since we'll have to accept the GNU assembler output, it probably makes sense to do this patch. I haven't reviewed the content, but seems okay in principle. Feel free to commit once reviewers are happy.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91993