Bug 29457 - Consider making --disassembler-color=color default if terminal
Summary: Consider making --disassembler-color=color default if terminal
Status: REOPENED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.40 (HEAD)
: P2 normal
Target Milestone: ---
Assignee: Nick Clifton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-08 11:50 UTC by Martin Liska
Modified: 2022-10-31 09:37 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2022-08-08 00:00:00


Attachments
Proposed Patch (1.47 KB, patch)
2022-08-08 15:39 UTC, Nick Clifton
Details | Diff
Proposed patch (2.11 KB, patch)
2022-10-21 13:33 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liska 2022-08-08 11:50:18 UTC
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!
Comment 1 Nick Clifton 2022-08-08 15:39:07 UTC
Created attachment 14263 [details]
Proposed Patch

Something like this ?
Comment 2 Martin Liska 2022-08-09 09:05:50 UTC
Yes, thanks for it. I've just tested the patch and it looks nice!
Comment 3 cvs-commit@gcc.gnu.org 2022-08-09 13:58:24 UTC
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.
Comment 4 Martin Liska 2022-08-09 18:04:20 UTC
Thanks for the implementation.
Comment 5 cvs-commit@gcc.gnu.org 2022-08-10 16:12:10 UTC
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.
Comment 6 Jan Beulich 2022-10-19 15:01:28 UTC
(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?
Comment 7 Nick Clifton 2022-10-19 15:08:31 UTC
(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
Comment 8 Jan Beulich 2022-10-19 15:14:03 UTC
(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.
Comment 9 Nick Clifton 2022-10-21 13:33:53 UTC
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
Comment 10 Jan Beulich 2022-10-24 07:41:25 UTC
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.
Comment 11 cvs-commit@gcc.gnu.org 2022-10-31 09:37:30 UTC
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.