Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH] gdb/tui: use init_extended_color where possible
@ 2026-07-21 14:24 Andrew Burgess
  2026-08-05 11:50 ` [PATCHv2] " Andrew Burgess
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Burgess @ 2026-07-21 14:24 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess, jakob.schaeffeler

After commit:

  commit fbe7f20a0f098ca03913452b29f50f0dc8568f77
  Date:   Sat May 9 23:27:43 2026 +0200

    gdb/tui: fix unexpected reuse of color pairs

which converted GDB to use init_extended_pair where possible, I
realised we could also make use of init_extended_color.

The motivation for using init_extended_color is slightly less than
init_extended_pair.  Assuming the terminal supports it the standard
init_color API supports up to SHRT_MAX (32767) different colors,
switching to init_extended_color removes the SHRT_MAX limit on color
indices, allowing us to support the full range of COLORS.

But the cost of making this change is minimal, we already track the
color indices as an `int` within the global COLOR_MAP, so it's mostly
just a case of calling init_extended_color where needed.

We only use init_extended_color when both that function and
init_extended_pair is available.  The fallback to init_extended_pair
is init_pair, which expects the color indices to be shorts.  If we are
using the init_pair fallback then using init_extended_color is
pointless.

In reality init_extended_pair and init_extended_color were both added
in ncurses 6.1, so should both be available together.

There is one additional change in here.  Assuming that a terminal does
support more than SHRT_MAX colours, but for some reason GDB is
compiled with a version of the curses library that doesn't support
init_extended_color, then it is possible that in `get_color` the value
of NEXT could end up above SHRT_MAX, in which case the `init_color`
call will truncate the value of NEXT to a short and we will end up
redefining an earlier color index.  To avoid this unlikely case I've
added a compare against SHRT_MAX.

The init_extended_color path doesn't have this risk as COLORS is an
`int` and NEXT is passed as an `int` on this path so there is no risk
of truncation.
---
 gdb/config.in    |  3 +++
 gdb/configure    |  1 +
 gdb/configure.ac |  1 +
 gdb/tui/tui-io.c | 20 +++++++++++++++++---
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/gdb/config.in b/gdb/config.in
index 1ef5dc5c2f4..74745665d4d 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -250,6 +250,9 @@
 /* Define to 1 if you have the `iconvlist' function. */
 #undef HAVE_ICONVLIST
 
+/* Define to 1 if you have the `init_extended_color' function. */
+#undef HAVE_INIT_EXTENDED_COLOR
+
 /* Define to 1 if you have the `init_extended_pair' function. */
 #undef HAVE_INIT_EXTENDED_PAIR
 
diff --git a/gdb/configure b/gdb/configure
index 303d6ea011c..fe58ba47e73 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -30238,6 +30238,7 @@ for ac_func in  \
   getrlimit \
   getuid \
   iconvlist \
+  init_extended_color \
   init_extended_pair \
   libiconvlist \
   posix_madvise \
diff --git a/gdb/configure.ac b/gdb/configure.ac
index e55a733fba7..3d8847df9d8 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1520,6 +1520,7 @@ AC_CHECK_FUNCS([ \
   getrlimit \
   getuid \
   iconvlist \
+  init_extended_color \
   init_extended_pair \
   libiconvlist \
   posix_madvise \
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 896d00f44c8..1c430f2ea45 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -252,10 +252,24 @@ get_color (const ui_file_style::color &color, int *result)
 	  if (next >= COLORS)
 	    return false;
 	  rgb_color rgb = color.get_rgb ();
-	  /* We store RGB as 0..255, but curses wants 0..1000.  */
-	  if (init_color (next, rgb[0] * 1000 / 255, rgb[1] * 1000 / 255,
-			  rgb[2] * 1000 / 255) == ERR)
+	  /* We store RGB as 0..255, but curses wants 0..1000.  There's no
+	     point using init_extended_color if init_extended_pair is not
+	     available as the fallback init_pair can only handle short
+	     color IDs.  */
+#if defined HAVE_INIT_EXTENDED_COLOR && defined HAVE_INIT_EXTENDED_PAIR
+	  if (init_extended_color (next, rgb[0] * 1000 / 255,
+				   rgb[1] * 1000 / 255,
+				   rgb[2] * 1000 / 255) == ERR)
 	    return false;
+#else
+	  /* NEXT is an int, but is passed as a short.  If COLORS is
+	     more than SHRT_MAX then NEXT will be truncated and end up
+	     redefining a color entry that we don't expect.  */
+	  if (next > SHRT_MAX
+	      || init_color (next, rgb[0] * 1000 / 255, rgb[1] * 1000 / 255,
+			     rgb[2] * 1000 / 255) == ERR)
+	    return false;
+#endif
 	  color_map[color] = next;
 	  *result = next;
 	}

base-commit: fbe7f20a0f098ca03913452b29f50f0dc8568f77
-- 
2.25.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCHv2] gdb/tui: use init_extended_color where possible
  2026-07-21 14:24 [PATCH] gdb/tui: use init_extended_color where possible Andrew Burgess
