[PATCH] D69434: ExecutionEngine: add preliminary support for COFF ARM64

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:117
+    const RelocationEntry RE(SectionID, Offset, RelType, Addend);
+    resolveRelocation(RE, Section.getLoadAddressWithOffset(StubOffset));
+
----------------
kaadam wrote:
> mstorsjo wrote:
> > I can't say I entirely understand this function (I understand what it tries to do, but fail to follow exactly how the details fit together), but as I see that it is very similar to the corresponding existing code for x86_64, I presume it's correct.
> Yes, the logic is the same as other COFF targets do. Basically we should handle an external symbol which is so far. 
> 
> So first time when we detected we have an external symbol, generate a stub function (movz/movk [1]). Currently the Value contains the address of the external symbol for the original relocation branch26 (b/bl instruction).  We need to replace the Value here to point to the address of the stub instead of the external symbol. That's why we need to call resolveRelocation for the original relocation, but here we will pass the stub address as Value to it. So when original branch26 relocation is resolved that will point to stub. After that we returned with our internal relocation type and stuboffset, create RelocationEntry for these data and we  can use the original Value for external symbol. Finally we will resolve the relocation for this internal type, the symbol address will be encode the into movz/movk stub instructions.
> 
> all in all we're going to have two jump, bl -> stub -> external symbol. 
> 
> [1]: https://github.com/llvm-mirror/llvm/blob/master/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp#L933
Yes, that much I understand (I implemented the same feature in the lld linker as well) - it was more relating to the exact details of each of the method calls here, but I don't have any concrete question regarding it right now either.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:193
+      Addend = (orig & 0x00FFFFE0) >> 3;
+      orig &= ~(0x00FFFFE0);
+      break;
----------------
You can't do the masking out of the original immediate here; updating `orig` has no effect at all, as that's a local variable.

Handling masking out the old value within `processRelocationRef` feels wrong in general (as this function only inspects and gathers info but doesn't update anything yet), I think this should be in `resolveRelocation`.

So then you can't use `or32le` there in those cases, but more something like ` write32le(P, (read32le(P) & ~Mask) | V);` (which perhaps can warrant a helper function of its own).


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

https://reviews.llvm.org/D69434