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

Sun Dec 12 12:43:35 PST 2021

MaskRay added inline comments.

================
Comment at: lld/ELF/Relocations.cpp:1639-1640
+
+  // Local symbols may need the aforementioned non-preemptible ifunc and GOT
+  // handling. They don't need regular PLT.
+  for (InputFile *file : objectFiles)
----------------
ikudrin wrote:
> Maybe extract a shortened version of `fn`, so that local symbols do not waste time checking flags that are always `false` for them?
A local symbol does not need to check `needsCopy`, but the majority of checks (non-preemptible ifunc/GOT/PLT) are possible.

================
Comment at: lld/test/ELF/aarch64-cortex-a53-843419-recognize.s:611
         .data
         .globl dat
         .globl dat2
----------------
peter.smith wrote:
> 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.   
Fixed the typo in a separate commit. The differences disappeared.

================
Comment at: lld/test/ELF/arm-branch-undef-weak-plt-thunk.s:30
 // CHECK: <__ARMv7ABSLongThunk_undefined_weak_we_expect_a_plt_entry_for>:
-// CHECK-NEXT:    201ec:        30 c2 00 e3     movw    r12, #560
+// CHECK-NEXT:    201ec:        40 c2 00 e3     movw    r12, #576
 // CHECK-NEXT:    201f0:        02 c2 40 e3     movt    r12, #514
----------------
ikudrin wrote:
> Do we really need such precise checks in the test?
No, this just keeps the state as is. It would be nice if `movt/movw` can symbolize the target then we can avoid the immediates.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114783