[PATCH] D127581: [ELF] Relax R_RISCV_ALIGN

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: lld/ELF/Arch/RISCV.cpp:277
   case R_RISCV_ALIGN:
-    // Not just a hint; always padded to the worst-case number of NOPs, so may
-    // not currently be aligned, and without linker relaxation support we can't
-    // delete NOPs to realign.
-    errorOrWarn(getErrorLocation(loc) + "relocation R_RISCV_ALIGN requires "
-                "unimplemented linker relaxation; recompile with -mno-relax");
-    return R_NONE;
+    return R_RELAX_HINT;
   default:
----------------
ALIGN is not a hint, it's a requirement (as opposed to R_RISCV_RELAX, which is a true hint). I would suggest separating these two notions at the RelExpr level, then the config->relax check to skip R_RISCV_RELAX in the CALL(_PLT) patch can instead be done at the generic level rather than in the target.


================
Comment at: lld/ELF/Arch/RISCV.cpp:491
+  // 0).
+  SmallVector<int32_t, 0> relocDeltas;
+};
----------------
Should this not be Elf_Sword or similar? ELFCLASS64 _can_ overflow this, even if you really really really shouldn't.


================
Comment at: lld/ELF/Arch/RISCV.cpp:494
+
+static void getSymbolAnchors() {
+  for (OutputSection *osec : outputSections) {
----------------
A getter that returns nothing is odd, saveSymbolAnchors/recordSymbolAnchors/initSymbolAnchors/similar?


================
Comment at: lld/ELF/Arch/RISCV.cpp:500
+      sec->relaxAux = make<RISCVRelaxAux>();
+      sec->relaxAux->relocDeltas.resize(sec->relocations.size());
+    }
----------------
Do you not just want an `int32_t *` (or smart pointer) given it's either 0 or relocations.size() elements? SmallVector adds overhead as it tracks both size and capacity, but we don't need any dynamic behaviour (beyond "does not exist" (null) and "exists with relocations.size() elements").


================
Comment at: lld/ELF/Arch/RISCV.cpp:523
+    for (InputSection *sec : getInputSections(*osec)) {
+      llvm::sort(sec->relaxAux->anchors, [](auto &a, auto &b) {
+        return std::make_pair(a.offset, a.end) <
----------------
Does this not need to be stable_sort to guarantee the zero-sized symbols have their anchors in the right order? Also, does the order of A's end vs B's start matter for this implementation? That should be documented (and, ideally, why).


================
Comment at: lld/ELF/Arch/RISCV.cpp:540
+  llvm::TimeTraceScope timeScope("RISC-V relaxOnce");
+  if (config->relocatable)
+    return false;
----------------
This seems like it belongs in generic code?


================
Comment at: lld/ELF/Arch/RISCV.cpp:554
+
+      // Restore original st_value for symbols relative to this section.
+      ArrayRef<SymbolAnchor> sa = makeArrayRef(aux.anchors);
----------------
Is this actually the original st_value? If you interleave relaxation with other adjustment of st_value, won't delta stay the same but then the "unrelaxed" st_value will be different?


================
Comment at: lld/ELF/Arch/RISCV.cpp:570
+      for (auto it : llvm::enumerate(sec->relocations)) {
+        Relocation &r = it.value();
+        const size_t i = it.index();
----------------
It might be nice to hoist this out to a separate function, it's quite nested here and this is the bit people care about editing to add new relaxations, so separating the "do the relaxations" part from all the tracking infrastructure would help there.


================
Comment at: lld/ELF/Arch/RISCV.cpp:653
+        // to satisfy the alignment requirement. If `remove` is a multiple of 4,
+        // it is as if we have skipped some NOPs. Otherwise we are in the middle
+        // of a 4-byte NOP, and we need to rewrite the NOP sequence.
----------------
I don't know if it's a requirement that, say, 6 padding bytes be emitted as `nop; c.nop` rather than `c.nop; nop`. Does binutils make this assumption?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127581