[PATCH 0/3] Disassembler styling, bug fixes and customisation

[PATCH 0/3] Disassembler styling, bug fixes and customisation

[Andrew Burgess]

The first commit in this series is a bug fix for objdump's
disassembler styling.

The second and third patches in this series add a mechanism for
customising the colors used by objdumps disassembler output.

---

Andrew Burgess (3):
  objdump: fix extended (256) disassembler colors
  objdump: introduce OBJDUMP_COLORS environment variable
  objdump: allow the disassembler colors to be customized

 binutils/doc/binutils.texi |  83 ++++++++++++
 binutils/objdump.c         | 268 +++++++++++++++++++++++++++----------
 2 files changed, 284 insertions(+), 67 deletions(-)

-- 
2.25.4

[PATCH 1/3] objdump: fix extended (256) disassembler colors

[Andrew Burgess]

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.
---
 binutils/objdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/binutils/objdump.c b/binutils/objdump.c
index 08c335476ff..9259c76c716 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -2247,7 +2247,7 @@ objdump_styled_sprintf (SFILE *f, enum disassembler_style style,
 	{
 	  size_t space = f->alloc - f->pos;
 
-	  if (disassembler_color)
+	  if (disassembler_color == on)
 	    n = snprintf (f->buffer + f->pos, space, "\033[%dm", color);
 	  else
 	    n = snprintf (f->buffer + f->pos, space, "\033[38;5;%dm", color);
-- 
2.25.4

Re: [PATCH 1/3] objdump: fix extended (256) disassembler colors

[Nick Clifton]

Hi Andrew,

> 	PR 29457
> 	* objdump (objdump_styled_sprintf): Check disassembler_color
> 	against an enum value, don't treat it as a bool.

Doh - sorry for missing this.  Patch approved - please apply.

Cheers
   Nick

Re: [PATCH 1/3] objdump: fix extended (256) disassembler colors

[Andrew Burgess]

Nick Clifton via Binutils <binutils@sourceware.org> writes:

> Hi Andrew,
>
>> 	PR 29457
>> 	* objdump (objdump_styled_sprintf): Check disassembler_color
>> 	against an enum value, don't treat it as a bool.
>
> Doh - sorry for missing this.  Patch approved - please apply.

I've pushed this patch.  I'll rework #2 and #3 inline with your feedback
and repost.

Thanks,
Andrew

[PATCH 2/3] objdump: introduce OBJDUMP_COLORS environment variable

[Andrew Burgess]

Add OBJDUMP_COLORS environment variable, and allow this to control the
default behaviour of objdump when --disassembler-color is not used,
and we are disassembling to a terminal.

After this commit:

  commit a88c79b77036e4778e70d62081c3cfd1044bb8e3
  Date:   Tue Aug 9 14:57:48 2022 +0100

      Default to enabling colored disassembly if output is to a terminal.

If objdump is disassembling to a terminal, and the
--disassembler-color argument is not used, then objdump will apply
styling by default as if --disassembler-color=color was used.

However, some users might not like this color on by default approach,
or might prefer to use --disassembler-color=extended-color by default.

With this commit objdump will use the OBJDUMP_COLORS environment
variable to control its behaviour.

With:

  OBJDUMP_COLORS=off

The default behaviour is --disassembler-color=off, that is no colors
are added unless the user explicitly uses --disassembler-color at the
command line.

With:

  OBJDUMP_COLORS=color

Only the basic colors are used, as if --disassembler-color=color was
specified.  This can be overridden by the user at the command line by
explicitly passing --disassembler-color again.

With:

  OBJDUMP_COLORS=extended

Only the extended 256-colors are used, as if
--disassembler-color=extended-color was specified.  This can be
overridden by the user at the command line by explicitly passing
--disassembler-color again.

	* doc/binutils.texi (objdump): Add an @xref to the
	--disassembler-color description.  Add a new section describing
	how to use OBJDUMP_COLORS environment variable.
	* objdump.c (objdump_colors_var): New global.
	(objdump_default_disassembler_color_mode): New function.
	(main): Use objdump_default_disassembler_color_mode.
---
 binutils/doc/binutils.texi | 22 ++++++++++++++++++++++
 binutils/objdump.c         | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 41f38f479f6..6606d2c51ac 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -2830,6 +2830,8 @@
 
 If this option is not specified then the default is to enable color
 output if displaying to a terminal, but not otherwise.
+@xref{Customizing Objdump's Colors}, for more information on
+controlling the colors objdump uses in its disassembler output.
 
 @item -W[lLiaprmfFsoORtUuTgAckK]
 @itemx --dwarf[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,=frames-interp,=str,=str-offsets,=loc,=Ranges,=pubtypes,=trace_info,=trace_abbrev,=trace_aranges,=gdb_index,=addr,=cu_index,=links,=follow-links]
@@ -3009,6 +3011,26 @@
 any other data.
 @end table
 
+@anchor{Customizing Objdump's Colors}
+@section Customizing Objdump's Colors
+
+The environment variable @code{OBJDUMP_COLORS} can be used to control
+the colors objdump uses for it's disassembler output.
+
+When objdump disassembles to a terminal, and the
+@option{--disassembler-color} argument is not used, objdump will, by
+default, behave as if @option{--disassembler-color=color} had been
+used.
+
+If @code{OBJDUMP_COLORS} is set to @code{off} then objdump will behave
+as if @option{--disassembler-color=off} was given by default.
+
+If @code{OBJDUMP_COLORS} is set to @code{color} then objdump will
+behave as if @option{--disassembler-color=color} was given by default.
+
+If @code{OBJDUMP_COLORS} is set to @code{extended} then objdump will
+behave as if @option{--disassembler-color=extended-color} was given by
+default.
 @c man end
 
 @ignore
diff --git a/binutils/objdump.c b/binutils/objdump.c
index 9259c76c716..1231cda3657 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -139,6 +139,9 @@ static enum color_selection
     extended				/* --disassembler-color=extended-color.  */
   } disassembler_color = on_if_terminal_output;
 
+/* The environment variable to read for color information.  */
+static const char *objdump_colors_var = "OBJDUMP_COLORS";
+
 static int dump_any_debugging;
 static int demangle_flags = DMGL_ANSI | DMGL_PARAMS;
 
@@ -2155,6 +2158,32 @@ objdump_sprintf (SFILE *f, const char *format, ...)
   return n;
 }
 
+/* Figure out a default disassembler color mode.  */
+
+static enum color_selection
+objdump_default_disassembler_color_mode (void)
+{
+  enum color_selection mode;
+
+  if (isatty (1))
+    {
+      const char *tmp = getenv (objdump_colors_var);
+
+      if (tmp == NULL || strncmp (tmp, "color", 5) == 0)
+	mode = on;
+      if (strncmp (tmp, "extended", 8) == 0)
+	mode = extended;
+      else if (strncmp (tmp, "off", 3) == 0)
+	mode = off;
+      else
+	mode = on;
+    }
+  else
+    mode = off;
+
+  return mode;
+}
+
 /* Return an integer greater than, or equal to zero, representing the color
    for STYLE, or -1 if no color should be used.  */
 
@@ -5931,7 +5960,7 @@ main (int argc, char **argv)
     }
 
   if (disassembler_color == on_if_terminal_output)
-    disassembler_color = isatty (1) ? on : off;
+    disassembler_color = objdump_default_disassembler_color_mode ();
 
   if (show_version)
     print_version ("objdump");
-- 
2.25.4

Re: [PATCH 2/3] objdump: introduce OBJDUMP_COLORS environment variable

[Nick Clifton]

Hi Andrew,

> Add OBJDUMP_COLORS environment variable, and allow this to control the
> default behaviour of objdump when --disassembler-color is not used,
> and we are disassembling to a terminal.

Whilst this is a nice idea, there is a problem with using environment
variables: users often fail to include them when reporting a bug.

Imagine for example a bug report about the colored disassembly being
broken somehow and the user has forgotten to mention that they have
OBJDUMP_COLORS set to "extended".  We could waste hours trying to
track down the problem only to find that we had been looking at the
wrong piece of code.

I cannot think of a nice way to resolve this problem.  One idea that
does spring to mind is to have objdump generate a message to stdout
when the environment variable is used.  That way there is a good
likelihood that the message will be included in a bug report.  But
this does mean new, extraneous output will now appear in the
disassembly output.



> +static enum color_selection
> +objdump_default_disassembler_color_mode (void)
> +{
> +  enum color_selection mode;
> +
> +  if (isatty (1))
> +    {
> +      const char *tmp = getenv (objdump_colors_var);
> +
> +      if (tmp == NULL || strncmp (tmp, "color", 5) == 0)

Why strncmp() rather than strcmp () ?  [Because of patch 3/3 I expect...]

> +	mode = on;
> +      if (strncmp (tmp, "extended", 8) == 0)
> +	mode = extended;
> +      else if (strncmp (tmp, "off", 3) == 0)
> +	mode = off;
> +      else
> +	mode = on;

It would probably be best to check for strncmp (tmp, "on") here
and report if there is no match - that way users will know that
they have a malformed OBJDUMP_COLOR environment variable.

Cheers
   Nick

[PATCH 3/3] objdump: allow the disassembler colors to be customized

[Andrew Burgess]

This commit extends the use of OBJDUMP_COLORS to allow all the
disassembler colors to be controlled by the user.

The approach used is to have the environment variable OBJDUMP_COLORS
contain a simple map of a two letter code to the appropriate escape
sequence to use. For example:

  OBJDUMP_COLORS="sy=38;5;162:rg=38;5;215"

Makes use of color 162 for symbol names (sy) and color 215 for
register names (rg).  Both of these colors are from the 256 color
space.

Any styles not mentioned in OBJDUMP_COLORS will use their default
colors.

Building on the previous commit, OBJDUMP_COLORS can still control the
default styling mode, for example:

  OBJDUMP_COLORS="extended:sy=38;5;162:rg=38;5;215"

With the addition of the 'extended:' prefix, unless told otherwise by
a command line argument, objdump will use the 256 extended-colors by
default for styling, and symbol names and register names will use the
modified colors.

I don't know if this style of environment encoded settings will be
acceptable or not.  The alternative would be some kind of
configuration file system.  My inspiration for the approach I propose
here is the LS_COLORS variable used by `ls`.

	* doc/binutils.texi (objdump): Extend the description of
	OBJDUMP_COLORS environment variable.
	* objdump.c (disasm_styles): New global.
	(objdump_find_disasm_styles_entry): New function.
	(objdump_fill_styles_array): New function.
	(objdump_color_for_disassembler_style): Complete rewrite, return
	strings from disasm_styles array.
	(objdump_styled_sprintf): Update to account for changes to
	objdump_color_for_disassembler_style.
---
 binutils/doc/binutils.texi |  73 +++++++++++-
 binutils/objdump.c         | 237 ++++++++++++++++++++++++++-----------
 2 files changed, 238 insertions(+), 72 deletions(-)

diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 6606d2c51ac..3955cc954ae 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -3017,20 +3017,81 @@
 The environment variable @code{OBJDUMP_COLORS} can be used to control
 the colors objdump uses for it's disassembler output.
 
+The format of @code{OBJDUMP_COLORS} is given by @var{<objdump_colors>}
+in:
+
+@smallexample
+<objdump_colors> ::= <mode_selection> ":" <style_list>
+                   | <mode_selection>
+                   | <style_list>
+
+<mode_selection> ::= "OFF" | "B" | "X"
+
+<style_list> ::= <style_item>
+               | <style_item> ":" <style_list>
+
+<style_item> ::= <style_code> "=" <escape_string>
+
+<style_code> ::= "ad" | "ao" | "as" | "cm" | "im"
+               | "mn" | "rg" | "sm" | "sy" | "tx"
+@end smallexample
+
 When objdump disassembles to a terminal, and the
 @option{--disassembler-color} argument is not used, objdump will, by
 default, behave as if @option{--disassembler-color=color} had been
 used.
 
-If @code{OBJDUMP_COLORS} is set to @code{off} then objdump will behave
-as if @option{--disassembler-color=off} was given by default.
+If @var{<mode_selection>} is set to @code{off} then objdump will
+behave as if @option{--disassembler-color=off} was given by default.
 
-If @code{OBJDUMP_COLORS} is set to @code{color} then objdump will
-behave as if @option{--disassembler-color=color} was given by default.
+If @var{<mode_selection>} is set to @code{color} then objdump will behave
+as if @option{--disassembler-color=color} was given by default.
 
-If @code{OBJDUMP_COLORS} is set to @code{extended} then objdump will
-behave as if @option{--disassembler-color=extended-color} was given by
+If @var{<mode_selection>} is set to @code{extended} then objdump will behave
+as if @option{--disassembler-color=extended-color} was given by
 default.
+
+Each @var{<style_code>} is a two letter sequence that identifies a
+particular part of the disassembler output, the valid codes are:
+
+@itemize @bullet
+@item ad
+The style used to for numerical addresses.
+@item ao
+The style used for numerical address offsets.
+@item as
+The style used for assembler directives.
+@item cm
+The style used for comments.
+@item im
+The style used for numerical immediates.
+@item mn
+The style used for the primary instruction mnemonic.
+@item rg
+The style used for register names.
+@item sm
+The style used for any secondary mnemonics within an instruction.
+@item sy
+The style used for symbol names.
+@item tx
+The style used for anything that is not covered by any of the above
+styles.  This style should not usually be modified, this will leave
+this text styled the same as the rest of objdump's output.
+@end itemize
+
+Each @var{<style_item>} associates a @var{<style_code>} with an
+@var{<escape_string>}.  Each escape string is a series of digits and
+@code{;} characters, and will be used as part of an escape sequence to
+add styling to the disassembler output.  Each @var{<escape_string>}
+has @code{\033[} added as a prefix, and @code{m} added as a suffix to
+create the complete escape sequence.
+
+Alternatively, if an @var{<escape_string>} is empty then no styling is
+added to this element of the disassembler output.
+
+Any styles specified in the @var{<style_list>} override the
+corresponding style in the objdump output.  Any style not specified in
+the @var{<style_list>} will retain its default styling.
 @c man end
 
 @ignore
diff --git a/binutils/objdump.c b/binutils/objdump.c
index 1231cda3657..56fe051b62b 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -2184,79 +2184,186 @@ objdump_default_disassembler_color_mode (void)
   return mode;
 }
 
-/* Return an integer greater than, or equal to zero, representing the color
-   for STYLE, or -1 if no color should be used.  */
+/* An array of strings, one for each entry in enum disassembler_style.
+   Each string here is either NULL or something that can be used in an
+   escape sequence to style the disassembler output.  */
+static const char *disasm_styles[10] = { NULL };
 
-static int
-objdump_color_for_disassembler_style (enum disassembler_style style)
+/* CODE should be a string starting 'XX=' where 'XX' is a two letter style
+   code.  This function returns a pointer to the entry in the global
+   disasm_styles array that corresponds to 'XX', or NULL if there is no
+   suitable entry.
+
+   Code must not be NULL.  If this function returns non-NULL then CODE is
+   guaranteed to start with the 3 character pattern 'XX=', but nothing is
+   guaranteed after that.
+
+   Care is taken to ensure we don't index outside the bounds of
+   disasm_style, which might happen if new styles have been added and
+   disasm_styles has not been extended correctly.  */
+
+static const char **
+objdump_find_disasm_styles_entry (const char *code)
 {
-  int color = -1;
+  enum disassembler_style style;
+  unsigned int style_idx;
+
+  if (strncmp ("ad=", code, 3) == 0)
+    style = dis_style_address;
+  else if (strncmp ("ao=", code, 3) == 0)
+    style = dis_style_address_offset;
+  else if (strncmp ("as=", code, 3) == 0)
+    style = dis_style_assembler_directive;
+  else if (strncmp ("cm=", code, 3) == 0)
+    style = dis_style_comment_start;
+  else if (strncmp ("im=", code, 3) == 0)
+    style = dis_style_immediate;
+  else if (strncmp ("mn=", code, 3) == 0)
+    style = dis_style_mnemonic;
+  else if (strncmp ("rg=", code, 3) == 0)
+    style = dis_style_register;
+  else if (strncmp ("sm=", code, 3) == 0)
+    style = dis_style_sub_mnemonic;
+  else if (strncmp ("sy=", code, 3) == 0)
+    style = dis_style_symbol;
+  else if (strncmp ("tx=", code, 3) == 0)
+    style = dis_style_text;
+  else
+    return NULL;
 
-  if (style == dis_style_comment_start)
-    disassembler_in_comment = true;
+  style_idx = (unsigned int) style;
+  if (style_idx > (sizeof (disasm_styles) / sizeof (disasm_styles[0])))
+    return NULL;
+
+  return &disasm_styles[style_idx];
+}
+
+/* This function should be called at most once, before we first try to use
+   the global disasm_styles array.  This function fills the disasm_styles
+   array with suitable strings based on which disassembler-color arguments
+   the user passed to objdump, as well as the OBJDUMP_COLORS environment
+   variable, if this is set.  */
 
+static void
+objdump_fill_styles_array (void)
+{
   if (disassembler_color == on)
     {
-      if (disassembler_in_comment)
-	return color;
-
-      switch (style)
-	{
-	case dis_style_symbol:
-	  color = 32;
-	  break;
-        case dis_style_assembler_directive:
-	case dis_style_sub_mnemonic:
-	case dis_style_mnemonic:
-	  color = 33;
-	  break;
-	case dis_style_register:
-	  color = 34;
-	  break;
-	case dis_style_address:
-        case dis_style_address_offset:
-	case dis_style_immediate:
-	  color = 35;
-	  break;
-	default:
-	case dis_style_text:
-	  color = -1;
-	  break;
-	}
+      /* The defaults colors for --disassembler-color=color.  */
+      disasm_styles[(unsigned int) dis_style_address] = "35";
+      disasm_styles[(unsigned int) dis_style_address_offset] = "35";
+      disasm_styles[(unsigned int) dis_style_assembler_directive] = "33";
+      disasm_styles[(unsigned int) dis_style_comment_start] = NULL;
+      disasm_styles[(unsigned int) dis_style_immediate] = "35";
+      disasm_styles[(unsigned int) dis_style_mnemonic] = "33";
+      disasm_styles[(unsigned int) dis_style_register] = "34";
+      disasm_styles[(unsigned int) dis_style_sub_mnemonic] = "33";
+      disasm_styles[(unsigned int) dis_style_symbol] = "32";
+      disasm_styles[(unsigned int) dis_style_text] = NULL;
     }
   else if (disassembler_color == extended)
     {
-      if (disassembler_in_comment)
-	return 250;
+      /* The defaults colors for --disassembler-color=extended-color.  */
+      disasm_styles[(unsigned int) dis_style_address] = "38;5;134";
+      disasm_styles[(unsigned int) dis_style_address_offset] = "38;5;134";
+      disasm_styles[(unsigned int) dis_style_assembler_directive] = "38;5;142";
+      disasm_styles[(unsigned int) dis_style_comment_start] = "38;5;250";
+      disasm_styles[(unsigned int) dis_style_immediate] = "38;5;134";
+      disasm_styles[(unsigned int) dis_style_mnemonic] = "38;5;142";
+      disasm_styles[(unsigned int) dis_style_register] = "38;5;27";
+      disasm_styles[(unsigned int) dis_style_sub_mnemonic] = "38;5;142";
+      disasm_styles[(unsigned int) dis_style_symbol] = "38;5;40";
+      disasm_styles[(unsigned int) dis_style_text] = NULL;
+    }
+  else if (disassembler_color != off)
+    bfd_fatal (_("disassembly color not correctly selected"));
 
-      switch (style)
+  if (disassembler_color != off)
+    {
+      const char *env_var = getenv (objdump_colors_var);
+      if (env_var != NULL)
 	{
-	case dis_style_symbol:
-	  color = 40;
-	  break;
-        case dis_style_assembler_directive:
-	case dis_style_sub_mnemonic:
-	case dis_style_mnemonic:
-	  color = 142;
-	  break;
-	case dis_style_register:
-	  color = 27;
-	  break;
-	case dis_style_address:
-        case dis_style_address_offset:
-	case dis_style_immediate:
-	  color = 134;
-	  break;
-	default:
-	case dis_style_text:
-	  color = -1;
-	  break;
+	  /* Create a copy of the colors environment variable.  The
+	     disasm_styles array will point directly into our copy, which
+	     we will modify to insert '\0' at appropriate places.  */
+	  static char *env_colors = NULL;
+	  env_colors = strdup (env_var);
+	  const char **style_entry;
+	  char *ptr;
+
+	  /* Skip a leading color mode specifier.  */
+	  if (strncmp (env_colors, "off", 3) == 0
+	      || strncmp (env_colors, "color", 5) == 0
+	      || strncmp (env_colors, "extended", 8) == 0)
+	    {
+	      while (*env_colors != ':' && *env_colors != '\0')
+		++env_colors;
+	    }
+
+	  /* Skip any leading colon.  */
+	  if (env_colors[0] == ':')
+	    env_colors++;
+
+	  ptr = env_colors;
+	  while (*ptr != '\0')
+	    {
+	      style_entry = objdump_find_disasm_styles_entry (ptr);
+	      if (style_entry != NULL)
+		{
+		  ptr += 3;
+		  if (*ptr == '\0' || *ptr == ':')
+		    *style_entry = NULL;
+		  else
+		    {
+		      const char *esc_str = ptr;
+
+		      for (; *ptr != '\0' && *ptr != ':'; ++ptr)
+			{
+			  if (!ISDIGIT (*ptr) && *ptr != ';')
+			    esc_str = NULL;
+			}
+		      *style_entry = esc_str;
+		    }
+		}
+
+	      if (*ptr == ':')
+		{
+		  *ptr = '\0';
+		  ptr++;
+		}
+	    }
 	}
     }
-  else if (disassembler_color != off)
-    bfd_fatal (_("disassembly color not correctly selected"));
+}
+
+/* Return the escape code sequence for STYLE, or NULL if no escape code
+   should be emitted for this style.  */
+
+static const char *
+objdump_color_for_disassembler_style (enum disassembler_style style)
+{
+  unsigned int style_idx;
+  static bool color_init = false;
+
+  if (!color_init)
+    {
+      objdump_fill_styles_array ();
+      color_init = true;
+    }
+
+  /* If this is the start of a comment, or we previously saw the start of a
+     comment, then force the style to comment style.  */
+  if (style == dis_style_comment_start)
+    disassembler_in_comment = true;
+  if (disassembler_in_comment)
+    style = dis_style_comment_start;
+
+  /* Select and return the appropriate style from the styles array.  */
+  style_idx = (unsigned int) style;
+  if (style_idx > (sizeof (disasm_styles) / sizeof (disasm_styles[0])))
+    return NULL;
+  return disasm_styles[style_idx];
 
-  return color;
 }
 
 /* Like objdump_sprintf, but add in escape sequences to highlight the
@@ -2268,18 +2375,16 @@ objdump_styled_sprintf (SFILE *f, enum disassembler_style style,
 {
   size_t n;
   va_list args;
-  int color = objdump_color_for_disassembler_style (style);
+  const char *color_esc_seq = objdump_color_for_disassembler_style (style);
 
-  if (color >= 0)
+  if (color_esc_seq != NULL)
     {
       while (1)
 	{
 	  size_t space = f->alloc - f->pos;
 
-	  if (disassembler_color == on)
-	    n = snprintf (f->buffer + f->pos, space, "\033[%dm", color);
-	  else
-	    n = snprintf (f->buffer + f->pos, space, "\033[38;5;%dm", color);
+	  n = snprintf (f->buffer + f->pos, space, "\033[%sm",
+			color_esc_seq);
 	  if (space > n)
 	    break;
 
@@ -2305,7 +2410,7 @@ objdump_styled_sprintf (SFILE *f, enum disassembler_style style,
     }
   f->pos += n;
 
-  if (color >= 0)
+  if (color_esc_seq != NULL)
     {
       while (1)
 	{
-- 
2.25.4

Re: [PATCH 3/3] objdump: allow the disassembler colors to be customized

[Nick Clifton]

Hi Andrew,

> I don't know if this style of environment encoded settings will be
> acceptable or not.  

If we are going to accept an environment variable for patch 2/3 then
I see no reason why it cannot be extended here.

> The alternative would be some kind of
> configuration file system. 

Meh - of the two I prefer environment variables over configuration files.
Environment variables are easier to change in a nested fashion, configuration
files are not.

> My inspiration for the approach I propose
> here is the LS_COLORS variable used by `ls`.

Agreed - there is precedence for this kind of behaviour.

> +<mode_selection> ::= "OFF" | "B" | "X"

Shouldn't the modes be "off", "on" and "extended" ?

> +<style_item> ::= <style_code> "=" <escape_string>

Your syntax description does not document "<escape string>".
It really should do so, or else provide a reference to
where it is documentede.

> +<style_code> ::= "ad" | "ao" | "as" | "cm" | "im"
> +               | "mn" | "rg" | "sm" | "sy" | "tx"

I am a big fan of long descriptive names.  It makes things
much easier to read.  So by all means, allow shortened versions
if you want, but it would be really nice to support names like
"registers", "addresses", "symbols" etc.



> +      /* The defaults colors for --disassembler-color=color.  */
> +      disasm_styles[(unsigned int) dis_style_address] = "35";
> +      disasm_styles[(unsigned int) dis_style_address_offset] = "35";

You might as well replace these magic numbers (eg 35) with #define'd
constants.  That would be more descriptive.  (Assuming that the defined
names are color names of course).


Cheers
   Nick