@ 2026-08-05 11:50 ` Andrew Burgess
  2026-08-12 19:26   ` Simon Marchi
  2026-08-12 19:44   ` Tom Tromey
  0 siblings, 2 replies; 5+ messages in thread
From: Andrew Burgess @ 2026-08-05 11:50 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

In v2:

  - Split the scaling of GDB's RGB value out from the init_color and
    init_extended_color calls.  This removes some code duplication and
    allows the comment to sit closer to the code in question.

  - Rebase to current HEAD and retest.

---

After commit:

  commit fbe7f20a0f098ca03913452b29f50f0dc8568f77
  Date:   Sat May 9 23:27:43 2026 +0200

    gdb/tui: fix unexpected reuse of color pairs

which converted GDB to use init_extended_pair where possible, I
realised we could also make use of init_extended_color.

The motivation for using init_extended_color is slightly less than
init_extended_pair.  Assuming the terminal supports it the standard
init_color API supports up to SHRT_MAX (32767) different colors,
switching to init_extended_color removes the SHRT_MAX limit on color
indices, allowing us to support the full range of COLORS.

But the cost of making this change is minimal, we already track the
color indices as an `int` within the global COLOR_MAP, so it's mostly
just a case of calling init_extended_color where needed.

We only use init_extended_color when both that function and
init_extended_pair is available.  The fallback to init_extended_pair
is init_pair, which expects the color indices to be shorts.  If we are
using the init_pair fallback then using init_extended_color is
pointless.

In reality init_extended_pair and init_extended_color were both added
in ncurses 6.1, so should both be available together.

There is one additional change in here.  Assuming that a terminal does
support more than SHRT_MAX colours, but for some reason GDB is
compiled with a version of the curses library that doesn't support
init_extended_color, then it is possible that in `get_color` the value
of NEXT could end up above SHRT_MAX, in which case the `init_color`
call will truncate the value of NEXT to a short and we will end up
redefining an earlier color index.  To avoid this unlikely case I've
added a compare against SHRT_MAX.

The init_extended_color path doesn't have this risk as COLORS is an
`int` and NEXT is passed as an `int` on this path so there is no risk
of truncation.
---
 gdb/config.in    |  3 +++
 gdb/configure    |  1 +
 gdb/configure.ac |  1 +
 gdb/tui/tui-io.c | 21 +++++++++++++++++++--
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/gdb/config.in b/gdb/config.in
index 1ef5dc5c2f4..74745665d4d 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -250,6 +250,9 @@
 /* Define to 1 if you have the `iconvlist' function. */
 #undef HAVE_ICONVLIST
 
+/* Define to 1 if you have the `init_extended_color' function. */
+#undef HAVE_INIT_EXTENDED_COLOR
+
 /* Define to 1 if you have the `init_extended_pair' function. */
 #undef HAVE_INIT_EXTENDED_PAIR
 
