Similar to other tools, please consider colored output when the output goes to the terminal (and not a file). Thanks! Anyway, it's a nice feature, I like it!
Created attachment 14263 [details] Proposed Patch Something like this ?
Yes, thanks for it. I've just tested the patch and it looks nice!
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a88c79b77036e4778e70d62081c3cfd1044bb8e3 commit a88c79b77036e4778e70d62081c3cfd1044bb8e3 Author: Nick Clifton <nickc@redhat.com> Date: Tue Aug 9 14:57:48 2022 +0100 Default to enabling colored disassembly if output is to a terminal. PR 29457 * objdump.c (disassembler_color): Change type to an enum. (disassembler_extended_color): Remove. (usage): Update. (objdump_color_for_assembler_style): Update. (main): Update initialisation of disassembler_color. If not initialised via a command line option, set based upon terminal output. * doc/binutils.texi: Update description of disassmbler-color option. * testsuite/binutils-all/arc/objdump.exp: Add --disassembler-color=off option when disassembling. * testsuite/binutils-all/arm/objdump.exp: Likewise.
Thanks for the implementation.
The master branch has been updated by Andrew Burgess <aburgess@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=daf2618a918f2fd338e2519b51d7599943ccb3e8 commit daf2618a918f2fd338e2519b51d7599943ccb3e8 Author: Andrew Burgess <aburgess@redhat.com> Date: Wed Aug 10 12:42:35 2022 +0100 objdump: fix extended (256) disassembler colors After commit: commit a88c79b77036e4778e70d62081c3cfd1044bb8e3 Date: Tue Aug 9 14:57:48 2022 +0100 Default to enabling colored disassembly if output is to a terminal. The 256 extended-color support for --disassembler-color was broken. This is fixed in this commit. PR 29457 * objdump (objdump_styled_sprintf): Check disassembler_color against an enum value, don't treat it as a bool.
(In reply to Martin Liska from comment #2) > Yes, thanks for it. I've just tested the patch and it looks nice! I'm afraid I disagree. It not only doesn't look nice, especially register names are close to unreadable on a terminal with a black background. Nick - I'd like to ask that this be reconsidered. When Andrew first introduced the feature, I specifically asked that coloring not be made the default. And now it suddenly is (the patch wasn't even sent to the list afaics, which is likely part of the reason why I didn't notice until now). I would have been kind of okay if the envvar override had been accepted, but now I need to always remember to add a pretty long option to objdump, just for its output to be at least readable?
(In reply to Jan Beulich from comment #6) Hi Jan, > I'm afraid I disagree. It not only doesn't look nice, especially register > names are close to unreadable on a terminal with a black background. Nick - > I'd like to ask that this be reconsidered. When Andrew first introduced the > feature, I specifically asked that coloring not be made the default. And now > it suddenly is Sorry - that is my bad. > I would have been > kind of okay if the envvar override had been accepted, but now I need to > always remember to add a pretty long option to objdump, just for its output > to be at least readable? Fair enough. What do think would be better: using an environment variable to set the colours or having a configure time option which decides upon the default behaviour ? Or both ? Cheers Nick
(In reply to Nick Clifton from comment #7) > What do think would be better: using an environment variable to > set the colours or having a configure time option which decides upon the > default behaviour ? Or both ? Having both would of course be most flexible. I recall though that you had (imo valid) concerns regarding the use of an envvar. For a possible configure option, I'd like to ask that even that default to off. At the very least until colored output is readable regardless of terminal settings.
Created attachment 14410 [details] Proposed patch Hi Jan, Well I am still not a fan of environment variables, so how about this patch ? It adds a new configure time option --enable-colored-disassembly which defaults to "off'. It also cleans up the arguments to the --disassembler-color= option so that there are now four alternatives: off, on, terminal and extended. (Plus aliases of these options to match the old values). Any suggestions/improvements ? Cheers Nick
Looks okay to me, with perhaps two remarks: "color" and "colour" are aliases of "on" according to the code. Documentation may want to reflect this. And then, with my observation of output potentially being (close to?) unreadable, I guess there may want to be a warning (for the time being) in this regard? I admit it's not really clear to me where such a warning would best go.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=18bf56434d1adabbd82fa6a33d08a60ef2d71001 commit 18bf56434d1adabbd82fa6a33d08a60ef2d71001 Author: Nick Clifton <nickc@redhat.com> Date: Mon Oct 31 09:35:16 2022 +0000 objdump: Add configure time option to enable colored disassembly output by default. PR 29457 * configure.ac: Add --enable-colored-disassembly. * objdump.c: Add --disassembler-color=terminal. * doc/binutils.texi (objdump): Document the new option. * NEWS: Mention new feature. * config.in: Regenerate in. * configure: Regenerate.
Let's close it again as there's a new configure option added: --disassembler-color.