[PATCH] D47098: [ELF] Make R_GOTONLY_PC_FROM_END R_GOTREL_FROM_END relative to _GLOBAL_OFFSET_TABLE_ instead of end of .got

Tue May 29 05:16:20 PDT 2018

grimar added a comment.

Thanks, Peter!

In https://reviews.llvm.org/D47098#1114534, @peter.smith wrote:

> I can think of two possible fixes that satisfy the conditions:
>
> - Keep the _FROM_END relocations and define _GLOBAL_OFFSET_TABLE_ to be the end of the .got section on x86, extend the .got by one word so that _GLOBAL_OFFSET_TABLE_[0] can contain the link time address of _DYNAMIC. This sounds a bit hacky to me, but I think it would work.
> - Redefine the relocations that use _FROM_END to use the location of the _GLOBAL_OFFSET_TABLE_ symbol (or where we have defined it to be) rather than hard code the end of the .got section.
>
>   Personally I'd favour the second approach as it looks like the _FROM_END relocations are a vestige of when _GLOBAL_OFFSET_TABLE_ was set to 0 and the value was never used. I'm not expert in x86 though so it would be best to get as many opinions as possible here.

I am not sure that the second way should work (if I understand everything correctly). 
For x86 as you know we define _GLOBAL_OFFSET_TABLE_  in the .got.plt[0] now. 
Then for example if we have the following TLS code:

  addl $_GLOBAL_OFFSET_TABLE_, %ebx #add    $0x199f,%ebx, R_386_GOTPC
  subl $8, %esp                                         #sub    $0x8,%esp
  leal gd at tlsgd(%ebx), %eax                    #lea    -0x10(%ebx),%eax, R_386_TLS_GD

it expects the first line to be resolved to the end of .got (to the GOT base in ABI terminology).
If we redefine the relocations that use _FROM_END to use the location of the _GLOBAL_OFFSET_TABLE_ (I think that is what this patch tried to do),
then it will not help us I think as we will end up with the address of .got.plt at the first line. And the code will fail to find a proper TLS slot as a result since .got.plt
can be far away from .got in LLD.

So we seem to have 2 problems actually:

1. The current code does not relocate correctly and does not produce valid value when_GLOBAL_OFFSET_TABLE_ symbol is involved. (That would be fixed with second way).
2. _GLOBAL_OFFSET_TABLE_  is expected by existent code to point to GOT base (end of .got). (Second way does not help here) And at the same time _GLOBAL_OFFSET_TABLE_ [0] should contain _DYNAMIC for glibc.

These above issues should be both solved with the first way (writing _DYNAMIC at the last .got slot and pointing _GLOBAL_OFFSET_TABLE_ to that slot).
If we do that then we probably will probably need to make _FROM_END use the location of the _GLOBAL_OFFSET_TABLE still for nice code, but it has to be in .got first of all I think.

================
Comment at: test/ELF/x86-64-reloc-shared.s:17
+R_X86_64_GOTPC64:
+  movabsq $_GLOBAL_OFFSET_TABLE_-., %r11
----------------
peter.smith wrote:
> grimar wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > What I see is that llvm-mc produce following relocations here for me:
> > > > 
> > > > ```
> > > > 000000000003  000300000002 R_X86_64_PC32     0000000000000000 _GLOBAL_OFFSET_TABLE_ - 4
> > > > 000000000009  00030000001d R_X86_64_GOTPC64  0000000000000000 _GLOBAL_OFFSET_TABLE_ + 2
> > > > ```
> > > > 
> > > > So R_X86_64_GOTPC32 seems to be uncovered be this test.
> > > `R_X86_64_GOTPC32` is a special case of `R_X86_64_PC32` where the symbol is `_GLOBAL_OFFSET_TABLE_`. GNU as emits `R_X86_64_GOTPC32` but llvm-mc emits `R_X86_64_PC32`. I have also tested GNU as 
> > If llvm-mc is unable to generate inputs for the test, you can use yaml2obj to craft objects.
> > (For example, see `ELF\invalid\invalid-relocation-x64.test`)
> > 
> > But currently the R_X86_64_GOTPC32 is untested by your test case and that is not OK.
> Arm has a similar discrepancy between GNU as and llvm-mc. GNU as will use R_ARM_BASE_PREL if there is a relative relocation to _GLOBAL_OFFSET_TABLE_, this is useful as the relocation doesn't depend on the value of _GLOBAL_OFFSET_TABLE_. I submitted https://reviews.llvm.org/D46319 to see if I could get this fixed for Arm but I didn't get any response for it.
Yeah, I did not mean we need to fix llvm-mc, but at least I think we should test the particular relocation with use of yaml2obj then.

Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47098