[PATCH] D142879: [RISCV] Emit relocation for uleb128

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:277
+  const MCExpr *A, *B;
+
+  if (!requiresFixups(getContext(), Value, A, B))
----------------
delete the blank line. The prevailing LLVM style does not prefer inserting a blank line after a declaration.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:287
+    // by linker later.
+    IntValue = -1ll;
+  }
----------------
jrtc27 wrote:
> Why not just use encodeULEB128's PadTo and encode 0?
-1ll is not tested. This needs a test with `llvm-readelf -x $name` or `llvm-objdump -s` dumping the section content.


================
Comment at: llvm/test/MC/RISCV/fixups-expr.s:30
 .byte G2-G1
+.uleb128 .L2-.L1
+.uleb128 G2-G1
----------------
Suggest: create a new file for `.uleb128`. Add a test where `.uleb128` precedes the label definition (it currently crashes).

Add a test with labels defined relative to two different sections.

I don't have a very strong opinion about whether non-local symbols should be allowed. For psABI, it may be safer to start with more restriction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142879