[PATCH] D127581: [ELF] Relax R_RISCV_ALIGN

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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:
----------------
jrtc27 wrote:
> 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.
The R_RISCV_ALIGN case label is specific to RISC-V. The pass is done after scanRelocations, so the code does not fit into the generic code. RelExpr can be split for ALIGN and RELAX but its necessity isn't that high. If just that `HINT` is a bit of misnomer I can rename it separately to `R_RELAX` or `R_RELAX_OR_ALIGN`.


================
Comment at: lld/ELF/Arch/RISCV.cpp:491
+  // 0).
+  SmallVector<int32_t, 0> relocDeltas;
+};
----------------
jrtc27 wrote:
> Should this not be Elf_Sword or similar? ELFCLASS64 _can_ overflow this, even if you really really really shouldn't.
There are precedents using int32_t in many places. Elf_Sword is not used. Since relocations aren't that many, just switched to int64_t.


================
Comment at: lld/ELF/Arch/RISCV.cpp:500
+      sec->relaxAux = make<RISCVRelaxAux>();
+      sec->relaxAux->relocDeltas.resize(sec->relocations.size());
+    }
----------------
jrtc27 wrote:
> 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").
changed to std::unique_ptr<int64_t[]>


================
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) <
----------------
jrtc27 wrote:
> 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).
llvm::sort is fine. The previous comment explains it. Improved the comment a bit.


================
Comment at: lld/ELF/Arch/RISCV.cpp:540
+  llvm::TimeTraceScope timeScope("RISC-V relaxOnce");
+  if (config->relocatable)
+    return false;
----------------
jrtc27 wrote:
> This seems like it belongs in generic code?
This is RISC-V specific. Unless another supported architecture adds relaxation, this can stay here.


================
Comment at: lld/ELF/Arch/RISCV.cpp:554
+
+      // Restore original st_value for symbols relative to this section.
+      ArrayRef<SymbolAnchor> sa = makeArrayRef(aux.anchors);
----------------
jrtc27 wrote:
> 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?
For most symbols, this is the original st_value.
Linker script symbol assignments may rewrite st_value (and does not care about the original value).
The code should be fine.


================
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.
----------------
jrtc27 wrote:
> 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?
I vaguely remember that GNU ld seems to use `nop; c.nop` (prefer long to short). This code should match its behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127581