[PATCH] D82816: [LLD][PowerPC] Implement R_PPC64_REL24_NOTOC calls, callee also has no TOC

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

Patch looks very good Victor. I've left a couple minor comments in the implementation. I have a few suggestions in relation to the testing.

You have done a very thorough job adding tests. Ideally I think we should be able to condense this down to 2 lit tests with  2 input files.

Just to make sure I understand the extent of what we want to test in this patch:

- All calls that use the new NOTOC call relocation that **do not ** need thunks/stubs.
- We want to test both calls and tail calls.
- Have  callee's with both st_other=0 and st_other=1.

I think we can handle that by having one lit test that we link into a executable. You can define what you are calling the local callees (with both st_other=1 and st_other=0) in that file as well as the callers,  and then define what you are calling the externalcalll-samedso in an assembly file in /Inputs/ (we already have a handful of ppc64-* files in there).

I would then have a different lit test which we link into a shared object, where the callees have hidden visibility so thjat we do not need a plt stub to call them (similarly to above define callees in the same file, and other callees in /Input/)



================
Comment at: lld/ELF/Arch/PPC64.cpp:1041
     return false;
 
   // If a function is in the Plt it needs to be called with a call-stub.
----------------
We should probably insert a couple of fatal error here:
1) if the type in NOTOC and the symbols st_other indicates it needs the toc-pointer setup.
2) If the type is not NOTOC but the symbols st_other indicates it tramples the toc.


================
Comment at: lld/ELF/Thunks.cpp:950
+          type == R_PPC64_REL24_NOTOC) &&
          "unexpected relocation type for thunk");
   if (s.isInPlt())
----------------
Insert a fatal error here if the type is `R_PPC64_REL24_NOTOC` with a message indicating notoc thunks are not yet supported.  (With the other fatal_errors I suggested I think only a plt-stub could cause this).


================
Comment at: lld/test/ELF/ppc64-reloc-rel24notoc-externcall-samedso-callee-st-other0.s:29
+# SYMBOL-NEXT: 2: 000000001001001c     0 NOTYPE  LOCAL  DEFAULT [<other: 0x20>]   1 caller2
+# SYMBOL-NEXT: 3: 0000000010010028     0 NOTYPE  LOCAL  HIDDEN                   2 callee
+
----------------
This is a little odd. We define 'callee' with default visibility, then reference it in another object file with hidden visibility, but we are linking an exec so the visibility doesn't really affect the behavior. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82816