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

Ádám Kallai via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 29 09:58:30 PDT 2019


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

Martin, thanks for your thoughts. I'm on to fix them.



================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:169
+      // therefore implicit (<< 12).
+      Addend = ((*p & 0x60000000) >> 29) | ((*p & 0x01FFFFE0) >> 3) << 12;
+      Addend = SignExtend64(Addend, 33);
----------------
mstorsjo wrote:
> 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.
Thanks for explaining this. It seems I misunderstood this behavior. To be honest I looked at how MachOAArch64 does, how they decode the addend, I compared to aarch64 reference manual to see how it works, it seemed to me I can do the same thing as MachO does.


================
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.
----------------
mstorsjo wrote:
> I guess this check could also be for the relocation type `IMAGE_REL_ARM64_PAGEOFFSET_12L`?
It is just for IMAGE_REL_ARM64_PAGEOFFSET_12L, we need to determine the shift value only for load/store, since for instructions ADD/ADDS (immediate) we should use zero shift.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFAArch64.h:210
+                    << " RelType: " << RelTypeName << " TargetName: "
+                    << TargetName << " Addend " << Addend << "\n");
+
----------------
mstorsjo wrote:
> 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).
Yes, you're right, I will fix it. 


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


================
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);
----------------
mstorsjo wrote:
> 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.
Yes, you're right. Currently I handle a long branch instruction with this relocation, so when I needed to create stub function (which generates movz/movk instruction), I  just set this relocation to detect this case when we have an external symbol. When I tested it with a small examples and Swithshader It worked because I haven't got this relocation type. Of course I should use another one, maybe an internal type.
What do you think?


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69434





More information about the llvm-commits mailing list