[PATCH] D103539: RISCV: adjust handling of relocation emission for RISCV

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

With Diff 352163 of this patch applied, I can build+boot a mainline riscv Linux kernel.
https://github.com/ClangBuiltLinux/linux/issues/1023 is resolved, and further now so is https://github.com/ClangBuiltLinux/linux/issues/1143. This is gets us back to being able to build entirely with LLVM (clang, clang's integrated assembler, LLD). That's huge! Thanks @compnerd !

I also built the kernel with debug info; `llvm-dwarfdump` didn't have any complaints about the input.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:195
+
+  static bool useLinkerRelaxation(MCContext &C, const MCExpr *Value,
+                                  const MCExpr *&LHS, const MCExpr *&RHS) {
----------------
does this need to be `static`?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:208
+    const auto &A = E.getSymA()->getSymbol();
+    const auto &B = E.getSymA()->getSymbol();
+
----------------
Should this call `getSymB()` rather than `getSymA()`?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:230-233
+    if (!useLinkerRelaxation(getContext(), Value, A, B))
+      return MCELFStreamer::emitValueImpl(Value, Size, Loc);
+
+    MCStreamer::emitValueImpl(Value, Size, Loc);
----------------
Should this be:
```
MCStreamer::emitValueImpl(Value, Size, Loc);
if (!useLinkerRelaxation(getContext(), Value, A, B))
  return;
```


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h:105-120
+  // fixup_riscv_add_32 - 32-bit fixup corresponding to R_RIRSCV_ADD32 for
+  // 32-bit symbolic difference paired relocations.
+  fixup_riscv_add_32,
+  // fixup_riscv_sub_32 - 32-bit fixup corresponding to R_RIRSCV_SUB32 for
+  // 32-bit symbolic difference paired relocations.
+  fixup_riscv_sub_32,
+  // fixup_riscv_add_64 - 64-bit fixup corresponding to R_RIRSCV_ADD64 for
----------------
Some of the comments have an extra `R` in the relocation type identifier.


================
Comment at: llvm/test/MC/RISCV/hilo-constaddr-expr.s:4
-# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=-relax %s 2>&1 \
-# RUN:     | llvm-objdump -d - | FileCheck %s --check-prefix=CHECK-INSTR
-# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=-relax %s 2>&1 \
----------------
is it ok to drop the `llvm-objdump -d` test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103539