Page MenuHomePhabricator

[llvm-objdump] Print comments for the disassembled code
ClosedPublic

Authored by ikudrin on Jun 22 2021, 4:11 AM.

Details

Summary

LLVM disassembler can generate comments for disassembled instructions. The patch enables printing these comments for llvm-objdump -d.

Diff Detail

Event Timeline

ikudrin created this revision.Jun 22 2021, 4:11 AM
ikudrin requested review of this revision.Jun 22 2021, 4:11 AM

Perfectly happy to have these comments, but should it be behind an option? No strong opinion either way.

Perfectly happy to have these comments, but should it be behind an option? No strong opinion either way.

I've done that mostly as a preparation for D104701. I hope that we gradually can provide more useful information in the comments. For example, I plan to annotate some PC-relative constants, similar to what GNU's objdump does; note that they print such comments without additional options. If someone finds the comments unhelpful, we can add an option to disable them, but I believe it's an unlikely case.

MaskRay added inline comments.Jun 22 2021, 9:46 AM
llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-print-comments.s
8

For lsl #12 it is useful to display 8192.

But for add z31.d, z31.d, #65280, 0xff00 may be less useful. GNU objdump displays many such immediates in hexadecimal, if I switch it to hexadecimal, then it should be clear the comment will not be needed.

jhenderson accepted this revision.Jun 23 2021, 12:36 AM

LGTM, with one suggestion and one question. It looks to me like @MaskRay's comment is somewhat unrelated to this patch?

llvm/tools/llvm-objdump/llvm-objdump.cpp
1042

"Stuff" sounds a little unprofessional, although I am struggling to come up with a good alternative!

Maybe emitPostInstructionInfo?

1429

I'm not really up-to-speed on a lot of the MC stuff, but is there a need to do this?

This revision is now accepted and ready to land.Jun 23 2021, 12:36 AM
ikudrin updated this revision to Diff 353917.Jun 23 2021, 3:43 AM
ikudrin marked an inline comment as done.
  • emitEOLStuff() -> emitPostInstructionInfo()
ikudrin added inline comments.
llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-print-comments.s
8

I guess that a person who added generating the comments, @sdesmalen, found them useful for whatever reason. For my personal taste, it is better to have more comments than fewer. They can always be easily ignored if you do not need them.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1042

Thanks for the suggestion!

1429

CommentStream is local to the function while IP is external, thus the provided reference should be removed. We could set the comment stream somewhere at the top of the function, where it is created, and remove it at the end, but the function is very long and these phases would be too far away from each other. Such code would be a bit fragile to my taste and could be broken unintentionally during development.

MaskRay accepted this revision.Jun 23 2021, 12:00 PM
MaskRay added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
1429

Does this mean that adding a no-comment instruction to the test file can increase coverage?

ikudrin added inline comments.Jun 24 2021, 3:06 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1429

There is a nop instruction in X86/disassemble-print-comments.s. There are also lots of existing tests with no-comment instructions. If that is not sufficient, what do you suggest exactly?

MaskRay added inline comments.Jun 24 2021, 12:55 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1429

I am thinking of

insn_need_comment
insn_no_comment

Make sure insn_no_comment does not have extra comments or get weird formatting after insn_need_comment

ikudrin added inline comments.Jun 24 2021, 6:36 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1429

I'll append a nop to ELF/AArch64/disassemble-align.s and add a --match-full-lines switch to FileCheck in X86/disassemble-align.s. Do you think that is sufficient?

ikudrin updated this revision to Diff 354413.Jun 24 2021, 6:37 PM
  • Updated tests
MaskRay accepted this revision.Jun 24 2021, 6:51 PM

Looks great!

llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-align.s
7

Append {{$}} after nop?

ikudrin added inline comments.Jun 24 2021, 7:00 PM
llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-align.s
7

As both --match-full-lines and --strict-whitespace are used, that is not necessary.

This revision was automatically updated to reflect the committed changes.