[PATCH] D73211: [RISCV] Fix evaluating %pcrel_lo against global and weak symbols

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

This approach seems much easier to understand.

Can you add a testcase for the case where the auipc and addi are in different fragments, like I mentioned in D71978 <https://reviews.llvm.org/D71978>?



================
Comment at: llvm/include/llvm/MC/MCAsmBackend.h:114
+                                   const MCFixup &Fixup, const MCFragment *DF,
+                                   MCValue &Target, uint64_t &Value,
+                                   bool &WasForced) {
----------------
Can the "Target" be a const reference?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:279
+    const MCExpr *AUIPCExpr = AUIPCFixup->getValue();
+    if (!AUIPCExpr->evaluateAsRelocatable(AUIPCTarget, &Layout, AUIPCFixup))
+      return true;
----------------
MCAssembler::evaluateFixup emits an error if evaluateAsRelocatable fails... maybe worth adding a comment if you're intentionally not emitting a similar error?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:283
+    if (const MCSymbolRefExpr *RefB = Target.getSymB()) {
+      if (RefB->getKind() != MCSymbolRefExpr::VK_None)
+        return true;
----------------
If "Target.getSymB()" is true, should getPCRelHiFixup fail?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:323
+
+  if (shouldForceRelocation(Asm, Fixup, Target)) {
+    WasForced = true;
----------------
`shouldForceRelocation(Asm, AUIPCFixup, AUIPCTarget)`?  (I think the difference is significant; please make sure we have a testcase that covers it.)


================
Comment at: llvm/test/MC/RISCV/pcrel-fixups.s:86
+
+	.weak	global_function
+	.type	global_function, at function
----------------
Did you mean to make both global_function and weak_function weak?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73211