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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Looks sensible overall, except for two relocations that have incorrect interpretations. You seem to be missing a couple relocation types as well; check lld/COFF/Chunks.cpp for reference on how to handle the rest of them. Some of them are not very probable to use in JITed code (as TLS code requires space for the variables to be allocated in the TLS section by the system's runtime loader; the SECREL_LOW/HIGH_12A/L relocations are used for that), but most of the remaining ones are pretty straightforward to handle anyway.



================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:169
+      // therefore implicit (<< 12).
+      Addend = ((*p & 0x60000000) >> 29) | ((*p & 0x01FFFFE0) >> 3) << 12;
+      Addend = SignExtend64(Addend, 33);
----------------
No, this is wrong - and this is one of the less obvious details.

If you have an addend stored in the instruction pointed to by `IMAGE_REL_ARM64_PAGEBASE_REL21`, the addend is expressed in bytes, not in 4096 byte pages.

Consider you have a symbol close to the end of a page, and you want to express an offset by a few bytes (less than a page), that makes the pointed to location in another page.

If the addend would express a number of pages (as this patch expects right now), the addend here would be zero, and you'd end up with this part of the instruction pair pointing at the wrong page.

Therefore, the immediate stored in the instruction before handling relocation is expressed as a number of bytes, even though it means a number of pages after the relocation is done and the instruction is executed.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:175
+    case COFF::IMAGE_REL_ARM64_PAGEOFFSET_12L: {
+     // Verify that the relocation points to one of the expected load / store
+      // or add / sub instructions.
----------------
Nit: The indentation of the first comment line is off here


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:180
+              ((*p & 0x11C00000) == 0x11000000)   ) &&
+             "Expected load / store  or add/sub instruction.");
+
----------------
Nit: Double spaces between "store" and "or"


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:188
+      int ImplicitShift = 0;
+      if ((*p & 0x3B000000) == 0x39000000) { // << load / store
+        // For load / store instructions the size is encoded in bits 31:30.
----------------
I guess this check could also be for the relocation type `IMAGE_REL_ARM64_PAGEOFFSET_12L`?


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:210
+                    << " RelType: " << RelTypeName << " TargetName: "
+                    << TargetName << " Addend " << Addend << "\n");
+
----------------
Would it make more sense, stylistically, to extend the ifdef around the debug statement as well? Right now it does look weird to have code referring to variables that don't exist (even though LLVM_DEBUG will make them disappear).


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:220
+          TargetSectionID = *TargetSectionIDOrErr; 
+          }
+      else
----------------
The indentation here is weird. Please run `clang-format-diff -style LLVM` on the changes.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:243
+      case COFF::IMAGE_REL_ARM64_ADDR64: {
+        or32le(Target + 12, ((Value + RE.Addend) & 0xFFFF) << 5);
+        or32le(Target + 8, ((Value + RE.Addend) & 0xFFFF0000) >> 11);
----------------
This doesn't seem right. `IMAGE_REL_ARM64_ADDR64` is a plain 64 bit integer (just like `IMAGE_REL_ARM64_ADDR32NB`), not a series of instructions that should get immediates added.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:287
+          }
+        }
+        // Compensate for implicit shift.
----------------
This switch for checking for alignment is rather verbose - would it make sense to condense it down to a single expression for all alignments?


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:330
+#endif
\ No newline at end of file

----------------
Missing newline at end of file


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69434