[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

609

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

609

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

609

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

609

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

622

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

622

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

622

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

622

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

[jhenderson]

tools/llvm-objdump/llvm-objdump.cpp

622

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

622

I find parentheses very useful for instant understanding of expressions.

[MaskRay]

tools/llvm-objdump/llvm-objdump.cpp

622

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 %s -o %t
+# RUN: llvm-objdump -d -print-imm-hex %t | sed 'y/\t/ /' | FileCheck -strict-whitespace %s
+
+# RUN: llvm-objdump -d -print-imm-hex -no-show-raw-insn %t | 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: 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>
+
+# 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>
+
+.text
+  retq
+  movq 0x123456(%rip),%rax
+  movabs $0x5555555555555554,%rax
+  lwpval $0x0, 0x40(%rdx,%rax), %r15d
+  lwpins $0x0, 0x1cf01cf0, %r15d
+  .word 0xffff

tools/llvm-objdump/llvm-objdump.cpp

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);
+
+    {
+      formatted_raw_ostream FOS(OS);
       if (!NoLeadingAddr)
-      OS << format("%8" PRIx64 ":", Address.Address);
+        FOS << format("%8" PRIx64 ":", Address.Address);
       if (!NoShowRawInsn) {
-      OS << "\t";
-      dumpBytes(Bytes, OS);
+        FOS << ' ';
+        dumpBytes(Bytes, FOS);
       }
+      FOS.flush();
+      // The output of printInst starts with a tab. Print some spaces so that
+      // the tab has 1 column and advances to the target tab stop.
+      unsigned TabStop = NoShowRawInsn ? 16 : 40;
+      unsigned Column = FOS.getColumn();
+      FOS.indent(Column < TabStop - 1 ? TabStop - 1 - Column : 7 - Column % 8);
+
+      // The dtor calls flush() to ensure the indent comes before printInst().
+    }
+
     if (MI)
       IP.printInst(MI, OS, "", STI);
     else
       OS << "\t<unknown>";
   }
 };
 PrettyPrinter PrettyPrinterInst;
 class HexagonPrettyPrinter : public PrettyPrinter {