This is an archive of the discontinued LLVM Phabricator instance.

[1/4] [llvm-objdump][test] Add RISC-V objdump test case
ClosedPublic

Authored by asb on Jan 5 2022, 10:32 AM.

Details

Summary

This test case captures the current state of support for printing branch targets.

Diff Detail

Unit TestsFailed

Event Timeline

asb created this revision.Jan 5 2022, 10:32 AM
asb requested review of this revision.Jan 5 2022, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2022, 10:32 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
MaskRay added inline comments.Jan 5 2022, 10:41 AM
llvm/test/tools/llvm-objdump/ELF/RISCV/branches.s
3

Remove --match-full-lines

In most test/tools/, the prevailing continuation-line format is to place | at the line end.

# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c %s |
# RUN:     llvm-objdump -d -M no-aliases --no-show-raw-insn - |
5

The address formatting is checked by other tests so this test doesn't need to check it.

By removing addresses, when the number of instruction changes, the update (ongoing maintenance) will touch fewer lines.

7

For similar reasons, remove leading addresses.

8

I have a slight preference that instructions are indented by two: to be clearer that they belong to the function (nearest label).

77
79

60: c.nop should probably keep its address to be clear that its a jump target.

[NFC]

I prefer [test] to be clear that no code is touched.

asb added a comment.Jan 5 2022, 10:48 AM

Thank for such a rapid review @MaskRay! I've left a couple of queries in response, but otherwise I'll refresh the test case once I've had feedback on other patches in the series (to minimise repeated rebase effort).

llvm/test/tools/llvm-objdump/ELF/RISCV/branches.s
3

The reasoning behind --match-full-lines was that it will catch changes to printing of branch targets (as in D116678), ensuring the test case is updated.

8

Do you still feel it's worth doing, when taking into account the changes to this test in D116678?

asb retitled this revision from [1/4] [llvm-objdump][NFC] Add RISC-V objdump test case to [1/4] [llvm-objdump][test] Add RISC-V objdump test case.Jan 5 2022, 10:49 AM
MaskRay added inline comments.Jan 5 2022, 11:29 AM
llvm/test/tools/llvm-objdump/ELF/RISCV/branches.s
3

An alternative is to add {{$}} in a few places where it is significant.

asb updated this revision to Diff 398427.Jan 9 2022, 5:19 AM
asb marked 5 inline comments as done and an inline comment as not done.

Address review comments.

MaskRay accepted this revision.Jan 12 2022, 4:57 PM
This revision is now accepted and ready to land.Jan 12 2022, 4:57 PM
This revision was automatically updated to reflect the committed changes.