[PATCH] D113038: [ORC] Add a utility for adding missing "self" relocations to a Symbol

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

We should fix MC to emit the missing relocations, but we'll have a lot of legacy objects to deal with -- this is a great solution for those.

LGTM.



================
Comment at: llvm/lib/ExecutionEngine/Orc/IndirectionUtils.cpp:447
+    LLVM_DEBUG(dbgs() << "Adding delta32 self-relocation at " << InstrStart);
+    B.addEdge(jitlink::x86_64::Delta32, RelocOffInBlock, Sym, /*Addend=*/-4);
+  }
----------------
benlangmuir wrote:
> I wasn't sure if it was worth zeroing out the existing addend in the instruction stream - it would seem to be "nice" to zero it out to ensure we're depending on the relocation instead of the original addend, but it seems to get overwritten completely when we perform the fixup anyway and the contents of the block need to be copied and made mutable to make any changes.
We've never formalized this as a policy but in practice JITLink relocations always overwrite all operand bits, so it should be safe to leave the existing ones in there. I think leaving the bits as-is is the right trade-off.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:412-413
+  Optional<uint64_t>
+  getMemoryOperandRelocationOffset(const MCInst &Inst,
+                                   uint64_t Size) const override;
 };
----------------
My knee-jerk reaction was "what happens if there are multiple memory operands", but we've already got `evaluateMemoryOperandAddress` above so there's some precedent for assuming one memory operand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113038