[PATCH] D56920: [PPC64] Sort .toc sections accessed with small code model relocs close to the .got

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 9 inline comments as done.
sfertile added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:112
+    return false;
+}
+
----------------
MaskRay wrote:
> MaskRay wrote:
> > grimar wrote:
> > > This does not seem to be clang-formatted.
> > > Also, since you only have 3 entries here, probably it will be better to use `if` instead of `switch`.
> > Using a `switch` for 3 entries looks good to me. I guess this is an incomplete list (but enough to make some not-so-small applications able to link against `libgcc.a`). If it is indeed incomplete, this code deserves a comment.
> > 
> > I've checked some powerpc64le target which failed to link due to
> > `libgcc/libgcc2.c:1444: relocation R_PPC64_ADDR16_DS out of range: 37088 is not in [-32768, 32767]`
> > The relocation out-of-range issues goes away with this patch.
> > 
> > "Power Architecture 64-bit ELF V2 ABI Specification" published on July 16, 2015 does not give examples what relocation types may be used in small/medium/large code models... The x86-64 ABI gives a few regarding PIC/non-PIC * small/medium/large code model..
> There are indeed a few other relocation types that need to be added. They can be added in another patch, though.
> 
> gold
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=dff021e14a9c72380311c15a90c1a646b179b987;f=gold/powerpc.cc#l7291
> 
> ld.bfd
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=dff021e14a9c72380311c15a90c1a646b179b987;f=bfd/elf64-ppc.c#l4395
> and line 4535
> 
> ```c
> 	  if (r_type == R_PPC64_GOT_TLSLD16
> 	      || r_type == R_PPC64_GOT_TLSGD16
> 	      || r_type == R_PPC64_GOT_TPREL16_DS
> 	      || r_type == R_PPC64_GOT_DTPREL16_DS
> 	      || r_type == R_PPC64_GOT16
> 	      || r_type == R_PPC64_GOT16_DS)
> 	    {
> 	      htab->do_multi_toc = 1;
> 	      ppc64_elf_tdata (abfd)->has_small_toc_reloc = 1;
> 	    }
> ```
Thanks for looking into this, glad to hear it fixes the libgcc related relocation overflows. 
I intend to add the tls related relocations as follow on patches, 1 for the GOT related ones, then a separate patch for R_PPC_TPREL16 and R_PPC64_TPREL16_DS (which I seemed to have missed when adding tls support)


================
Comment at: test/ELF/ppc64-sort-small-cm-relocs.s:6
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %p/Inputs/ppc64-sort-small-cm-relocs-Input3.s -o %t4.o
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %p/Inputs/ppc64-sort-small-cm-relocs-Input4.s -o %t5.o
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %p/Inputs/ppc64-sort-small-cm-relocs-shared-defs.s -o %t6.o
----------------
grimar wrote:
> Do you really need so many inputs? You have 6. Feels to be a bit excessive amount.
I've cut it down to 2 inputs with small code model relocations and 2 without (mainly to show the sorting is meant to be stable).


================
Comment at: test/ELF/ppc64-sort-small-cm-relocs.s:38
+# Default sort if the linker script does not have a sections command.
+# RUN: echo "foo = 0x22;" > %t.script
+# RUN: ld.lld %t1.o %t2.o %t3.o %t4.o %t5.o %t.so -o %t -script %t.script -Map=%t.map
----------------
grimar wrote:
> Can it be just an empty script?
Thank you, I didn't realize an empty script was valid.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56920