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

[jacek]

Rebased on top of other changes. The new version uses yaml2obj for tests. It also contains additional alignment fixup on switch from x86_64 to aarch64 mode.

[jacek]

Rebased.

[MaskRay]

(I'll go on a trip for a few days and won't look at this change. Land if jhenderson approves:) .)

llvm/tools/llvm-objdump/llvm-objdump.cpp

1422

This is an instance that auto is obvious https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

1427

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.

1638

FWIW, I am going to change MappingSymbols in D156190

[jhenderson]

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?

llvm/tools/llvm-objdump/llvm-objdump.cpp

1626

Nit: the coding standards say to prefer preincrement.

1923

2255

Same comment as @MaskRay's above: use const auto * here.

2258

Nit: "Setup" = noun, "Set up" = verb or adjective.

2259

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?

2264

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)?

[jacek]

Thanks for reviews!

llvm/tools/llvm-objdump/llvm-objdump.cpp

1427

Looking closer at this, COFFObjectFile::initLoadConfigPtr already validates it, so we may use cantFail here.

2259

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.

2264

Nice idea, I will include it in the next version. It will also allow testing the triple used.

[jhenderson]

Only nits left from me, but someone with more familiarity with this stuff should review it.

llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml

140

llvm/tools/llvm-objdump/llvm-objdump.cpp

1423

Unlike the previous line, this ISN'T (in my opinion) a case where we should use auto. It's not obvious at the callsite that CHPEMetadata is of type chpe_metadata to me, because getCHPEMetadata() could return any type (in particular, it could easily be Expected<chpe_metadata *> if there was a chance for errors being reported etc. The reason you get away with it in the previous line is because the dyn_cast clearly shows the type being cast to.

2256

No auto here too

2263

Ditto.

[jacek]

I rebased the patch on top of D156190. While doing so, I changed a bit how it's handled, I think it matches better the new code. I created D156622 with a bit more refactoring for mapping symbols and this patch uses separated 'A' and 'X' labels.

[jhenderson]

No real objections from me to the latest version. I'll put a LGTM, but it should get reviewed by someone more familiar with CHPE and related things before you land it.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1429

It might be worth either replacing the "3" literal here and below with a named constant, or at least adding a comment explaining why "3" for those not familiar with the area.

[mstorsjo]

Sorry for the slow reaction on this one... It looks reasonable to me too, even if I'm not very familiar with the CHPE stuff. Looping in @efriedma as well, who might be familiar with it, in case he has something to add - otherwise I guess this is good to go, with @jhenderson and @MaskRay's reviews.

[MaskRay]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1122

Why is this islower change?

[jacek]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1122

CHPE ranges, marked with upper letters, can cross section section boundaries. I will add a comment to clarify that.

1429

I added a constant and moved that logic to chpe_range_entry itself. If it looks fine to you, I will follow up and use it in llvm-readobj, where we have another instance of it.

[efriedma]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1122

Please don't use ctype.h routines; they can be locale-sensitive in some cases, which can lead to weird results. StringExtras.h has alternative routines you can use. (It looks like there isn't an islower replacement, but feel free to add it if you need it.)

[jhenderson]

I'm off for the next few days, so I'm happy with this once the islower issue has been addressed (in a separate patch) and my comments addressed (in this patch).

llvm/include/llvm/Object/COFF.h

748 ↗

