[PATCH] D104699: [llvm-objdump] Print comments for the disassembled code

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1427
+
+          IP->setCommentStream(llvm::nulls());
 
----------------
MaskRay wrote:
> ikudrin wrote:
> > MaskRay wrote:
> > > ikudrin wrote:
> > > > jhenderson wrote:
> > > > > I'm not really up-to-speed on a lot of the MC stuff, but is there a need to do this?
> > > > `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.
> > > Does this mean that adding a no-comment instruction to the test file can increase coverage?
> > 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?
> 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`
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104699/new/

https://reviews.llvm.org/D104699