[llvm-objdump] Align instructions to a tab stop in disassembly output

[MaskRay]

Summary

In GNU objdump, -w/--wide aligns instructions in the disassembly output.
This patch does the same to llvm-objdump. However, we always use the
wide format (-w/--wide is ignored), because the narrow format is
probably not very useful.

In llvm-readobj, we made a similar decision: always use the wide format,
accept but ignore -W/--wide.

[MaskRay]

use sed y

not sure if tr is usable (not used by any test yet and not listed on https://llvm.org/docs/GettingStarted.html) so I use sed here.

[MaskRay]

Add <unknown> to the test

[rupprecht]

I'm fine with the -w flag, but I haven't had time to look at tab alignment code -- it seems very hard to follow. Perhaps it should be a separate patch? This seems like two unrelated changes.

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

3

You can include literal tabs in tests
In some editors it is ctrl+v, then tab

[MaskRay]

Use formatted_raw_ostream to track current column

[MaskRay]

Use LWP instructions

[MaskRay]

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

3

I use a space to demonstrate that the column of the instruction name is not affected by the tabsize setting in the editor.

e.g. in (neo)vim, set ts=1 set ts=2 set ts=4 set ts=8 align the instruction name in the same column.

Committed -w/--wide in a separate change.

[jhenderson]

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

5

Since you did -w/--wide in a different change, you should probably remove the references to it from here.

tools/llvm-objdump/llvm-objdump.cpp

618

Why do you need to scope this block?

[MaskRay]

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

5

This is to demonstrate that our output format is similar to that of objdump -w. I want to add a test for --wide -w and I think here is probably the best place. I'd be happy to move it to a difference place if you have a better suggestion.

tools/llvm-objdump/llvm-objdump.cpp

618

Let ~formatted_raw_ostream call flush() before we call IP.printInst(MI, OS, "", STI);

This is required if outs() is buffered (e.g. when redirecting to a file)

[jhenderson]

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

5

This is to demonstrate that our output format is similar to that of objdump -w

I don't think this is something we should be testing in this sense. This test looks to be to do with alignment of columns.

What might make sense is to have a separate test that shows that the default, with --wide, and with -w all produce identical output (since --wide is a silent no-op). This should be a separate change, and should have been part of the change that added --wide.

tools/llvm-objdump/llvm-objdump.cpp

618

Okay. This probably deserves a comment then.

Alternatively, is it possible to just pass FOS into the printInst call?

[MaskRay]

Remove objdump --wide/-w tests.
Add a comment about the dtor.

[MaskRay]

tools/llvm-objdump/llvm-objdump.cpp

618

Added a comment. OS is slightly more efficient if we don't need the column information. This curly brace also shows we don't need FOS after FOS.indent().

[jhenderson]

tools/llvm-objdump/llvm-objdump.cpp

631

I can never remember the precedence of %, so would you mind adding some brackets to the LHS.

7 - Column seems a bit weird given that Column is always going to be bigger than that. Could you rewrite this bit so that it doesn't rely on underflowing?

[MaskRay]

tools/llvm-objdump/llvm-objdump.cpp

631

Multiplication, division and remainder have the same precedence. Just as 1 - 2 / 3 doesn't need parentheses, the remainder expression doesn't need parentheses.

7 - Column % 8 computes to the range 0~7, there is no underflow.

[jhenderson]

tools/llvm-objdump/llvm-objdump.cpp

631

Oh, right, that's me misremembering things. Even more of a reason to add brackets around Column % 8 then. They aren't needed for correctness, but are for readability.

[MaskRay]

tools/llvm-objdump/llvm-objdump.cpp

631

I don't agree. For me, the redundant pair of parentheses harms readability.

[jhenderson]

tools/llvm-objdump/llvm-objdump.cpp

631

Hmm... okay. I know others I work with have the same issue as I do in interpreting it quickly when reading.

[MaskRay]

Update description

[MaskRay]

Further update description

[phosek]

This broke tests on Darwin:

-- Testing: 30534 tests, 24 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
FAIL: LLVM :: tools/llvm-objdump/X86/disassemble-align.s (29994 of 30534)
******************** TEST 'LLVM :: tools/llvm-objdump/X86/disassemble-align.s' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/bin/llvm-mc -filetype=obj -triple=x86_64 /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-objdump/X86/disassemble-align.s -o /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/test/tools/llvm-objdump/X86/Output/disassemble-align.s.tmp
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/bin/llvm-objdump -d -print-imm-hex /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/test/tools/llvm-objdump/X86/Output/disassemble-align.s.tmp | sed 'y/\t/ /' | /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/bin/FileCheck -strict-whitespace /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-objdump/X86/disassemble-align.s
: 'RUN: at line 4';   /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/bin/llvm-objdump -d -print-imm-hex -no-show-raw-insn /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/test/tools/llvm-objdump/X86/Output/disassemble-align.s.tmp | sed 's/\t/ /g' |    /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/bin/FileCheck -check-prefix=NORAW -strict-whitespace /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-objdump/X86/disassemble-align.s
--
Exit Code: 2

Command Output (stderr):
--
sed: 1: "y/\t/ /": transform strings are not the same length
FileCheck error: '-' is empty.
FileCheck command line:  /b/s/w/ir/k/recipe_cleanup/clangSNTUbE/llvm_build_dir/bin/FileCheck -strict-whitespace /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-objdump/X86/disassemble-align.s

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 106.86s
********************
Failing Tests (1):
    LLVM :: tools/llvm-objdump/X86/disassemble-align.s

  Expected Passes    : 22599
  Expected Failures  : 66
  Unsupported Tests  : 7868
  Unexpected Failures: 1

Can we either fix or revert this?

[arphaman]

Reverted in r358459

[MaskRay]

In D60376#1467572, @arphaman wrote:

Reverted in r358459

@phosek @arphaman

sed: 1: "y/\t/ /": transform strings are not the same length

This error message was probably due to # RUN: llvm-objdump -d -print-imm-hex %t | sed 'y/\t/ /' | FileCheck -strict-whitespace %s. I don't have a Mac OS X, do you know if I can use the tr utility? I chose sed because tr is not listed at https://llvm.org/docs/GettingStarted.html#software

[MaskRay]

@rupprecht Some people's editors are configured to replace tabs with spaces. And here, I want to test the tab takes exactly one column. I relanded this revision and it seems 'tr' works.

[rupprecht]

In D60376#1468514, @MaskRay wrote:

@rupprecht Some people's editors are configured to replace tabs with spaces. And here, I want to test the tab takes exactly one column. I relanded this revision and it seems 'tr' works.

Even in editors that do this, there is usually a way to enter it literally, and some tests have literal tabs in them for this cases like this. And editor hooks that might do this usually don't do this for any file -- e.g. a clang-format hook would run on a .cpp file and remove tabs there, but since tabs are significant in Makefile, no hooks would strip tabs there.

It's not clear to me what the test is testing. If the concern is that tabs aren't visible, maybe it would be better to use tr to convert tabs to a visible character like x or t, so you can be clearer about where you expect tabs and where you expect spaces?

[MaskRay]

In D60376#1468542, @rupprecht wrote:

In D60376#1468514, @MaskRay wrote:

@rupprecht Some people's editors are configured to replace tabs with spaces. And here, I want to test the tab takes exactly one column. I relanded this revision and it seems 'tr' works.

Even in editors that do this, there is usually a way to enter it literally, and some tests have literal tabs in them for this cases like this. And editor hooks that might do this usually don't do this for any file -- e.g. a clang-format hook would run on a .cpp file and remove tabs there, but since tabs are significant in Makefile, no hooks would strip tabs there.

It's not clear to me what the test is testing. If the concern is that tabs aren't visible, maybe it would be better to use tr to convert tabs to a visible character like x or t, so you can be clearer about where you expect tabs and where you expect spaces?

tr '\t' '!' (% & or whatever) looks good to me.

[MaskRay]

Changed to | in D60777

[sidneym]

tools/llvm-objdump/llvm-objdump.cpp

631

I find parentheses very useful for instant understanding of expressions.

[MaskRay]

tools/llvm-objdump/llvm-objdump.cpp

631

I find parentheses very useful for instant understanding of expressions.

I also know a group of people who can't stand superfluous parentheses.

This passes -Wparentheses. Note, some rules in -Wparentheses itself is debatable.

This is not the only instance in the code base.

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

+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s | \
+# RUN:   llvm-objdump -d -print-imm-hex - | sed 'y/\t/ /' | FileCheck -strict-whitespace %s
+
+# -w/--wide is ignored. llvm-objdump always produces wide output.
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s | \
+# RUN:   llvm-objdump -wd -print-imm-hex - | sed 'y/\t/ /' | FileCheck -strict-whitespace %s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s | \
+# RUN:   llvm-objdump --wide -d -print-imm-hex - | sed 's/\t/ /g' | FileCheck -strict-whitespace %s
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s | \
+# RUN:   llvm-objdump -d -print-imm-hex -no-show-raw-insn - | sed 's/\t/ /g' | \
+# RUN:   FileCheck -check-prefix=NORAW -strict-whitespace %s
+
+# Instructions are expected to be aligned if the instruction in hex is not too long.
+
+# CHECK:       0: 55                      pushq %rbp
+# 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: c3                      retq
+
+# NORAW:       0:       pushq %rbp
+# NORAW-NEXT:  1:       movq 0x123456(%rip), %rax
+# NORAW-NEXT:  8:       movabsq $0x5555555555555554, %rax
+# NORAW-NEXT: 12:       retq
+
+.text
+  pushq %rbp
+  movq 0x123456(%rip),%rax
+  movabs $0x5555555555555554,%rax
+  ret

tools/llvm-objdump/llvm-objdump.cpp

DisassemblerOptions("disassembler-options",
                         cl::desc("Pass target specific disassembler options"),
                         cl::value_desc("options"), cl::CommaSeparated);
 static cl::alias
     DisassemblerOptionsShort("M", cl::desc("Alias for --disassembler-options"),
                              cl::NotHidden, cl::Grouping, cl::Prefix,
                              cl::CommaSeparated,
                              cl::aliasopt(DisassemblerOptions));
 
+static cl::opt<bool>
+    Wide("wide", cl::desc("Ignored for compatibility with GNU objdump"));
+static cl::alias WideShort("w", cl::Grouping, cl::aliasopt(Wide));
+
 static StringRef ToolName;
 
 typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
 
 SectionFilter llvm::ToolSectionFilter(llvm::object::ObjectFile const &O) {
   return SectionFilter(
       [](llvm::object::SectionRef const &S) {
         if (FilterSections.empty())
public:
   virtual void printInst(MCInstPrinter &IP, const MCInst *MI,
                          ArrayRef<uint8_t> Bytes,
                          object::SectionedAddress Address, raw_ostream &OS,
                          StringRef Annot, MCSubtargetInfo const &STI,
                          SourcePrinter *SP,
                          std::vector<RelocationRef> *Rels = nullptr) {
     if (SP && (PrintSource || PrintLines))
       SP->printSourceLine(OS, Address);
-    if (!NoLeadingAddr)
+    unsigned Column = 0;
+    if (!NoLeadingAddr) {
       OS << format("%8" PRIx64 ":", Address.Address);
+      Column += 9;
+    }
     if (!NoShowRawInsn) {
       OS << "\t";
+      Column = (Column + 7) & -8;
       dumpBytes(Bytes, OS);
+      Column += 3 * Bytes.size();
     }
+    // The output of printInst always starts with a tab. Print some spaces so
+    // that the tab advances to a tab stop.
+    unsigned TabStop = NoShowRawInsn ? 16 : 40;
+    OS.indent(Column < TabStop - 1 ? TabStop - 1 - Column : 7 - (Column & 7));
     if (MI)
       IP.printInst(MI, OS, "", STI);
     else
-      OS << " <unknown>";
+      OS << "\t<unknown>";
   }
 };
 PrettyPrinter PrettyPrinterInst;
 class HexagonPrettyPrinter : public PrettyPrinter {
 public:
   void printLead(ArrayRef<uint8_t> Bytes, uint64_t Address,
                  raw_ostream &OS) {
     uint32_t opcode =
       (Bytes[3] << 24) | (Bytes[2] << 16) | (Bytes[1] << 8) | Bytes[0];
     if (!NoLeadingAddr)
       OS << format("%8" PRIx64 ":", Address);
     if (!NoShowRawInsn) {