[llvm-objdump] Print comments for the disassembled code

[ikudrin]

Summary

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

[jhenderson]

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

[ikudrin]

In D104699#2832845, @jhenderson wrote:

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]

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]

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?

[ikudrin]

• emitEOLStuff() -> emitPostInstructionInfo()

[ikudrin]

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]

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]

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]

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]

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 tests

[MaskRay]

Looks great!

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

7

Append {{$}} after nop?

[ikudrin]

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.

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

 # RUN: llvm-mc -filetype=obj -triple=aarch64 %s -o %t
 # RUN: llvm-objdump -d %t | tr '\t' '|' | FileCheck --match-full-lines --strict-whitespace %s
 
 ## Use '|' to show where the tabs line up.
 #       CHECK:0000000000000000 <$x.0>:
-#  CHECK-NEXT:       0: 62 10 00 91  |add|x2, x3, #4
+#  CHECK-NEXT:       0: 62 10 00 91  |add|x2, x3, #4              // =4
+#  CHECK-NEXT:       4: 1f 20 03 d5  |nop
 # CHECK-EMPTY:
-#  CHECK-NEXT:0000000000000004 <$d.1>:
-#  CHECK-NEXT:       4:|ff ff 00 00|.word|0x0000ffff
+#  CHECK-NEXT:0000000000000008 <$d.1>:
+#  CHECK-NEXT:       8:|ff ff 00 00|.word|0x0000ffff
 
   add x2, x3, #4
+  nop
   .word 0xffff

llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-print-comments.s

+## Check that 'llvm-objdump -d' prints comments generated by the disassembler.
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64 -mattr=+sve %s -o %t
+# RUN: llvm-objdump -d --mattr=+sve --no-show-raw-insn %t | FileCheck %s
+
+# CHECK:      0000000000000000 <foo>:
+# CHECK-NEXT:        0:   add x0, x2, #2, lsl #12    // =8192
+# CHECK-NEXT:        4:   add z31.d, z31.d, #65280   // =0xff00
+
+    .text
+foo:
+    add x0, x2, 8192
+    add z31.d, z31.d, #65280

llvm/test/tools/llvm-objdump/X86/disassemble-align.s

 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
 
 # Use '|' to show where the tabs line up.
-# RUN: llvm-objdump -d --print-imm-hex %t | tr '\t' '|' | FileCheck --strict-whitespace %s
+# RUN: llvm-objdump -d --print-imm-hex %t | tr '\t' '|' | \
+# RUN:   FileCheck --match-full-lines --strict-whitespace %s
 
 # RUN: llvm-objdump -d --print-imm-hex --no-show-raw-insn %t | tr '\t' '|' | \
-# RUN:   FileCheck -check-prefix=NORAW -strict-whitespace %s
+# RUN:   FileCheck -check-prefix=NORAW --match-full-lines -strict-whitespace %s
 
 # Instructions are expected to be aligned if the instruction in hex is not too long.
 
-# CHECK:       0: c3                           |retq
-# CHECK-NEXT:  1: 48 8b 05 56 34 12 00         |movq|0x123456(%rip), %rax
-# CHECK-NEXT:  8: 48 b8 54 55 55 55 55 55 55 55|movabsq|$0x5555555555555554, %rax
-# CHECK-NEXT: 12: 8f ea 00 12 4c 02 40 00 00 00 00     |lwpval|$0x0, 0x40(%rdx,%rax), %r15d
-# CHECK-NEXT: 1d: 8f ea 00 12 04 25 f0 1c f0 1c 00 00 00 00    |lwpins|$0x0, 0x1cf01cf0, %r15d
-# CHECK-NEXT: 2b: ff ff                        |<unknown>
+#      CHECK:       0: c3                           |retq
+# CHECK-NEXT:       1: 48 8b 05 56 34 12 00         |movq|0x123456(%rip), %rax  # 12345e <.text+0x12345e>
+# CHECK-NEXT:       8: 48 b8 54 55 55 55 55 55 55 55|movabsq|$0x5555555555555554, %rax # imm = 0x5555555555555554
+# CHECK-NEXT:      12: 8f ea 00 12 4c 02 40 00 00 00 00     |lwpval|$0x0, 0x40(%rdx,%rax), %r15d
+# CHECK-NEXT:      1d: 8f ea 00 12 04 25 f0 1c f0 1c 00 00 00 00    |lwpins|$0x0, 0x1cf01cf0, %r15d
+# CHECK-NEXT:      2b: ff ff                        |<unknown>
 
