ARM64EC/ARM64X binaries use ARM64 or AMD64 machine types, but provide additional CHPE metadata that may be used to distinguish them from pure ARM64/AMD64 binaries.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
+1 to this if at all possible: the code under test isn't LLD specific (I assume, since it's in the LLVM header area), so sticking the testing in lld seems wrong. It might require some extra work in yaml2obj to get it to support new things, but I don't think that's a bad thing.
| lld/test/COFF/arm64ec.test | ||
|---|---|---|
| 43 ↗ | (On Diff #516509) | Nit: delete extra blank line. |
Comment Actions
Thanks for reviews. The new version uses yaml2obj instead of lld-link. Previous tests will be useful for testing lld-link itself, but I will send it later, probably together with code range generation support.
yaml-based test is a bit less similar to real-world binaries. Linker should align code boundaries to page size, but doing that with yaml SectionData makes it hard to follow. I just used much smaller alignment, it shouldn't matter for disassembler anyway.
CHPE and load config are quite hard to read. I guess we could have some helper syntax for that in yaml2obj, but it's not clear to me how it should look like. Unlike other things like PE headers, load config and CHPE metadata are part of .data/.rdata sections (so it would need to alter SectionData content) and we'd need some way of referencing it (unless we'd still hard-code RVAs). I will investigate if further if you prefer.
Comment Actions
I guess we could have some helper syntax for that in yaml2obj, but it's not clear to me how it should look like.
Someone can always use llvm-readobj or dumpbin to help understand the fine details if necessary, so it's not a big deal if it's only a few tests (particularly if they have comments explaining the tricky bits).
We could invent a new kind of "SectionData" that allows structured data, instead of just a string, I guess. We don't want to get too fancy here; even just representing the string as an array of 32-bit integers would be sufficient to annotate various bits using comments.
Adding a mechanism for relocations to represent RVAs seems overkill. yaml2obj is intentionally kept simple.
Comment Actions
The new yaml2obj-based test certainly looks reasonable to me. However, it looks like there are three code paths now, and the test only tests one of them. The case where CHPEMetadata is false/nullptr/whatever is possibly covered by other tests(?), but I think you also need to test the other part of the switch you've added? (I don't know anything really about the code area to give any particular guidance about it though).
| llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml | ||
|---|---|---|
| 2 ↗ | (On Diff #536902) | Nits:
|
| 28 ↗ | (On Diff #536902) | Rather than having two identical(?)-barring-one-field input YAMLs between the two tests, can you use the yaml2obj -D feature to enable sharing of the two? I don't know if that feature is specific to the ELF side or not, but if it is usable, you could have both tests in the same file, and just create two input files from the same input block, which differ only by the Machine type. This would also allow you to minimise the duplication in the FileCheck CHECK patterns. A good example of what I mean can be seen in this test: https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/file-header-machine-types.test. |
Comment Actions
Rebased, using a single file and DISASM and double-dash for disassembler output checks.
Comment Actions
LGTM, with nits.
| llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml | ||
|---|---|---|
| 1 ↗ | (On Diff #537188) | Nit: newer tools tests use ## for comments, to distinguish them from lit and FileCheck directives. |
| 5 ↗ | (On Diff #537188) | I would consider reordering your CHECK/RUN lines for a bit of improved readability/comparability, since you're partly sharing them (I also added some extra whitespace to make things nicely line up): # 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 # 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 # ARM64EC: file format coff-arm64ec # ARM64X: file format coff-arm64x # DISASM: ... |
Comment Actions
NB: It wouldn't hurt to get an approval from someone more familiar with COFF/ARM64EC etc.
Comment Actions
Hi, I think this broke lldb-aarch64-windows : https://lab.llvm.org/buildbot/#/builders/219/builds/4366
Could you please take a look at it?
Comment Actions
I think I can see what's going on, sorry about that. Now that COFFObjectFile recognizes ARM64X, it reports that for system DLLs and lldb doesn't know what to do about them. I think that something like the attached patch should help, but I will need a bit to verify.