[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 04:29:39 PDT 2018

peter.smith added a comment.

Hello,

Apologies for the delay in responding, Monday was a public holiday in the UK. I'm not sure if I have much to add over what has been already said. My understanding from when I looked at pr36555:

- Each target has a convention on where _GLOBAL_OFFSET_TABLE_ is placed.
- glibc uses _GLOBAL_OFFSET_TABLE_[0] to find where it has been loading, it expects the link time address of _DYNAMIC. This seems to be the case for all targets.
- glibc reserves entries in the .got.plt to support lazy loading, these seem to be accessed via the dynamic tag DT_PLTGOT rather than relative to _GLOBAL_OFFSET_TABLE_ despite the comment mentioning _GLOBAL_OFFSET_TABLE_[1] and _GLOBAL_OFFSET_TABLE_[2], I suspect that some time ago these used _GLOBAL_OFFSET_TABLE_[0] but was at some point switched to .got.plt[1] and .got.plt[2] with .got.plt[0] possibly used for _GLOBAL_OFFSET_TABLE_[0] if the target sets _GLOBAL_OFFSET_TABLE_ to .got.plt.
- Some targets have a convention of loading the value of _GLOBAL_OFFSET_TABLE_ and accessing variables using some offset from the _GLOBAL_OFFSET_TABLE_.
- LLD initially set _GLOBAL_OFFSET_TABLE_ to 0 and defined some of the relocation expressions from the end of the .got.
  - This works for most programs as the precise value of _GLOBAL_OFFSET_TABLE_ isn't used, as long as the relocations that are calculated relative to it come out consistently. This obviously doesn't work for glibc as it uses _GLOBAL_OFFSET_TABLE_[0] directly.

To fix this problem I think that we need two things:

- _GLOBAL_OFFSET_TABLE_[0] needs to be able to contain the link time address of _DYNAMIC.
  - As George mentions in https://bugs.llvm.org/show_bug.cgi?id=36555#c19 it doesn't matter where _GLOBAL_OFFSET_TABLE_[0] is, as long as we can write a value there at link time.
- The relocations that programs use that are explicitly or implicitly relative to _GLOBAL_OFFSET_TABLE_ need to be consistent with where we place _GLOBAL_OFFSET_TABLE_.
  - Currently the _FROM_END relocations are defined to assume _GLOBAL_OFFSET_TABLE_ is set to the end of the .got section. If we've placed the _GLOBAL_OFFSET_TABLE_ somewhere else then that relocation needs redefining to where we have actually placed it.

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.

================
Comment at: ELF/InputSection.cpp:501
+    // If we scanned relocations earlier, GlobalOffsetTable could't be null.
+    return (GOTBase ? GOTBase->getVA()
+                    : InX::Got->getVA() + InX::Got->getSize()) +
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > What is your plan to fix this `FIXME`? It does not seem nice approach to add weak _GLOBAL_OFFSET_TABLE_ symbols to test cases
> > > which affects the calculation.
> > Changed the description. I think the usage of `InX::Got->getVA() + InX::Got->getSize()` is incorrect. We should use either:
> > 
> > * `ElfSym::GlobalOffsetTable`. but it can be null when `_GLOBAL_OFFSET_TABLE_` is not explicitly referenced.
> > * If `GlobalOffsetTable` is null, use `InX::Got` or `InX::GotPlt` depending on `GotBaseSymInGotPlt`. We can avoid this if we scan relocations earlier and decide to emit `GlobalOffsetTable`.
> > 
> > To properly address this I also need to study the TLS stuff. This revision is already sort of complicated, I want to keep the changes in a future revision
> > If GlobalOffsetTable is null, use InX::Got or InX::GotPlt depending on GotBaseSymInGotPlt. We can avoid this if we scan relocations earlier and decide to emit GlobalOffsetTable.
> 
> I think `GlobalOffsetTable` should never be null here and scanning earlier seems to be the correct way to me. I am not sure if there will be problems or it is easy to do.
> Because of that, I think it would be better to see the full patch. It should reveal the whole picture and get rid of the need to 
> add symbols to the existent test cases.
> 
> 
I think that _GLOBAL_OFFSET_TABLE_ should not be null here in a well formed program (by convention rather than any specific wording in the ABI). It will be possible to create an example with assembly but I don't think that there would be any expectation that it would work. I think it would be better to error rather than make an arbitrary choice of value.

================
Comment at: test/ELF/x86-64-reloc-shared.s:17
+R_X86_64_GOTPC64:
+  movabsq $_GLOBAL_OFFSET_TABLE_-., %r11
----------------
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.

Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47098