[PATCH] D103539: WIP: try to repair RISCV handling of paired relocations

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/lib/MC/ELFObjectWriter.cpp:1522-1526
+  // RISC-V requires the relocation to be split up to handle the ADD_SUB split
+  // relocation.  These are required to ensure that symbolic differences are
+  // emitted as paired ADD/SUB relocations.
+  if (!InSet && TargetObjectWriter->getEMachine() == ELF::EM_RISCV)
+    return false;
----------------
jrtc27 wrote:
> Why is this not a hook?
This might make sense to be a hook, however, Im not sure if we need a new hook, I think that there are hooks that give us the necessary information, just need to wire them up.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:396
     return;
+
   MCContext &Ctx = Asm.getContext();
----------------
jrtc27 wrote:
> Please don't "fix" unrelated whitespace
Sure, from a final patch perhaps, but when working on something its very helpful.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:156-161
+      std::advance(iterator, RI);
+
+      Relocs[RI].Type = ELF::R_RISCV_ADD8;
+      Relocs.emplace(iterator, Relocs[RI].Offset, nullptr, ELF::R_RISCV_SUB8, 0,
+                     nullptr, 0);
+
----------------
jrtc27 wrote:
> I'm confused why this doesn't overwrite RI again with an R_RISCV_SUB8?
Because there are two relocations which need to be emitted.  The problem here is that we need a second relocation on a different symbol.  The ideal thing would be that we generate two fixups in the first place.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:160
+      Relocs.emplace(iterator, Relocs[RI].Offset, nullptr, ELF::R_RISCV_SUB8, 0,
+                     nullptr, 0);
+
----------------
jrtc27 wrote:
> This approach is also going to give quadratic complexity. Is there a reason we can't emplace_back? If so, can a stable sort post-processing step not fix that more efficiently (asymptotically)?
Yeah, I suppose we can do an `emplace_back` and sort at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103539