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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> 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.


================
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
+
----------------
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).


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:1
+## Make sure it highlights symbol names, registers, and immediate values.
+
----------------
What is "it" here?

Perhaps make this more verbose: "Show that llvm-objdump highlights ... by default."


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


================
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);
+  }
----------------
MaskRay wrote:
> Use WithColor::changeColor
> Use WithColor::changeColor

This is marked as done, but I don't see it being used?


================
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];
----------------
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()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191