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

[Seiya Nuta via Phabricator via llvm-commits]

seiya added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:1
+# RUN: llvm-objdump -d --highlight %p/Inputs/highlight.elf-x86_64 | FileCheck %s
+# REQUIRES: x86-registered-target
----------------
jhenderson wrote:
> jhenderson wrote:
> > Try and avoid adding a binary if you can. There's no particular reason this can't use yaml2obj to do things. Just add a .text section with appropriate content.
> I'm being dumb. This should probably just be an assembly file and use llvm-mc to compile it. That'll make the input text more readable too. Also, the test should be in the X86 directory.
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?


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:4
+## This test uses ANSI escape sequences and the x86 disassembler.
+# REQUIRES: shell, x86-registered-target
+
----------------
jhenderson wrote:
> MaskRay wrote:
> > ```
> > # UNSUPPORTED: system-windows
> > # REQUIRES: shell, x86-registered-target
> > ```
> Do we need both `UNSUPPORTED: system-windows` and `REQUIRES: shell`? I'd think the latter effectively implies the former (but could be wrong).
Basically, `REQUIRES: shell` implies that the test environment is not Windows, but it looks that setting `$LIT_USE_INTERNAL_SHELL` forces running tests on Windows that requires `shell` [1].

Namely, `UNSUPPORTED: system-windows` disables this test on Windows even if `$LIT_USE_INTERNAL_SHELL` is set.

I'm not familiar with Windows, but I think this it makes sense because I suppose that SetConsoleTextAttribute API does not emit ANSI escape sequences.

[1]: https://reviews.llvm.org/D31423


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:1
+## Make sure it highlights symbol names, registers, and immediate values.
+
----------------
jhenderson wrote:
> What is "it" here?
> 
> Perhaps make this more verbose: "Show that llvm-objdump highlights ... by default."
> What is "it" here?
"it" referred to llvm-objdump. Fixed the comment.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:335
 
+static cl::opt<bool> Highlight("highlight",
+                               cl::desc("Enable syntax highlighting"),
----------------
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;`).



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:372
+    std::pair<raw_ostream::Colors, bool> Pair = HighlightColors[Color];
+    outs().changeColor(Pair.first, Pair.second);
+  }
----------------
jhenderson wrote:
> MaskRay wrote:
> > Use WithColor::changeColor
> > Use WithColor::changeColor
> 
> This is marked as done, but I don't see it being used?
I've replaced `outs().changeColor` with `WithColor`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:368-370
+  int ColorIdx = static_cast<int>(Color);
+  assert(ColorIdx < static_cast<int>(ObjdumpColor::Sentinel));
+  auto Pair = HighlightColors[ColorIdx];
----------------
jhenderson wrote:
> Rather than static casting things to int, for the assert, compare the enum values directly:
> 
> `assert(Color != ObjdumpColor::Sentinel)`
> 
> The whole point of a enum class is that you can't implicitly convert from/to them. You shouldn't be casting them ever.
> 
> Also add a message to the assert i.e. `assert(... && "Color cannot be Sentinel");`.
> 
> This all becomes moot if you follow this suggestion: instead of `HighlightColors` being an array, it should probably be a `std::unordered_map` or similar with `ObjdumpColor` as the key. That way you don't need the cast for the lookup, although you will need to check against `HighlightColors.end()`.
I used an array since it should be faster lookup but it was a premature optimization. I've replaced with `std::map` instead of `std::unordered_map` here. Enum class as key of std::unordered_map is not supported because of `"the specified hash does not meet the Hash requirements"` (I must have misunderstood something... it is supported in C++14).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191