Details
Diff Detail
Unit Tests
Time | Test | |
---|---|---|
60,040 ms | x64 debian > MLIR.Examples/standalone::test.toy |
Event Timeline
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.
(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 | This is an instance that auto is obvious https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | |
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)? |
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. |
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. |
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. |
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.
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
1132 | Why is this islower change? |
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. |
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. |
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. |
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!
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.
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.
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 |
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.
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.
This is uncommon and will make many build configurations not able to detect this issue.
Keep just # REQUIRES: aarch64-registered-target and use
so that most RUN lines in this test are still testable on a default configuration that builds all non-experimental targets.