diff --git a/gdb/configure b/gdb/configure
index 303d6ea011c..fe58ba47e73 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -30238,6 +30238,7 @@ for ac_func in  \
   getrlimit \
   getuid \
   iconvlist \
+  init_extended_color \
   init_extended_pair \
   libiconvlist \
   posix_madvise \
diff --git a/gdb/configure.ac b/gdb/configure.ac
index e55a733fba7..3d8847df9d8 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1520,6 +1520,7 @@ AC_CHECK_FUNCS([ \
   getrlimit \
   getuid \
   iconvlist \
+  init_extended_color \
   init_extended_pair \
   libiconvlist \
   posix_madvise \
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 896d00f44c8..b6afe08be3e 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -252,10 +252,27 @@ get_color (const ui_file_style::color &color, int *result)
 	  if (next >= COLORS)
 	    return false;
 	  rgb_color rgb = color.get_rgb ();
+
 	  /* We store RGB as 0..255, but curses wants 0..1000.  */
-	  if (init_color (next, rgb[0] * 1000 / 255, rgb[1] * 1000 / 255,
-			  rgb[2] * 1000 / 255) == ERR)
+	  short r = rgb[0] * 1000 / 255;
+	  short g = rgb[1] * 1000 / 255;
+	  short b = rgb[2] * 1000 / 255;
+
+	  /* If init_extended_pair is not available then we fallback to
+	     using init_pair.  However, init_pair can only handle 'short'
+	     color indices so there is no point using init_extended_color
+	     to allow for the generation of longer 'int' color indices.  */
+#if defined HAVE_INIT_EXTENDED_COLOR && defined HAVE_INIT_EXTENDED_PAIR
+	  if (init_extended_color (next, r, g, b) == ERR)
 	    return false;
+#else
+	  /* NEXT is an int, but is passed as a short.  If COLORS is
+	     more than SHRT_MAX then NEXT will be truncated and end up
+	     redefining a color entry that we don't expect.  */
+	  if (next > SHRT_MAX
+	      || init_color (next, r, g, b) == ERR)
+	    return false;
+#endif
 	  color_map[color] = next;
 	  *result = next;
 	}

base-commit: cd4e3876afb11e07fb7f3ccc10a87e3b990074eb
-- 
2.25.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2] gdb/tui: use init_extended_color where possible
  2026-08-05 11:50 ` [PATCHv2] " Andrew Burgess
@ 2026-08-12 19:26   ` Simon Marchi
  2026-08-12 19:44   ` Tom Tromey
  1 sibling, 0 replies; 5+ messages in thread
From: Simon Marchi @ 2026-08-12 19:26 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

On 8/5/26 7:50 AM, Andrew Burgess wrote:
> In v2:
> 
>   - Split the scaling of GDB's RGB value out from the init_color and
>     init_extended_color calls.  This removes some code duplication and
>     allows the comment to sit closer to the code in question.
> 
>   - Rebase to current HEAD and retest.
> 
> ---
> 
> After commit:
> 
>   commit fbe7f20a0f098ca03913452b29f50f0dc8568f77
>   Date:   Sat May 9 23:27:43 2026 +0200
> 
>     gdb/tui: fix unexpected reuse of color pairs
> 
> which converted GDB to use init_extended_pair where possible, I
> realised we could also make use of init_extended_color.
> 
> The motivation for using init_extended_color is slightly less than
> init_extended_pair.  Assuming the terminal supports it the standard
> init_color API supports up to SHRT_MAX (32767) different colors,
> switching to init_extended_color removes the SHRT_MAX limit on color
> indices, allowing us to support the full range of COLORS.
> 
> But the cost of making this change is minimal, we already track the
> color indices as an `int` within the global COLOR_MAP, so it's mostly
> just a case of calling init_extended_color where needed.
> 
> We only use init_extended_color when both that function and
> init_extended_pair is available.  The fallback to init_extended_pair
> is init_pair, which expects the color indices to be shorts.  If we are
> using the init_pair fallback then using init_extended_color is
> pointless.
> 
> In reality init_extended_pair and init_extended_color were both added
> in ncurses 6.1, so should both be available together.
> 
> There is one additional change in here.  Assuming that a terminal does
> support more than SHRT_MAX colours, but for some reason GDB is
> compiled with a version of the curses library that doesn't support
> init_extended_color, then it is possible that in `get_color` the value
> of NEXT could end up above SHRT_MAX, in which case the `init_color`
> call will truncate the value of NEXT to a short and we will end up
> redefining an earlier color index.  To avoid this unlikely case I've
> added a compare against SHRT_MAX.
> 
> The init_extended_color path doesn't have this risk as COLORS is an
> `int` and NEXT is passed as an `int` on this path so there is no risk
> of truncation.

Seems fine to me, thanks.  Hopefully one day we can just assume that the
extended functions are present.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2] gdb/tui: use init_extended_color where possible
  2026-08-05 11:50 ` [PATCHv2] " Andrew Burgess
  2026-08-12 19:26   ` Simon Marchi
@ 2026-08-12 19:44   ` Tom Tromey
  2026-08-13  9:53     ` Andrew Burgess
  1 sibling, 1 reply; 5+ messages in thread
From: Tom Tromey @ 2026-08-12 19:44 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> The motivation for using init_extended_color is slightly less than
Andrew> init_extended_pair.  Assuming the terminal supports it the standard
Andrew> init_color API supports up to SHRT_MAX (32767) different colors,
Andrew> switching to init_extended_color removes the SHRT_MAX limit on color
Andrew> indices, allowing us to support the full range of COLORS.

First, I think the patch is fine.
Approved-By: Tom Tromey <tom@tromey.com>

However I have a question

Andrew>  	  /* We store RGB as 0..255, but curses wants 0..1000.  */
Andrew> -	  if (init_color (next, rgb[0] * 1000 / 255, rgb[1] * 1000 / 255,
Andrew> -			  rgb[2] * 1000 / 255) == ERR)
Andrew> +	  short r = rgb[0] * 1000 / 255;
Andrew> +	  short g = rgb[1] * 1000 / 255;
Andrew> +	  short b = rgb[2] * 1000 / 255;
Andrew> +
Andrew> +	  /* If init_extended_pair is not available then we fallback to
Andrew> +	     using init_pair.  However, init_pair can only handle 'short'
Andrew> +	     color indices so there is no point using init_extended_color
Andrew> +	     to allow for the generation of longer 'int' color indices.  */
Andrew> +#if defined HAVE_INIT_EXTENDED_COLOR && defined HAVE_INIT_EXTENDED_PAIR
Andrew> +	  if (init_extended_color (next, r, g, b) == ERR)
Andrew>  	    return false;
Andrew> +#else
Andrew> +	  /* NEXT is an int, but is passed as a short.  If COLORS is
Andrew> +	     more than SHRT_MAX then NEXT will be truncated and end up
Andrew> +	     redefining a color entry that we don't expect.  */
Andrew> +	  if (next > SHRT_MAX
Andrew> +	      || init_color (next, r, g, b) == ERR)
Andrew> +	    return false;
Andrew> +#endif

IIUC init_extended_color allows a bigger range for 'next' but also for
the RGB components.  However despite the text above, I think we don't
actually use the bigger RGB range.  And, perhaps we don't really care
to, I don't know.

Tom

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2] gdb/tui: use init_extended_color where possible
  2026-08-12 19:44   ` Tom Tromey
