Bug 29457

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.