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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Looking pretty good now, just a few details I found. Then this needs tests to get rid of the WIP status.



================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:46
+  write32le(T, orig | ((imm & (0xFFF >> rangeLimit)) << 10));
+//  write32le(T, orig | ((uint32_t)imm << 10) & 0x003FFC00);
+}
----------------
Leftover commented out code?


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:57
+  if ((imm & ((1 << size) - 1)) != 0)
+    assert("misaligned ldr/str offset");
+  write32AArch64Imm(T, imm >> size, size);
----------------
Is there any better error reporting mechanism available here?

The current form of assert is wrong, as the address of the string will evaluate as nonzero and the assert wouldn't trigger. Maybe `assert(0 && "message")` in case there's no better way of actually reporting the issue to the caller.


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


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:171
+      // Get the 26 bit addend encoded in the branch instruction.
+      Addend = ((orig & 0x03FFFFFF) << 2);
+
----------------
Nit: Here and below you have pretty superfluous outer parentheses


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:180
+      uint32_t orig = read32le(Displacement);
+      Addend = (orig & 0x00FFFFF0);
+      break;
----------------
This doesn't seem to be right?

For instructions B.cond and e.g. cbz, you have 5 least significant bits of the instruction being other data than the immediate, and after that, the resulting value should be left shifted by 2.

So here, it should be `Addend = ((orig & 0x00FFFFE0) >> 5) << 2;` (for clarity) or `Addend = (orig & 0x00FFFFE0) >> 3;` (for more straightforward but less obvious code).

Something similar should be done for branch14 below as well.

LLD currently actually ignores the existing immediate in these relocations (which hasn't been an issue so far, but technically is an oversight).


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:205
+    }
+    default:
+      break;
----------------
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?


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:302
+        assert("Branch target is out of range.");
+      or32le(Target, (PCRelVal & 0x0FFFFFFC) >> 2);
+      break;
----------------
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.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:338
+             "relocation overflow");
+      add32(Target, RE.Addend);
+    }
----------------
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?


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

https://reviews.llvm.org/D69434