(On Diff #545841)

You should explicitly assign these values, as their value is significant, it seems. Also, unless these names are spec-defined (I'm guessing they might be), they should follow the LLVM coding standards for enums (see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

754 ↗

(On Diff #545841)

[jhenderson]

Oh, the pre-merge check is failing on Windows too for a test from this patch.

llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml

141

Nit: it's generally preferable to use different temp file names for different cases in the same test file. That way, later test cases won't overwrite the test input created for an earlier part of the test, which is useful for test debugging purposes. I suggest naming them %t1, %t2 etc, or e.g. %t-invalid for this case.

[jacek]

Thanks for reviews.

llvm/include/llvm/Object/COFF.h

748 ↗

(On Diff #545841)

This will require changing llvm-readobj, so I moved COFF.h changes to D156797.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1122

I created D156796 with new helpers and will use it here.

[jacek]

Also changed temp file names in the second test file.

[MaskRay]

Nit about a test

llvm/test/tools/llvm-objdump/COFF/arm64ec-unsupported.yaml

1 ↗

(On Diff #546263)

This is uncommon and will make many build configurations not able to detect this issue.

Keep just # REQUIRES: aarch64-registered-target and use

RUN: %if !x86-registered-target %{ ... %}

so that most RUN lines in this test are still testable on a default configuration that builds all non-experimental targets.

[jacek]

In D149095#4555237, @MaskRay wrote:

Nit about a test

Oh, nice, I didn't know it's possible. It even allows using a single file instead of duplicating most of it. I will change it, thanks!

[jacek]

After landing D156622, it came out that it was wrong because mappings from different sections may share VA ranges. I think I should give up trying to share MappingSymbols with CHPE and instead just use a separated mechanism.

[jhenderson]

Windows pre-merge CI is still failing with a test related to this patch.

[jacek]

Fixed test, sorry for missing that earlier.

[jhenderson]

LGTM, thanks. I think it would be a good idea for @MaskRay to take another look given the fairly significant changes that happened since his approval.

[MaskRay]

llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml

143

When testing warning/error, we generally start with error: and omit the part before it.

FileCheck --check-prefixes=ERR %s -DFILE=%t-invalid

[jacek]

I changed the test and pushed. Thanks for reviews!

[DavidSpickett]

The new parts of the test failed on Linaro's Windows on Arm bot, so I've made the checks a bit less strict: https://github.com/llvm/llvm-project/commit/40ee8abee77a2e8fb0089d4c7f5723b71f27d416

[DavidSpickett]

Also if future commits could include a little bit about what CHPE is, it would be helpful for people triaging commits. Looks like an architecture feature just from the title.

[jacek]

Thanks and sorry for the problem. I should have changed it when I changed error test. I will try to improve commit messages in the future.

llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml

 # REQUIRES: aarch64-registered-target
 
 ## Check that AMD64 image file with CHPE data is recognized as ARM64EC.
-# RUN: yaml2obj %s -o %t -DMACHINE=IMAGE_FILE_MACHINE_AMD64
-# RUN: llvm-objdump -d %t | FileCheck --check-prefixes=DISASM,ARM64EC %s
-# RUN: llvm-readobj --coff-load-config %t | FileCheck --check-prefix=CODEMAP %s
+# RUN: yaml2obj --docnum=1 %s -o %t1 -DMACHINE=IMAGE_FILE_MACHINE_AMD64
+# RUN: llvm-readobj --coff-load-config %t1 | FileCheck --check-prefix=CODEMAP %s
+# RUN: %if x86-registered-target %{                                                           \
+# RUN:   llvm-objdump -d %t1 | FileCheck --check-prefixes=DISASM,ARM64EC %s                   \
+# RUN: %} %else %{                                                                            \
+# RUN:   llvm-objdump -d %t1 2>&1 | FileCheck --check-prefixes=DISASM-NO-X64,ARM64EC,WARN1 %s \
+# RUN: %}
 
 ## Check that ARM64 image file with CHPE data is recognized as ARM64X.
-# RUN: yaml2obj %s -o %t -DMACHINE=IMAGE_FILE_MACHINE_ARM64
-# RUN: llvm-objdump -d %t | FileCheck --check-prefixes=DISASM,ARM64X %s
-# RUN: llvm-readobj --coff-load-config %t | FileCheck --check-prefix=CODEMAP %s
+# RUN: yaml2obj --docnum=1 %s -o %t2 -DMACHINE=IMAGE_FILE_MACHINE_ARM64
+# RUN: llvm-readobj --coff-load-config %t2 | FileCheck --check-prefix=CODEMAP %s
+# RUN: %if x86-registered-target %{                                                          \
+# RUN:   llvm-objdump -d %t2 | FileCheck --check-prefixes=DISASM,ARM64X %s                   \
+# RUN: %} %else %{                                                                           \
+# RUN:   llvm-objdump -d %t2 2>&1 | FileCheck --check-prefixes=DISASM-NO-X64,ARM64X,WARN1 %s \
+# RUN: %}
+
+## Check handling of an explicit, non-default triple.
+# RUN: %if !x86-registered-target %{                                              \
+# RUN:   llvm-objdump --triple arm64ec-w64-windows-gnu -d %t1 2>&1                \
+# RUN:                | FileCheck --check-prefixes=DISASM-NO-X64,ARM64EC,WARN2 %s \
+# RUN: %}
 
 # ARM64EC: file format coff-arm64ec
 # ARM64X:  file format coff-arm64x
 
-# DISASM:      180001000: 52800040     mov     w0, #0x2
+# WARN1: llvm-objdump: warning: '{{.*}}': unable to get target for 'x86_64--', see --version and --triple.
+# WARN2: llvm-objdump: warning: '{{.*}}': unable to get target for 'x86_64-w64-windows-gnu', see --version and --triple.
+
+# DISASM:      Disassembly of section .text:
+# DISASM-EMPTY:
+# DISASM-NEXT: 0000000180001000 <.text>:
+# DISASM-NEXT: 180001000: 52800040     mov     w0, #0x2
 # DISASM-NEXT: 180001004: d65f03c0     ret
 # DISASM-NEXT: ...
-# DISASM:      180002020: 528000a0     mov     w0, #0x5
+# DISASM-NEXT: 180001020: b8 03 00 00 00     movl    $0x3, %eax
+# DISASM-NEXT: 180001025: c3                 retq
+# DISASM-EMPTY:
+# DISASM-NEXT: Disassembly of section .test:
+# DISASM-EMPTY:
+# DISASM-NEXT: 0000000180002000 <.test>:
+# DISASM-NEXT: 180002000: b8 06 00 00 00     movl    $0x6, %eax
+# DISASM-NEXT: 180002005: c3                 retq
+# DISASM-NEXT: 180002006: cc                 int3
+# DISASM-NEXT: ...
+# DISASM-NEXT: 180002020: 528000a0     mov     w0, #0x5
 # DISASM-NEXT: 180002024: d65f03c0     ret
 
+# DISASM-NO-X64:      Disassembly of section .text:
+# DISASM-NO-X64-EMPTY:
+# DISASM-NO-X64-NEXT: 0000000180001000 <.text>:
+# DISASM-NO-X64-NEXT: 180001000: 52800040     mov     w0, #0x2
+# DISASM-NO-X64-NEXT: 180001004: d65f03c0     ret
+# DISASM-NO-X64-NEXT: ...
+# DISASM-NO-X64-NEXT: 180001020: 000003b8     udf     #0x3b8
+# DISASM-NO-X64-NEXT: 180001024: 00 c3        <unknown>
+# DISASM-NO-X64-EMPTY:
+# DISASM-NO-X64-NEXT: Disassembly of section .test:
+# DISASM-NO-X64-EMPTY:
+# DISASM-NO-X64-NEXT: 0000000180002000 <.test>:
+# DISASM-NO-X64-NEXT: 180002000: 000006b8     udf     #0x6b8
+# DISASM-NO-X64-NEXT: 180002004: 00ccc300     <unknown>
+# DISASM-NO-X64-NEXT: ...
+# DISASM-NO-X64-NEXT: 180002020: 528000a0     mov     w0, #0x5
+# DISASM-NO-X64-NEXT: 180002024: d65f03c0     ret
+
 # CODEMAP:      CodeMap [
 # CODEMAP-NEXT:   0x1000 - 0x1008  ARM64EC
 # CODEMAP-NEXT:   0x1020 - 0x2007  X64
 # CODEMAP-NEXT:   0x2020 - 0x2028  ARM64EC
 # CODEMAP-NEXT: ]
 
 --- !COFF
 OptionalHeader:
StructuredData:
       - UInt32: 0x1001  # CodeMap[0]
       - UInt32: 0x8
       - UInt32: 0x1022  # CodeMap[1]
       - UInt32: 0x0fe7
       - UInt32: 0x2021  # CodeMap[2]
       - UInt32: 0x8
 symbols:         []
 ...
+
+## Check error handling of invalid code map RVA.
+# RUN: yaml2obj --docnum=2 %s -o %t-invalid
+# RUN: not llvm-objdump -d %t-invalid 2>&1 | FileCheck --check-prefixes=ERR %s -DFILE=%t-invalid
+# ERR: error: '[[FILE]]': RVA 0x6000 for CHPE code map not found
+
+--- !COFF
+OptionalHeader:
+  ImageBase:        0x180000000
+  SectionAlignment: 4096
+  FileAlignment:    512
+  DLLCharacteristics: [ ]
+  LoadConfigTable:
+    RelativeVirtualAddress: 0x3000
+    Size:                   320
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
+sections:
+  - Name:            .rdata
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  0x3000
+    VirtualSize:     328
+    StructuredData:
+      - LoadConfig:
+          CHPEMetadataPointer: 0x180004000
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    VirtualAddress:  0x4000
+    VirtualSize:     112
+    StructuredData:
+      - UInt32: 1       # Version
+      - UInt32: 0x6000  # CodeMap
+      - UInt32: 3       # CodeMapCount
+      - UInt32: 0       # CodeRangesToEntryPoints
+      - UInt32: 0       # RedirectionMetadata
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0       # CodeRangesToEntryPointsCount
+      - UInt32: 0       # RedirectionMetadataCount
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+      - UInt32: 0
+symbols:         []
+...

llvm/tools/llvm-objdump/llvm-objdump.cpp

static char getMappingSymbolKind(ArrayRef<MappingSymbolPair> MappingSymbols,
                                  uint64_t Address) {
   auto It =
       partition_point(MappingSymbols, [Address](const MappingSymbolPair &Val) {
         return Val.first <= Address;
       });
   // Return zero for any address before the first mapping symbol; this means
   // we should use the default disassembly mode, depending on the target.
   if (It == MappingSymbols.begin())
     return '\x00';
   return (It - 1)->second;
 }
 
 static uint64_t dumpARMELFData(uint64_t SectionAddr, uint64_t Index,
                                uint64_t End, const ObjectFile &Obj,
                                ArrayRef<uint8_t> Bytes,
                                ArrayRef<MappingSymbolPair> MappingSymbols,
                                const MCSubtargetInfo &STI, raw_ostream &OS) {

 
 static void
 disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
                   DisassemblerTarget &PrimaryTarget,
                   std::optional<DisassemblerTarget> &SecondaryTarget,
                   SourcePrinter &SP, bool InlineRelocs) {
   DisassemblerTarget *DT = &PrimaryTarget;
   bool PrimaryIsThumb = false;
-  if (isArmElf(Obj))
-    PrimaryIsThumb = PrimaryTarget.SubtargetInfo->checkFeatures("+thumb-mode");
+  SmallVector<std::pair<uint64_t, uint64_t>, 0> CHPECodeMap;
+
+  if (SecondaryTarget) {
+    if (isArmElf(Obj)) {
+      PrimaryIsThumb =
+          PrimaryTarget.SubtargetInfo->checkFeatures("+thumb-mode");
+    } else if (const auto *COFFObj = dyn_cast<COFFObjectFile>(&Obj)) {
+      const chpe_metadata *CHPEMetadata = COFFObj->getCHPEMetadata();
+      if (CHPEMetadata && CHPEMetadata->CodeMapCount) {
+        uintptr_t CodeMapInt;
+        cantFail(COFFObj->getRvaPtr(CHPEMetadata->CodeMap, CodeMapInt));
+        auto CodeMap = reinterpret_cast<const chpe_range_entry *>(CodeMapInt);
+
+        for (uint32_t i = 0; i < CHPEMetadata->CodeMapCount; ++i) {
+          if (CodeMap[i].getType() == chpe_range_type::Amd64 &&
+              CodeMap[i].Length) {
+            // Store x86_64 CHPE code ranges.
+            uint64_t Start = CodeMap[i].getStart() + COFFObj->getImageBase();
+            CHPECodeMap.emplace_back(Start, Start + CodeMap[i].Length);
+          }
+        }
+        llvm::sort(CHPECodeMap);
+      }
+    }
+  }
 
   std::map<SectionRef, std::vector<RelocationRef>> RelocMap;
   if (InlineRelocs)
     RelocMap = getRelocsMap(Obj);
   bool Is64Bits = Obj.getBytesInAddress() > 4;
 
   // Create a mapping from virtual address to symbol name.  This is used to
   // pretty print the symbols while disassembling.
for (const SectionRef &Section : ToolSectionFilter(Obj)) {
     // corresponding to this section, if present.
     if (SymbolizeOperands && Obj.isRelocatableObject())
       ReadBBAddrMap(Section.getIndex());
 
     // Get the list of all the symbols in this section.
     SectionSymbolsTy &Symbols = AllSymbols[Section];
     auto &MappingSymbols = AllMappingSymbols[Section];
     llvm::sort(MappingSymbols);
 
     ArrayRef<uint8_t> Bytes = arrayRefFromStringRef(
         unwrapOrError(Section.getContents(), Obj.getFileName()));
 
     std::vector<std::unique_ptr<std::string>> SynthesizedLabelNames;
     if (Obj.isELF() && Obj.getArch() == Triple::amdgcn) {
       // AMDGPU disassembler uses symbolizer for printing labels
       addSymbolizer(*DT->Context, DT->TheTarget, TripleName, DT->DisAsm.get(),
                     SectionAddr, Bytes, Symbols, SynthesizedLabelNames);
     }
 
     StringRef SegmentName = getSegmentName(MachO, Section);
     StringRef SectionName = unwrapOrError(Section.getName(), Obj.getFileName());
     // If the section has no symbol at the start, just insert a dummy one.
     // Without --show-all-symbols, also insert one if all symbols at the start
     // are mapping symbols.
     bool CreateDummy = Symbols.empty();
     if (!CreateDummy) {
       CreateDummy = true;
       for (auto &Sym : Symbols) {
         if (Sym.Addr != SectionAddr)
for (size_t SI = 0, SE = Symbols.size(); SI != SE;) {
         // same section. We rely on the markers introduced to understand what
         // we need to dump. If the data marker is within a function, it is
         // denoted as a word/short etc.
         if (!MappingSymbols.empty()) {
           char Kind = getMappingSymbolKind(MappingSymbols, Index);
           DumpARMELFData = Kind == 'd';
           if (SecondaryTarget) {
             if (Kind == 'a') {
               DT = PrimaryIsThumb ? &*SecondaryTarget : &PrimaryTarget;
             } else if (Kind == 't') {
               DT = PrimaryIsThumb ? &PrimaryTarget : &*SecondaryTarget;
             }
           }
+        } else if (!CHPECodeMap.empty()) {
+          uint64_t Address = SectionAddr + Index;
+          auto It = partition_point(
+              CHPECodeMap,
+              [Address](const std::pair<uint64_t, uint64_t> &Entry) {
+                return Entry.first <= Address;
+              });
+          if (It != CHPECodeMap.begin() && Address < (It - 1)->second) {
+            DT = &*SecondaryTarget;
+          } else {
+            DT = &PrimaryTarget;
+            // X64 disassembler range may have left Index unaligned, so
+            // make sure that it's aligned when we switch back to ARM64
+            // code.
+            Index = llvm::alignTo(Index, 4);
+            if (Index >= End)
+              break;
+          }
         }
 
         if (DumpARMELFData) {
           Size = dumpARMELFData(SectionAddr, Index, End, Obj, Bytes,
                                 MappingSymbols, *DT->SubtargetInfo, FOS);
         } else {
           // When -z or --disassemble-zeroes are given we always dissasemble
           // them. Otherwise we might want to skip zero bytes we see.
static void disassembleObject(ObjectFile *Obj, bool InlineRelocs) {
   if (isArmElf(*Obj)) {
     if (!PrimaryTarget.SubtargetInfo->checkFeatures("+mclass")) {
       if (PrimaryTarget.SubtargetInfo->checkFeatures("+thumb-mode"))
         Features.AddFeature("-thumb-mode");
       else
         Features.AddFeature("+thumb-mode");
       SecondaryTarget.emplace(PrimaryTarget, Features);
     }
+  } else if (const auto *COFFObj = dyn_cast<COFFObjectFile>(Obj)) {
+    const chpe_metadata *CHPEMetadata = COFFObj->getCHPEMetadata();
+    if (CHPEMetadata && CHPEMetadata->CodeMapCount) {
+      // Set up x86_64 disassembler for ARM64EC binaries.
+      Triple X64Triple(TripleName);
+      X64Triple.setArch(Triple::ArchType::x86_64);
+
+      std::string Error;
+      const Target *X64Target =
+          TargetRegistry::lookupTarget("", X64Triple, Error);
+      if (X64Target) {
+        SubtargetFeatures X64Features;
+        SecondaryTarget.emplace(X64Target, *Obj, X64Triple.getTriple(), "",
+                                X64Features);
+      } else {
+        reportWarning(Error, Obj->getFileName());
+      }
+    }
   }
 
   const ObjectFile *DbgObj = Obj;
   if (!FetchedBinary.getBinary() && !Obj->hasDebugInfo()) {
     if (std::optional<OwningBinary<Binary>> DebugBinaryOpt =
             fetchBinaryByBuildID(*Obj)) {
       if (auto *FetchedObj =
               dyn_cast<const ObjectFile>(DebugBinaryOpt->getBinary())) {