[PATCH] D114783: [ELF] Split scanRelocations into scanRelocations/postScanRelocations

[Peter Smith via Phabricator via llvm-commits]

peter.smith accepted this revision.
peter.smith added a comment.
This revision is now accepted and ready to land.

LGTM, I think this is a good simplification. The test changes all look related to GOT ordering, although I've concentrated my checking on AArch64. Maybe worth widening the reviewer pool to see if there are any alternative opinions.



================
Comment at: lld/ELF/Relocations.cpp:1612
+      } else {
+        if (!sym.isInPlt())
+          addPltEntry(in.plt, in.gotPlt, in.relaPlt, target->pltRel, sym);
----------------
Worth an `assert(sym.isFunc() && !sym.isGnuIFunc())` here?


================
Comment at: lld/ELF/Symbols.h:287
+  uint8_t needsPlt : 1;
+  uint8_t needsCopy : 1;
+  uint8_t hasDirectReloc : 1;
----------------
My initial thought was that `needsCopy` isn't specific enough, although I've struggled to come up with anything better. I think `needsCopyRelOrCanonicalPLT` is the best I could come up with. Similarly for `hasDirectReloc` the best alternative I could think of was `hasNonGotGeneratingReloc`.

Best stick to the names here unless someone can think of a better one. Perhaps worth some comments:
```
// Needs a copy relocation if Object or a Canonical PLT if a non-ifunc function symbol.
uint8_t needsCopy : 1;
// Target of a non-GOT-generating relocation.
uint8_t hasDirectReloc : 1;


================
Comment at: lld/test/ELF/aarch64-cortex-a53-843419-recognize.s:611
         .data
         .globl dat
         .globl dat2
----------------
I was curious why the .got entries were different. As part of the investigation I noticed a couple of things:
The first `.globl dat` has a typo, it should be `.globl dat1` this causes GOT generating relocations to a local symbol which maybe causing some of the differences. The second is that the ADRPs that are notionally paired with ldr xr, [xs, :got_lo12: symbol] should be `adrp xr, :got: symbol` This doesn't matter for the test as we're not executing it, but it may be worth correcting at some point.   


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114783