[PATCH] D149095: [llvm-objdump] Support CHPE code ranges in disassembler.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Unfortunately, my knowledge of COFF-related things is probably insufficient to review chunks of this code, so it would be best if you could get somebody else to review too, maybe @mstorsjo?



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1616
+      DT = &PrimaryTarget;
+      for (uint32_t i = 0; i < CodeMapCount; i++) {
+        if ((CodeMap[i].StartOffset & 3) != CHPE_RANGE_AMD64)
----------------
Nit: the coding standards say to prefer preincrement.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1915
+              if (CodeMapCount && DT != &PrimaryTarget) {
+                // X64 disassebler range may have left Index unaligned, so
+                // make sure that it's aligned when we switch back to ARM64
----------------



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2236
     }
+  } else if (const COFFObjectFile *COFFObj = dyn_cast<COFFObjectFile>(Obj)) {
+    const chpe_metadata *CHPEMetadata = COFFObj->getCHPEMetadata();
----------------
Same comment as @MaskRay's above: use `const auto *` here.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2239
+    if (CHPEMetadata && CHPEMetadata->CodeMapCount) {
+      // Setup x86_64 disassembler for ARM64EC binaries.
+      Triple X64Triple("x86_64-windows-msvc");
----------------
Nit: "Setup" = noun, "Set up" = verb or adjective.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2240
+      // Setup x86_64 disassembler for ARM64EC binaries.
+      Triple X64Triple("x86_64-windows-msvc");
+      std::string Error;
----------------
I'm not particularly up-to-speed with targets and triples, so this question may not make much sense, but does this and the following code mean that this will only work if the code has been built specifically with support for MSVC? What about other windows triples?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2245
+      if (!X64Target)
+        reportError(Obj->getFileName(), "can't find X86_64 target: " + Error);
+
----------------
I wonder if you could add a test case for this which is marked as unsupported if the target IS available (as opposed to the usual way round of it being unsupported if it isn't)?


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

https://reviews.llvm.org/D149095