[PATCH] D59559: [ELF] Make R_I386_GOTPC and R_X86_64_GOTPC32/64 resolve to .got.plt.

[Siva Chandra via Phabricator via llvm-commits]

sivachandra added a comment.

In D59559#1435826 <https://reviews.llvm.org/D59559#1435826>, @MaskRay wrote:

> > About "will break some _GLOBAL_OFFSET_TABLE_ use cases", can you elaborate which ones?
>
> See the example x86-64-reloc-gotoff64.s
>
> With the patch, the sum doesn't compute `addr(_DYNAMIC)`
>
>   leaq _GLOBAL_OFFSET_TABLE_(%rip), %rdx ; R_X86_64_GOTPC32 (i.e R_GOTPLTONLY_PC): start(.got.plt) + A - P
>   movabsq $_DYNAMIC at GOTOFF, %rax         ; R_X86_64_GOTOFF64 (i.e R_GOTREL_FROM_END): addr(_DYNAMIC) - end(.got)
>   addq %rdx, %rax                        ; the sum is supposed to compute addr(_DYNAMIC)
>   
>
>
>
> > So, AFAIU, this patch does not break anything wrt the symbol _GLOBAL_OFFSET_TABLE_. But, you can point me to examples if that is the case.
>
> The current implementation in lld can have problems when there is a relocation type using `_GLOBAL_OFFSET_TABLE_` as `S`, e.g. a `R_X86_64_PC32` with `S=_GLOBAL_OFFSET_TABLE_`. This is different from `R_X86_64_GOTPC32`.
>
> For the example above, suppose `leaq _GLOBAL_OFFSET_TABLE_(%rip), %rdx` emits `R_X86_64_PC32` instead of `R_X86_64_GOTPC32`:
>
>   leaq _GLOBAL_OFFSET_TABLE_(%rip), %rdx ; R_X86_64_PC32: addr(_GLOBAL_OFFSET_TABLE_) + A - P
>   movabsq $_DYNAMIC at GOTOFF, %rax         ; R_X86_64_GOTOFF64 (i.e R_GOTREL_FROM_END): addr(_DYNAMIC) - end(.got)
>   addq %rdx, %rax                        ; addr(_GLOBAL_OFFSET_TABLE) = start(.got.plt) != end(.got), thus the sum is incorrect
>   


Yes, I have noticed the problem with R_X86_64_GOTOFF64 and I have another change for that which I have not completed because of other reasons [1]. But I did not bother in this change because this test, and the glibc example I have been using, do not fail after this change [2].

So, about R_X86_64_GOTOFF64, IMHO it is already broken in ld.lld.  But, the test is passing and working as intended because the current implementations of GOTPC64 and GOTOFF64 are cancelling each other out [3]. The reason I say R_X86_64_GOTOFF64 is broken is because of this:

In ld.lld currently, R_X86_64_GOTOFF64 resolves as this:

Sym.getVA(A) - In.Got->getVA() - In.Got->getSize()

I think, it should actually be:

Sym.getVA(A) - In.GotPlt->getVA()  // [4]

The reason why I think it should be expression [4] is because the ABI doc specifies R_X86_64_GOTOFF64 to resolve as:

S + A - GOT

In ld.lld, GOT is start(.got.plt). Hence, we should be resolving R_X86_64_GOTOFF64 as:

S + A - start(.got.plt)

Which is nothing but [4].

[1] - To fix R_X86_64_GOTOFF64 the right way, I really think we need .got and .got.plt to be adjacent as we need negative indices into GOT. May be this is _the_ reason why we should make end(.got) == start(.got.plt). 
[2] - Glibc's X86_64 code uses `extern long _DYNAMIC[] __attribute__((visibility("hidden")));` just like the test in question. However, it refers to only &_DYNAMIC, for which the compiler emits R_X86_64_PC32. Hence, we do not hit the problem with R_X86_64_GOTOFF64. What is broken for glibc without this change is a direct reference to _GLOBAL_OFFSET_TABLE_[0], for which the comiler produces GOTPC32.
[3] - R_X86_64_GOTPC64 is GOT + A - P; R_X86_64_GOTOFF64 is S + A - GOT. The assembly code in the test in question is effectively doing a sum of these two expressions:

  GOT + A - P + S + A - GOT = 2A + S - P

The resulting expressing in the RHS is independent of GOT and hence the test works as expected currently.

PS: If we make end(.got) == start(.got.plt), then we should in principle not require this change. However, to avoid surprises while reading code, and to match what the ABI says in word, this change along with a change to R_X86_64_GOTOFF64 would still be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59559