[PATCH] D65191: [llvm-objdump] Implement highlighting

Wed Aug 7 04:59:54 PDT 2019

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-objdump/X86/highlight.test:1
+## Show that llvm-objdump highlights symbol names, registers, and immediate values by default.
+
----------------
Let's rename this test "disassembly-highlighting.test" (or whatever best matches the naming style of other disassembly tests in llvm-objdump). In particular, saying "disassemble" or "disassembly" or similar in the test name is important.

On that note, this comment should say "when disassembling" somewhere.

================
Comment at: llvm/test/tools/llvm-objdump/X86/highlight.test:3
+
+## This test uses ANSI escape sequences and the x86 disassembler.
+# UNSUPPORTED: system-windows
----------------
You can remove "and the X86 disassembler" now.

================
Comment at: llvm/test/tools/llvm-objdump/X86/highlight.test:44
+    AddressAlign:    0x0000000000000010
+    Content:         31ED4989D15E4889E24883E4F050544C8D059A010000488D0D23010000488D3DF7000000FF15C60A2000F40F1F440000488D3DF10A200055488D05E90A20004839F84889E57419488B059A0A20004885C0740D5DFFE0662E0F1F8400000000005DC30F1F4000662E0F1F840000000000488D3DB10A2000488D35AA0A2000554829FE4889E548C1FE034889F048C1E83F4801C648D1FE7418488B05610A20004885C0740C5DFFE0660F1F8400000000005DC30F1F4000662E0F1F840000000000803D5D0A200000752F48833D370A200000554889E5740C488B3D3A0A2000E80DFFFFFFE848FFFFFFC605350A2000015DC30F1F8000000000F3C3660F1F440000554889E55DE966FFFFFF554889E5C705100A2000C8010000905DC3554889E5B800000000E8E1FFFFFF8B15F10920008B05F309200001D05DC30F1F8000000000415741564989D7415541544C8D25AE07200055488D2DAE072000534189FD4989F64C29E54883EC0848C1FD03E857FEFFFF4885ED742031DB0F1F8400000000004C89FA4C89F64489EF41FF14DC4883C3014839DD75EA4883C4085B5D415C415D415E415FC390662E0F1F840000000000F3C3
+  - Name:            .data
----------------
> The input file should be an executable to test symbol highlighting because currently only RIP-relative symbols are supported. That said, I think this YAML lacks readability too. Do you have any idea?

I don't I'm afraid. You might be able to simulate it using an assembly file with a single text section still.

If you do stick with the YAML, I think you probably have too much content in your sections. You only check for 3 instructions, so you can reduce the bytes in the content down to those few. You can also significantly simplify much of this object. You probably don't need either .data and .bss. You probably don't need the section alignment fields. You probably can simplify the addresses to round numbers. You may not need all those symbols either (if I'm reading the checks correctly, only 'foo' is important). You don't need the entry label. Basically, you probably are better off starting with an empty YAML and building it up to the bare minimum for the test rather than using obj2yaml on a binary you want to simulate.

================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:335

+static cl::opt<bool> Highlight("highlight",
+                               cl::desc("Enable syntax highlighting"),
----------------
seiya wrote:
> jhenderson wrote:
> > seiya wrote:
> > > seiya wrote:
> > > > MaskRay wrote:
> > > > > jhenderson wrote:
> > > > > > MaskRay wrote:
> > > > > > > --color is a more common option name: ls, less, grep, zstd, etc
> > > > > > > 
> > > > > > > Can you investigate a bit if it is possible to have both --color and --color=when ?
> > > > > > LLVM already has a --color option (see for example llvm-dwarfdump output, WithColor::warning() and error() calls etc etc).
> > > > > > 
> > > > > > I'm inclined to think that syntax highlighting can be on by default and just use --color=false (or whatever it is) to turn it off. If we go that route, the --color option should be documented in the llvm-objdump command guide, and also we should check it is in the tool help text (since it's in a different option category).
> > > > > It looks lib/Support/WithColor.cpp#L106 already handles this...
> > > > > 
> > > > > ```
> > > > > bool WithColor::colorsEnabled() {
> > > > >   if (DisableColors)
> > > > >     return false;
> > > > >   if (UseColor == cl::BOU_UNSET)
> > > > >     return OS.has_colors();  // e.g. is a terminal
> > > > >   return UseColor == cl::BOU_TRUE;
> > > > > }
> > > > > ```
> > > > > I'm inclined to think that syntax highlighting can be on by default and just use --color=false (or whatever it is) to turn it off. If we go that route, the --color option should be documented in the llvm-objdump command guide, and also we should check it is in the tool help text (since it's in a different option category).
> > > > 
> > > > What do you think about the compatibility with GNU objdump? Making highlighting enabled by default is incompatible behavior with GNU objcopy. Using with objdump in script (e.g., `llvm-objdump -d foo | grep bar`) won't be broken (it checks if stdout is a tty) so it won't be problem I think.
> > > > 
> > > > Can you investigate a bit if it is possible to have both --color and --color=when ?
> > > Did you mean whether can we allow both `--color` and `--color=true` in command-line options or not and how it is handled in colorsEnabled?
> > > What do you think about the compatibility with GNU objdump?
> > 
> > Compatibility is only important for things that can affect parsers (though I have no idea why anybody would be parsing disassembly...). If this can't affect parser behaviour, then I don't think we need to worry about it. If it can, then it should be off by default.
> I think so too (nobody parse the disassembly output written to a tty not a pipe, I believe).
> 
> Do you think syntax highlighting should be enabled by default? This current implementation enables the feature only if `--color` is explicitly specified (`ColorsEnabled = UseColor == cl::BOU_TRUE;`).
> 
Yes, I think syntax highlighting can be on by default (it is for llvm-dwarfdump for example).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191