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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I am not PPC expert, but approach itself seems probably OK to me. At least I do not have good ideas on how to do what you want much simpler/better atm.
Maybe other reviewers will have something.

My comments/suggestions are below.

Also, do you know how this problem is solved by the GNU linkers?



================
Comment at: ELF/Arch/PPC64.cpp:112
+    return false;
+}
+
----------------
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`.


================
Comment at: ELF/InputFiles.h:128
+  // contain small code model relocations should have their .toc sections sorted
+  // closer to the .got section then files that do not contain any small code
+  // model relocations. Thats because the toc-pointer is defined to point at
----------------
then -> than


================
Comment at: ELF/Writer.cpp:1256
+    // Perform custom sorting of .toc sections.
+    if (!Script->HasSectionsCommand) {
+      assert(Sec->SectionCommands.size() == 1);
----------------
I would return early.

```
if (Script->HasSectionsCommand)
  return;
```


================
Comment at: test/ELF/Inputs/ppc64-sort-small-cm-relocs-Input2.s:24
+.Lfunc_end0:
+	.size	set, .Lfunc_end0-.Lfunc_begin0
+
----------------
Seems `Lfunc_end0` is not used. And I think you do not need `.size set...`
Also seems there are a lot of instructions that are not needed for the test. I think you could remove `stw`, `blr`, `.p2align`, `.abiversion`,
probably others.

That applies to all input files. They are a bit bulky now and it feels can be reduced significantly probably.

One more minor issue: we do not use uppercase in file names I think. Your input files contain `*-Input*`.


================
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
----------------
Do you really need so many inputs? You have 6. Feels to be a bit excessive amount.


================
Comment at: test/ELF/ppc64-sort-small-cm-relocs.s:33
+# RUN:          }                \
+# RUN:       } " > %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
----------------
This can be written in a single line:

```
# RUN: echo "SECTIONS {  .text : { *(.text*) } } " > %t.script
```


================
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
----------------
Can it be just an empty script?


================
Comment at: test/ELF/ppc64-sort-small-cm-relocs.s:78
+# CHECK: 10030100         10030100       10     8         {{.*}}/ppc64-sort-small-cm-relocs.s.tmp2.o:(.toc)
+# CHECK: 10030110         10030110       10     8         {{.*}}/ppc64-sort-small-cm-relocs.s.tmp3.o:(.toc)
+
----------------
Since you're testing the order of files here, I think you can omit addresses, entsize, alignment:

```
# CHECK:  .got
# CHECK:   <internal>:(.got)
# CHECK:  .toc
# CHECK:     {{.*}}/ppc64-sort-small-cm-relocs.s.tmp4.o:(.toc)
...
```

Also, please use `CHECK-NEXT` too.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56920