This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jacek on Apr 24 2023, 1:09 PM.

Diff Detail

Unit TestsFailed

Event Timeline

jacek created this revision.Apr 24 2023, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2023, 1:09 PM
Herald added a subscriber: zzheng. · View Herald Transcript
jacek requested review of this revision.Apr 24 2023, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2023, 1:09 PM
jacek updated this revision to Diff 516789.Apr 25 2023, 6:45 AM

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 updated this revision to Diff 544124.Jul 25 2023, 3:26 PM

Rebased.

(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
1429
1434

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.

1625

FWIW, I am going to change MappingSymbols in D156190

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
1613

Nit: the coding standards say to prefer preincrement.

1911
2233

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

2236

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

2237

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?

2242

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 marked 8 inline comments as done.Jul 27 2023, 7:35 AM

Thanks for reviews!

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

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

2237

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.

2242

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

jacek updated this revision to Diff 544770.Jul 27 2023, 7:36 AM
jacek marked 3 inline comments as done.

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

llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml
105
llvm/tools/llvm-objdump/llvm-objdump.cpp
1430

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.

2234

No auto here too

2241

Ditto.

jacek updated this revision to Diff 545452.Jul 30 2023, 10:45 AM
jacek marked 5 inline comments as done.

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 accepted this revision.Jul 31 2023, 12:03 AM

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
1436

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.

This revision is now accepted and ready to land.Jul 31 2023, 12:03 AM

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 added inline comments.Jul 31 2023, 1:22 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1132

Why is this islower change?

jacek added inline comments.Jul 31 2023, 3:44 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1132

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

1436

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.

jacek updated this revision to Diff 545841.Jul 31 2023, 3:55 PM
efriedma added inline comments.Jul 31 2023, 4:07 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1132

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

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)

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

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

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 marked 5 inline comments as done.Aug 1 2023, 7:26 AM

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
1132

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

jacek updated this revision to Diff 546052.Aug 1 2023, 7:26 AM
jacek marked an inline comment as done.
jacek updated this revision to Diff 546263.Aug 1 2023, 4:28 PM

Also changed temp file names in the second test file.

MaskRay accepted this revision.Aug 2 2023, 12:55 PM

Nit about a test

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

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 added a comment.Aug 2 2023, 4:53 PM

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 updated this revision to Diff 546643.Aug 2 2023, 4:59 PM

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 requested changes to this revision.Aug 10 2023, 12:03 AM

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

This revision now requires changes to proceed.Aug 10 2023, 12:03 AM
jacek updated this revision to Diff 549753.Aug 13 2023, 1:42 PM

Fixed test, sorry for missing that earlier.

jhenderson accepted this revision.Aug 13 2023, 11:46 PM

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.

This revision is now accepted and ready to land.Aug 13 2023, 11:46 PM
MaskRay accepted this revision.Aug 14 2023, 11:42 AM
MaskRay added inline comments.
llvm/test/tools/llvm-objdump/COFF/arm64ec.yaml
108

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

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

This revision was automatically updated to reflect the committed changes.
jacek marked an inline comment as done.Aug 14 2023, 3:08 PM

I changed the test and pushed. Thanks for reviews!

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

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 added a comment.Aug 15 2023, 1:59 PM

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.