Page MenuHomePhabricator

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

Authored by MaskRay on Apr 7 2019, 5:58 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Apr 7 2019, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2019, 5:58 AM
MaskRay updated this revision to Diff 194059.Apr 7 2019, 6:01 AM

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.

Harbormaster completed remote builds in B30145: Diff 194059.
MaskRay updated this revision to Diff 194060.Apr 7 2019, 6:05 AM

Add <unknown> to the test

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
2 ↗(On Diff #194060)

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

MaskRay updated this revision to Diff 194447.Apr 9 2019, 11:00 PM
MaskRay retitled this revision from [llvm-objdump] Align instructions to a tab stop and accept (and ignore) -w/--wide to [llvm-objdump] Align instructions to a tab stop.
MaskRay edited the summary of this revision. (Show Details)

Use formatted_raw_ostream to track current column

MaskRay updated this revision to Diff 194448.Apr 9 2019, 11:12 PM

Use LWP instructions

MaskRay marked 2 inline comments as done.Apr 9 2019, 11:16 PM
MaskRay added inline comments.
test/tools/llvm-objdump/X86/disassemble-align.s
2 ↗(On Diff #194060)

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.

MaskRay marked an inline comment as done.Apr 9 2019, 11:18 PM
jhenderson added inline comments.Apr 10 2019, 7:00 AM
test/tools/llvm-objdump/X86/disassemble-align.s
4 ↗(On Diff #194448)

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 ↗(On Diff #194448)

Why do you need to scope this block?

MaskRay marked 2 inline comments as done.Apr 10 2019, 7:20 AM
MaskRay added inline comments.
test/tools/llvm-objdump/X86/disassemble-align.s
4 ↗(On Diff #194448)

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 ↗(On Diff #194448)

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 added inline comments.Apr 11 2019, 2:56 AM
test/tools/llvm-objdump/X86/disassemble-align.s
4 ↗(On Diff #194448)

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 ↗(On Diff #194448)

Okay. This probably deserves a comment then.

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

MaskRay updated this revision to Diff 194800.Apr 11 2019, 7:53 PM

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

MaskRay marked 3 inline comments as done.Apr 11 2019, 7:58 PM
MaskRay added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
609 ↗(On Diff #194448)

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 added inline comments.Apr 12 2019, 2:55 AM
tools/llvm-objdump/llvm-objdump.cpp
622 ↗(On Diff #194800)

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 marked 2 inline comments as done.Apr 12 2019, 7:28 AM
MaskRay added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
622 ↗(On Diff #194800)

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 added inline comments.Apr 12 2019, 7:41 AM
tools/llvm-objdump/llvm-objdump.cpp
622 ↗(On Diff #194800)

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 marked an inline comment as done.Apr 12 2019, 6:34 PM
MaskRay added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
622 ↗(On Diff #194800)

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

jhenderson accepted this revision.Apr 15 2019, 4:45 AM
jhenderson added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
622 ↗(On Diff #194800)

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

This revision is now accepted and ready to land.Apr 15 2019, 4:45 AM
MaskRay updated this revision to Diff 195172.Apr 15 2019, 6:23 AM
MaskRay retitled this revision from [llvm-objdump] Align instructions to a tab stop to [llvm-objdump] Align instructions to a tab stop in disassembly output.
MaskRay edited the summary of this revision. (Show Details)

Update description

MaskRay updated this revision to Diff 195173.Apr 15 2019, 6:26 AM

Further update description

This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Apr 15 2019, 2:19 PM

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?

Reverted in r358459

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

@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 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?

@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.

Changed to | in D60777

sidneym added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
622 ↗(On Diff #194800)

I find parentheses very useful for instant understanding of expressions.

MaskRay marked 2 inline comments as done.Apr 16 2019, 10:50 PM
MaskRay added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
622 ↗(On Diff #194800)

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.