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

[Jacek Caban via Phabricator via llvm-commits]

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

Thanks for reviews!



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1434
+        if (Error E = COFFObj->getRvaPtr(CHPEMetadata->CodeMap, CodeMapInt))
+          reportError(std::move(E), Obj.getFileName());
+        CodeMap = reinterpret_cast<const chpe_range_entry *>(CodeMapInt);
----------------
MaskRay wrote:
> The error is not tested? For metadata issues, consider whether a warning suffices to continue other dumping functionality.
> 
> We also have a `reportUniqueWarning` now, though a `Dumper` object is required.
Looking closer at this, COFFObjectFile::initLoadConfigPtr already validates it, so we may use cantFail here.


================
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;
----------------
jhenderson wrote:
> 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?
Unless I'm missing something, disassembler doesn't really care about this part of the triple. It usually uses a result of makeTriple, which would be something like "x86_64--". I changed the code to copy the original triple and replace its arch. I think it's cleaner this way and should be safer in case other parts ever matter.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2245
+      if (!X64Target)
+        reportError(Obj->getFileName(), "can't find X86_64 target: " + Error);
+
----------------
jhenderson wrote:
> 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)?
Nice idea, I will include it in the next version. It will also allow testing the triple used.


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

https://reviews.llvm.org/D149095