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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:1106
                     getLocation(sec, sym, offset));
-      if (!sym.isInPlt())
-        addPltEntry(in.plt, in.gotPlt, in.relaPlt, target->pltRel, sym);
-      if (!sym.isDefined()) {
-        replaceWithDefined(
-            sym, in.plt,
-            target->pltHeaderSize + target->pltEntrySize * sym.pltIndex, 0);
-        if (config->emachine == EM_PPC) {
-          // PPC32 canonical PLT entries are at the beginning of .glink
-          cast<Defined>(sym).value = in.plt->headerSize;
-          in.plt->headerSize += 16;
-          cast<PPC32GlinkSection>(in.plt)->canonical_plts.push_back(&sym);
-        }
-      }
-      sym.needsPltAddr = true;
+      sym.needsCopy = true;
       sec.relocations.push_back({expr, type, offset, addend, &sym});
----------------
As we know that a PLT entry is needed, we can set the flag and simplify `postScanRelocations()` a bit.


================
Comment at: lld/ELF/Relocations.cpp:1612
+      addGotEntry(sym);
+    if (sym.needsPlt && !sym.isInPlt())
+      addPltEntry(in.plt, in.gotPlt, in.relaPlt, target->pltRel, sym);
----------------
The symbol has not been added in PLT yet, right?


================
Comment at: lld/ELF/Relocations.cpp:1618-1620
+        assert(sym.isFunc());
+        if (!sym.isInPlt())
+          addPltEntry(in.plt, in.gotPlt, in.relaPlt, target->pltRel, sym);
----------------
If the suggested change for `processRelocAux()` is applied.


================
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)
----------------
Maybe extract a shortened version of `fn`, so that local symbols do not waste time checking flags that are always `false` for them?


================
Comment at: lld/test/ELF/aarch64-ifunc-bti.s:45
 # CHECK-NEXT:                   nop
-# CHECK-EMPTY:
 ## The address of ifunc2 (STT_FUNC) escapes, so it must have `bti c`.
 # CHECK-NEXT:    103f8:         bti     c
----------------
The comment should be moved before `<ifunc2>:`


================
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
----------------
Do we really need such precise checks in the test?


================
Comment at: lld/test/ELF/ppc32-canonical-plt.s:37
 
 ## Canonical PLT entry of func2.
+## 0x10030318 = 65536*4099+792
----------------



================
Comment at: lld/test/ELF/ppc32-canonical-plt.s:45
 
 ## Canonical PLT entry of func.
+## 0x1003031C = 65536*4099+796
----------------



================
Comment at: lld/test/ELF/ppc32-reloc-got.s:14
 
 # SEC: .got PROGBITS 00020068 020068 000014
 
----------------
This is not checked (and not accurate), thus, can be removed.


================
Comment at: lld/test/ELF/ppc32-reloc-got.s:20
 
 # NM: 10030220 d a
 
----------------



================
Comment at: lld/test/ELF/ppc32-reloc-got.s:24-25
+# HEX:      section '.got':
+# HEX:      0x1002020c 100201a4 00000000 00000000 00000000
+# HEX-NEXT: 0x1002021c {{[0-9a-f]+}}
 
----------------



================
Comment at: lld/test/ELF/ppc32-reloc-got.s:27-28
 
-## a: &.got[3] - _GLOBAL_OFFSET_TABLE_ = 12
-## b: &.got[4] - _GLOBAL_OFFSET_TABLE_ = 16
-# CHECK: lwz 3, 12(30)
-# CHECK: lwz 4, 16(30)
+## a: &.got[4] - _GLOBAL_OFFSET_TABLE_ = 12
+## b: &.got[3] - _GLOBAL_OFFSET_TABLE_ = 16
+# CHECK: lwz 3, 16(30)
----------------



================
Comment at: lld/test/ELF/riscv-reloc-got.s:39-40
 ## .got[0] = _DYNAMIC
 ## .got[1] = a (filled at link time)
 ## .got[2] = 0 (relocated by R_RISCV_64 at runtime)
 # HEX32: section '.got':
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114783