This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump][RISCV] Use new common method to parse ARCH RISCV attribute
ClosedPublic

Authored by eklepilkina on Dec 7 2022, 9:32 AM.

Details

Summary

Added IgnoreUnknown parameter to parseArchString to allow objdump use this method without failing on unknown extensions

Diff Detail

Event Timeline

eklepilkina created this revision.Dec 7 2022, 9:32 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
eklepilkina requested review of this revision.Dec 7 2022, 9:32 AM
eklepilkina removed a reviewer: Hsiang-Kai.
eklepilkina added a subscriber: anton-afanasyev.

I haven't found any reasons written in comments or docs why llvm-objdump couldn't take the attributes from elf file and decode all instructions without extra flags. Could you please have a look is this change can be valid?

arichardson added inline comments.Dec 8 2022, 12:09 AM
llvm/test/tools/llvm-objdump/ELF/RISCV/unknown-arch-attr.test
13

IMO objdump should just ignore features it doesn't know about and still decode whatever it knows about. It could still print a warning though.

I've implement this before ( https://reviews.llvm.org/D125204), but I didn't find enough time to complete the error handle part, apparently this patch did better than mine in this part.

The only functional difference is this patch isn't ignore unsupported extension, feel free to take those part into your patch :)

I don't know how I missed this patch. @kito-cheng thanks you, if you aren't against I'll try merge these 2 patches.

if you aren't against I'll try merge these 2 patches.

I am happy to see that happen, just take anything you need from my patch :)

  • Ignore unknown extensions

Updated patch, thank you @kito-cheng

kito-cheng added inline comments.Dec 8 2022, 8:26 AM
llvm/test/MC/RISCV/attribute-with-insts.s
5–8 ↗(On Diff #481279)

Not sure why this removed? I suppose this should still work as well after this patch?

llvm/test/tools/llvm-objdump/ELF/RISCV/unknown-arch-attr.test
3–4

Those comment can keep now :)

13

And this can drop now.

37–38

Just note why this need change: arch string at least should be canonical order.

jrtc27 added inline comments.Dec 8 2022, 8:41 AM
llvm/lib/Object/ELFObjectFile.cpp
307

s/version/extension/?

310

This means a file that has, say, v0p7, will disassemble as v1p0 rather than being ignored like an extension whose name isn't known?..

llvm/test/tools/llvm-objdump/ELF/RISCV/objdump.s
9 ↗(On Diff #481279)

Fix

llvm/test/tools/llvm-objdump/ELF/RISCV/unknown-arch-attr.test
6

This isn't true any more

kito-cheng added inline comments.Dec 8 2022, 6:54 PM
llvm/test/tools/llvm-objdump/ELF/RISCV/extensions.test
1

Failed to apply this patch locally due to missing this file, are you forgot to add a precommit patch for this?

eklepilkina marked 5 inline comments as done.
  • Review fixes
eklepilkina added inline comments.Dec 8 2022, 9:13 PM
llvm/lib/Object/ELFObjectFile.cpp
307

It doesn't check version of experimental extensions only. The extension name is checked.

310

If it's experimental extension yes. This can be changed to use more strict rules, but not sure that this is needed for experimental extensions.

llvm/test/tools/llvm-objdump/ELF/RISCV/extensions.test
1

I wasn't told not to load precommit patch at the same review. So yes, it exsits now only on my branch. I can't commit it in upstream, so I have asked a collegue to do this. Precommit patch wil be on master soon.

eklepilkina added inline comments.Dec 9 2022, 1:24 AM
llvm/test/tools/llvm-objdump/ELF/RISCV/extensions.test
1

LGTM for RISC-V part, but I would like let @MaskRay to give final approve since this patch also touch something other than RISC-V port.

ping, @MaskRay could you take a look for this, RISC-V part is LGTM, but this patch has modify something more than RISC-V, so I would like to make sure you are happy with those change too :)

MaskRay added a comment.EditedJan 10 2023, 8:19 PM

ping, @MaskRay could you take a look for this, RISC-V part is LGTM, but this patch has modify something more than RISC-V, so I would like to make sure you are happy with those change too :)

Thanks! The intention LGTM to me as well, but the code style and tests need more work.

llvm/include/llvm/MC/SubtargetFeature.h
192

addFeaturesVector

Use ArrayRef if no need to be a vector

llvm/lib/Support/RISCVISAInfo.cpp
580

+= 1 + ConsumeLength

llvm/test/tools/llvm-objdump/ELF/RISCV/objdump.s
1 ↗(On Diff #481512)

drop < for llvm-mc

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

drop braces

llvm/tools/llvm-profgen/ProfiledBinary.cpp
571

(*...).xxx => ->

The addition of IgnoreUnknown should be mentioned in the commit message / Differential summary. And mention who will be the users.

MaskRay added inline comments.Jan 10 2023, 8:23 PM
llvm/test/tools/llvm-objdump/ELF/RISCV/objdump.s
1 ↗(On Diff #481512)

objdump.s is not a good name. Add a file-level comment what this test is about.

If you want to test that llvm-objdump by default decodes most instructions, add a few more instructions beside vsetvli (one instruction doesn't tell much).

  • Review fixes
  • Review fixes
eklepilkina edited the summary of this revision. (Show Details)Jan 12 2023, 2:56 AM
MaskRay accepted this revision.Jan 12 2023, 4:07 PM

Looks great with some nits.

llvm/lib/Object/ELFObjectFile.cpp
305

The common style doesn't add a blank line after a variable declaration.

llvm/test/tools/llvm-objdump/ELF/RISCV/objdump-instr-from-extensions.s
1 ↗(On Diff #488568)

For binary utilities, new test/tools/llvm-* tests are supposed to use ## for non-RUN-non-CHECK lines.

Nitpicking: objdump- in objdump-instr-from-extensions.s doesn't convey additional value as the test directory has llvm-objdump in the component.
I don't have a particular good name, and hope others can make a suggestion. Perhaps Tag_RISCV_arch.s is sufficiently clear.

llvm-objdump isn't objdump (which usually refers to GNU objdump).

Use the canonical name Tag_RISCV_arch.

24 ↗(On Diff #488568)

IIUC vfwsub/vfrsub are in the same family and it's too useful to have more than one instructions in one family for the purpose of this test. Perhaps just keep vfsub.vv and delete all lines below (and the blank line above).

This revision is now accepted and ready to land.Jan 12 2023, 4:07 PM
  • Review fixes
This revision was landed with ongoing or failed builds.Jan 16 2023, 5:58 AM
This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.Jan 18 2023, 1:57 AM
foad added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
2007

How does this work? Don't we need to somehow exit after reporting the error? Is there a test for this path?

eklepilkina added inline comments.Jan 18 2023, 2:34 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
2007

Yep, true, missed this, thank you. I'll make a new review to fix this today. This is already merged.

foad added inline comments.Jan 18 2023, 2:40 AM
llvm/lib/Support/RISCVISAInfo.cpp
692

This is dead code.

712

This is dead code.