[PATCH] D66419: [RISCV] Implement getExprForFDESymbol to ensure RISCV_32_PCREL is used for the FDE location

[Luís Marques via Phabricator via llvm-commits]

luismarques added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp:41
+  // enabled, so we follow binutils in using the R_RISCV_32_PCREL relocation
+  // for the FDE initial locatin.
+  MCContext &Ctx = Streamer.getContext();
----------------
Typo in "location".


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp:45
+      MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, Ctx);
+  return RISCVMCExpr::create(ME, RISCVMCExpr::VK_RISCV_32_PCREL, Ctx);
+}
----------------
LLVM has dropped support for DWARF64, so this should be correct more or less indefinitely, but this code is implicitly assuming that `Encoding` includes `dwarf::DW_EH_PE_sdata4`. Maybe assert that's the case, to trip if that ever changes?


================
Comment at: llvm/test/MC/RISCV/fde-reloc.s:12
 # RELAX-RELOC:   Section (4) .rela.eh_frame {
-# RELAX-RELOC-NEXT:   0x1C R_RISCV_ADD32 - 0x0
-# RELAX-RELOC-NEXT:   0x1C R_RISCV_SUB32 - 0x0
+# RELAX-RELOC-NEXT:   0x1C R_RISCV_32_PCREL - 0x0
 # RELAX-RELOC-NEXT:   0x20 R_RISCV_ADD32 - 0x0
----------------
The issue doesn't seem to be specific to this patch, but when you insert instructions before `.cfi_startproc` the relocation doesn't seem to be updated correctly, unlike with binutils. Might be worth looking where `EmitPersonality` gets its `Symbol` from.


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

https://reviews.llvm.org/D66419