-# NORAW:       0:      |retq
-# NORAW-NEXT:  1:      |movq|0x123456(%rip), %rax
-# NORAW-NEXT:  8:      |movabsq|$0x5555555555555554, %rax
-# NORAW-NEXT: 12:      |lwpval|$0x0, 0x40(%rdx,%rax), %r15d
-# NORAW-NEXT: 1d:      |lwpins|$0x0, 0x1cf01cf0, %r15d
-# NORAW-NEXT: 2b:      |<unknown>
+#      NORAW:       0:      |retq
+# NORAW-NEXT:       1:      |movq|0x123456(%rip), %rax  # 12345e <.text+0x12345e>
+# NORAW-NEXT:       8:      |movabsq|$0x5555555555555554, %rax # imm = 0x5555555555555554
+# NORAW-NEXT:      12:      |lwpval|$0x0, 0x40(%rdx,%rax), %r15d
+# NORAW-NEXT:      1d:      |lwpins|$0x0, 0x1cf01cf0, %r15d
+# NORAW-NEXT:      2b:      |<unknown>
 
 .text
   retq
   movq 0x123456(%rip),%rax
   movabs $0x5555555555555554,%rax
   lwpval $0x0, 0x40(%rdx,%rax), %r15d
   lwpins $0x0, 0x1cf01cf0, %r15d
   .word 0xffff

llvm/test/tools/llvm-objdump/X86/disassemble-print-comments.s

+## Check that 'llvm-objdump -d' prints comments generated by the disassembler.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck %s
+
+# CHECK:      0000000000000000 <foo>:
+# CHECK-NEXT:        0:   nop
+# CHECK-NEXT:        1:   cmpl $305419896, %eax   # imm = 0x12345678
+
+    .text
+foo:
+    nop
+    cmpl $0x12345678, %eax

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

static StringRef getSegmentName(const MachOObjectFile *MachO,
   if (MachO) {
     DataRefImpl DR = Section.getRawDataRefImpl();
     StringRef SegmentName = MachO->getSectionFinalSegmentName(DR);
     return SegmentName;
   }
   return "";
 }
 
+static void emitPostInstructionInfo(formatted_raw_ostream &FOS,
+                                    const MCAsmInfo &MAI,
+                                    const MCSubtargetInfo &STI,
+                                    StringRef Comments,
+                                    LiveVariablePrinter &LVP) {
+  do {
+    if (!Comments.empty()) {
+      // Emit a line of comments.
+      StringRef Comment;
+      std::tie(Comment, Comments) = Comments.split('\n');
+      // MAI.getCommentColumn() assumes that instructions are printed at the
+      // position of 8, while getInstStartColumn() returns the actual position.
+      unsigned CommentColumn =
+          MAI.getCommentColumn() - 8 + getInstStartColumn(STI);
+      FOS.PadToColumn(CommentColumn);
+      FOS << MAI.getCommentString() << ' ' << Comment;
+    }
+    LVP.printAfterInst(FOS);
+    FOS << '\n';
+  } while (!Comments.empty());
+  FOS.flush();
+}
+
 static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
                               MCContext &Ctx, MCDisassembler *PrimaryDisAsm,
                               MCDisassembler *SecondaryDisAsm,
                               const MCInstrAnalysis *MIA, MCInstPrinter *IP,
                               const MCSubtargetInfo *PrimarySTI,
                               const MCSubtargetInfo *SecondarySTI,
                               PrettyPrinter &PIP,
                               SourcePrinter &SP, bool InlineRelocs) {
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
               DisAsm->getInstruction(Inst, Size, Bytes.slice(Index),
                                      SectionAddr + Index, CommentStream);
           if (Size == 0)
             Size = 1;
 
           LVP.update({Index, Section.getIndex()},
                      {Index + Size, Section.getIndex()}, Index + Size != End);
 
+          IP->setCommentStream(CommentStream);
+
           PIP.printInst(
               *IP, Disassembled ? &Inst : nullptr, Bytes.slice(Index, Size),
               {SectionAddr + Index + VMAAdjustment, Section.getIndex()}, FOS,
               "", *STI, &SP, Obj->getFileName(), &Rels, LVP);
-          FOS << CommentStream.str();
-          Comments.clear();
+
+          IP->setCommentStream(llvm::nulls());
 
           // If disassembly has failed, avoid analysing invalid/incomplete
           // instruction information. Otherwise, try to resolve the target
           // address (jump target or memory operand address) and print it on the
           // right of the instruction.
           if (Disassembled && MIA) {
             uint64_t Target;
             bool PrintTarget =
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
                 FOS << ">";
               } else if (LabelAvailable) {
                 FOS << " <" << AllLabels[Target] << ">";
               }
             }
           }
         }
 
-        LVP.printAfterInst(FOS);
-        FOS << "\n";
+        assert(Ctx.getAsmInfo());
+        emitPostInstructionInfo(FOS, *Ctx.getAsmInfo(), *STI,
+                                CommentStream.str(), LVP);
+        Comments.clear();
 
         // Hexagon does this in pretty printer
         if (Obj->getArch() != Triple::hexagon) {
           // Print relocation for instruction and data.
           while (RelCur != RelEnd) {
             uint64_t Offset = RelCur->getOffset();
             // If this relocation is hidden, skip it.
             if (getHidden(*RelCur) || SectionAddr + Offset < StartAddress) {