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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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()) +
----------------
peter.smith wrote:
> 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.
We are on the same page. I said we could "scan relocations earlier" then _GLOBAL_OFFSET_TABLE_ would not be null and that is why I left a "FIXME" there.

But doing that is probably too difficult for me. The current change is imperfect but is compatible with other places. When _GLOBAL_OFFSET_TABLE_ is null, the code makes an arbitrary choice (the same as the old code does): the end of the .got section.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47098