[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()) +
----------------
MaskRay wrote:
> 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.
Added some comments about 

    return (ElfSym::GlobalOffsetTable
                ? ElfSym::GlobalOffsetTable->getVA()
                : InX::Got->getVA() + InX::Got->getSize()) +
           A - P;

It is very difficult to scan relocations earlier 

ElfSym::GlobalOffsetTable is defined in 

https://github.com/llvm-mirror/lld/tree/got2/ELF/Driver.cpp#L1254
    addReservedSymbols();

scanRelocations is transitively called in Writer<ELFT>::finalizeSections

createSyntheticSections and a bunch of other handlers are called in between and some of them checks ElfSym::GlobalOffsetTable.


I have left a comment here why the choice albeit imperfect, does not cause trouble in practice: because the two relocations S+A-GOT and GOT+A-P  counteract each other.
So an arbitrary address (end of .got) works.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47098