[PATCH] D86706: [LLD][PowerPC] Add pc-rel based long branch thunks

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: lld/ELF/Thunks.cpp:966
+void PPC64PCRelLongBranchThunk::writeTo(uint8_t *buf) {
+  int64_t offset = destination.getVA(addend) - getThunkTargetSym()->getVA();
+  if (!isInt<34>(offset))
----------------
I don't believe we have accounted for the addend in the other PPC64 thunks (both toc-based and pc-rel based). If we do take into account the addend then we have to account for that in 'isCompatableWith' as well.  I don't believe the compilers emit calls instructions using both symbol and addends (or we likely would have realized this earlier). I suggest to keep it simple we follow the example of the other thinks and omit the addend, or potentially error on a non-zero addend. We can then revisit this in a subsequent patch where we fix all the thunks that will need to account for the addend. 


================
Comment at: lld/ELF/Thunks.cpp:993
+
+void PPC64PCRelLongBranchThunk::writeTo(uint8_t *buf) {
+  int64_t offset = in.ppc64LongBranchTarget->getEntryVA(&destination, addend) -
----------------
NeHuang wrote:
> nemanjai wrote:
> > NeHuang wrote:
> > > sfertile wrote:
> > > > The R12Setup stub calculates the address of the callee relative to the program counter.
> > > > 
> > > > ```
> > > >   writePrefixedInstruction(buf + 0, paddi); // paddi r12, 0, func at pcrel, 1
> > > >   write32(buf + 8, MTCTR_R12);              // mtctr r12
> > > >   write32(buf + 12, BCTR);                  // bctr
> > > > ```
> > > > 
> > > > The toc-based long branch thunks have to use a table in the data segment because of limited address-ability. The pc-rel instructions and ABI doesn't have the same limitation, so why use the branch linkage table intead of doing the same thing the R12Setup stubs do?
> > > Good point! Will update the patch without using branch_lt table. 
> > Is it not possible to need a long branch to a destination that isn't available yet? I thought that something like the following would require a long branch to a PLT stub:
> > 
> > ```
> > #include <stdio.h>
> > int main(int argc, const char **argv) {
> >   int a = argc;
> >   printf("a = %d\n", a);
> >   asm (".space 134217728");
> >   printf("a = %d\n", a);
> > }
> > ```
> > But that seems to create two PLT stubs. Is this something that always happens? Namely, is it always the case that the linker will never create a thunk that goes to another thunk? If so, I think a comment explaining that might be useful.
> IIUC we only create a long branch thunk between a caller and callee and we check if there is a valid RelType (R_PPC64_REL14, R_PPC64_REL24, R_PPC64_REL24_NOTOC) in `PPC64::needsThunk` 
> is it always the case that the linker will never create a thunk that goes to another thunk.
[[ https://github.com/llvm/llvm-project/blob/a1bc37c9e54e0163bc6ccb7a438a68047310ccff/lld/ELF/Relocations.cpp#L1880 | getThunk]] looks at the symbol referenced by the relocation and sees if an existing compatible thunk can be reused, but if none fit the cirteria it creates a new thunk targeting the symbol rather then creating a thunk that targets another thunk. 

> Is it not possible to need a long branch to a destination that isn't available yet?
No: all the address sensitive content has been laid out and finalized by the time the thinks `writeTo` member function is called. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86706