@ 2026-08-13  9:53     ` Andrew Burgess
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Burgess @ 2026-08-13  9:53 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> The motivation for using init_extended_color is slightly less than
> Andrew> init_extended_pair.  Assuming the terminal supports it the standard
> Andrew> init_color API supports up to SHRT_MAX (32767) different colors,
> Andrew> switching to init_extended_color removes the SHRT_MAX limit on color
> Andrew> indices, allowing us to support the full range of COLORS.
>
> First, I think the patch is fine.
> Approved-By: Tom Tromey <tom@tromey.com>
>
> However I have a question
>
> Andrew>  	  /* We store RGB as 0..255, but curses wants 0..1000.  */
> Andrew> -	  if (init_color (next, rgb[0] * 1000 / 255, rgb[1] * 1000 / 255,
> Andrew> -			  rgb[2] * 1000 / 255) == ERR)
> Andrew> +	  short r = rgb[0] * 1000 / 255;
> Andrew> +	  short g = rgb[1] * 1000 / 255;
> Andrew> +	  short b = rgb[2] * 1000 / 255;
> Andrew> +
> Andrew> +	  /* If init_extended_pair is not available then we fallback to
> Andrew> +	     using init_pair.  However, init_pair can only handle 'short'
> Andrew> +	     color indices so there is no point using init_extended_color
> Andrew> +	     to allow for the generation of longer 'int' color indices.  */
> Andrew> +#if defined HAVE_INIT_EXTENDED_COLOR && defined HAVE_INIT_EXTENDED_PAIR
> Andrew> +	  if (init_extended_color (next, r, g, b) == ERR)
> Andrew>  	    return false;
> Andrew> +#else
> Andrew> +	  /* NEXT is an int, but is passed as a short.  If COLORS is
> Andrew> +	     more than SHRT_MAX then NEXT will be truncated and end up
> Andrew> +	     redefining a color entry that we don't expect.  */
> Andrew> +	  if (next > SHRT_MAX
> Andrew> +	      || init_color (next, r, g, b) == ERR)
> Andrew> +	    return false;
> Andrew> +#endif
>
> IIUC init_extended_color allows a bigger range for 'next' but also for
> the RGB components.  However despite the text above, I think we don't
> actually use the bigger RGB range.  And, perhaps we don't really care
> to, I don't know.

Great question!  I also wondered about this as I too noticed that
init_extended_color accepted r, g, b as `int`.

The ncurses docs for this are super unclear, at least on my machine.

For init_color I get an explicit paragraph which says:

  "Each of the last three arguments must be a value in the range 0 through 1000."

But for init_extended_color my man page says:

  "Because color_content uses signed shorts for its parameters, that
  limits color-values and their red, green, and blue components to 32767
  on modern hardware.  The extension extended_color_content uses ints
  for the color value and for returning the red, green, and blue
  components, allowing a larger number of colors to be supported."

which seems to suggest that r, g, b can have more range.

However, note that even for init_color, where r, g, b are `short` the
valid range is limited to 0 -> 1000, not the full short range as the
text for init_extended_color seems to suggest.

And the text for start_color, the general function to enable color
support, has some text that talks about the range of the rgb components,
and it too talks about 1000.

None of this is super convincing.  At least, none of it really convinced
me.  So in the end I just went to the sources.

Looking at ncurses-6.6 source, in the file base/lib_color.c we see that
both init_color and init_extended_color just forward their argument
unmodified to _nc_init_color, which uses `int` for all its arguments,
just like init_extended_color.

After some initial checks, none of which check the rgb values, the code
calls:

    if (InitColor
	&& sp->_coloron
	&& (color >= 0 && OkColorHi(color))
	&& (okRGB(r) && okRGB(g) && okRGB(b))) {
      /* This is where r, g, b are actually used.  */
    }

And elsewhere in the file we find:

  #define okRGB(n) ((n) >= 0 && (n) <= 1000)

Which for me is the definitive answer.  The r, g, b components are
always in the range 0 -> 1000 (inclusive).

What this all means is that for both init_color and init_extended_color
there are 1,000,000,000 different rgb color combinations that could be
created, but init_color will only allow you to use 32,767 of these at a
time, while init_extended_color will allow them all to be used.

Is this super useful?  Probably not.  But it doesn't cost much to
support it.

Thanks,
Andrew


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-08-13  9:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-21 14:24 [PATCH] gdb/tui: use init_extended_color where possible Andrew Burgess
2026-08-05 11:50 ` [PATCHv2] " Andrew Burgess
2026-08-12 19:26   ` Simon Marchi
2026-08-12 19:44   ` Tom Tromey
2026-08-13  9:53     ` Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox