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

[Ádám Kallai via Phabricator via llvm-commits]

kaadam marked 8 inline comments as done.
kaadam added a comment.

Thanks for the lots of comment, it was really helpful. I update the patch, I enclose a unit_test for this. There are tree test failures (adr, b.cond, secrel.), I'm still looking for the reasons.

Expression 'decode_operand(adr1, 1) = (_const[20:0] - adr1[20:0])' is false: 0x2014 != 0x10014
Expression 'decode_operand(bcond, 1)[23:5] = (_foo - bcond)[20:2]' is false: 0x7ffff != 0x7fff8
Expression '*{4}secrel = _foo - section_addr(COFF_AArch64.obj, .text)' is false: 0xfffeffea != 0x4



================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:117
+    const RelocationEntry RE(SectionID, Offset, RelType, Addend);
+    resolveRelocation(RE, Section.getLoadAddressWithOffset(StubOffset));
+
----------------
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


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:205
+    }
+    default:
+      break;
----------------
mstorsjo wrote:
> Do we need to support reading out the immediate from `INTERNAL_REL_ARM64_LONG_BRANCH26` here? Or as the only place that generates it writes a zero immediate I guess it's not necessary?
Yes, it is not necessary since the Addend is always zero this internal type.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:302
+        assert("Branch target is out of range.");
+      or32le(Target, (PCRelVal & 0x0FFFFFFC) >> 2);
+      break;
----------------
mstorsjo wrote:
> If there actually was a nonzero immediate in the instruction here from before, `or32le` won't do the right thing. We don't handle this in lld right now, but as this code at least tries to read out the immediate further up, it would be good for consistency to actually clear the immediate from the branch instruction here before or'ing in the final value.
Yes, you're right . maybe the immediate for these instruction could be cleared when the addends are decoded.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:338
+             "relocation overflow");
+      add32(Target, RE.Addend);
+    }
----------------
mstorsjo wrote:
> Hmm, where does Value end up added to this one?
> 
> I do see that the existing COFF targets does it the same way, and that code does seem to be used and have a working test, but I don't see how it works. Do you have any clue?
Unfortunately I'm not sure what happens under the hood, but it seems it is correct to use only the addend here, which contains the offset of the item from the beginning of its section in this case.


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

https://reviews.llvm.org/D69434