[PATCH v2] [gdb] Add c_ctrl/c_unctrl

[PATCH v2] [gdb] Add c_ctrl/c_unctrl

[Tom de Vries]

Readline exports macros CTRL/UNCTRL (compatible with readline macro
CTRL_CHAR), such that:
- CTRL_CHAR ('C') == 0
- CTRL_CHAR (0x03 /* ^C */) == 1
- CTRL ('C') == 0x03 /* ^C */
- CTRL ('c') == 0x03 /* ^C */
- UNCTRL (0x03 /* ^C */) == 'C'

Add c_ctrl/c_unctrl, a variant of CTRL/UNCTRL that's compatible with gnulib's
c_iscntrl.

While c_iscntrl (0x7f /* ^? */) == 1, CTRL_CHAR (0x7f /* ^? */) == 0.

Consequently, the current code using CTRL_CHAR also explicitly handles RUBOUT
(which is readline's way of representing ^?).

Use c_iscntrl/c_ctrl/c_unctrl instead of CTRL_CHAR/CTRL/UNCTRL, removing
redundant RUBOUT handling code.

Tested on x86_64-linux.

A v1 was submitted here [2].

Changes in v2:
- change $subject to use gdb instead of gdbsupport since we're adding the
  new functions in gdb/utils.c
- change parameter and result type of c_unctrl to unsigned char
- avoid using readline macros in c_unctrl implementation
- rewrite c_unctrl to be compatible with c_iscntrl, making sure to handle
  ^?.
- add c_ctrl
- use c_ctrl/c_iscntrl instead of CTRL/CTRL_CHAR.

Suggested-By: Tom Tromey <tom@tromey.com> [1]

[1] https://sourceware.org/pipermail/gdb-patches/2026-March/226136.html
[2] https://sourceware.org/pipermail/gdb-patches/2026-March/226171.html
---
 gdb/completer.c  | 16 ++--------
 gdb/tui/tui-io.c |  4 +--
 gdb/tui/tui.c    |  4 +--
 gdb/utils.c      | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/utils.h      |  8 +++++
 5 files changed, 95 insertions(+), 17 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 8c70a61cdec..49189ac183f 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -3044,7 +3044,7 @@ gdb_fnwidth (const char *string)
   width = pos = 0;
   while (string[pos])
     {
-      if (CTRL_CHAR (string[pos]) || string[pos] == RUBOUT)
+      if (c_iscntrl (string[pos]))
 	{
 	  width += 2;
 	  pos++;
@@ -3118,20 +3118,10 @@ gdb_fnprint (const char *to_print, int prefix_bytes,
   s = to_print + prefix_bytes;
   while (*s)
     {
-      if (CTRL_CHAR (*s))
+      if (c_iscntrl (*s))
 	{
 	  displayer->putch (displayer, '^');
-	  displayer->putch (displayer, UNCTRL (*s));
-	  printed_len += 2;
-	  s++;
-#if defined (HANDLE_MULTIBYTE)
-	  memset (&ps, 0, sizeof (mbstate_t));
-#endif
-	}
-      else if (*s == RUBOUT)
-	{
-	  displayer->putch (displayer, '^');
-	  displayer->putch (displayer, '?');
+	  displayer->putch (displayer, c_unctrl (*s));
 	  printed_len += 2;
 	  s++;
 #if defined (HANDLE_MULTIBYTE)
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 6b94fa2e8b0..e0892a40c0d 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -631,10 +631,10 @@ tui_redisplay_readline (void)
 	break;
 
       c = (unsigned char) rl_line_buffer[in];
-      if (CTRL_CHAR (c) || c == RUBOUT)
+      if (c_iscntrl (c))
 	{
 	  waddch (w, '^');
-	  waddch (w, CTRL_CHAR (c) ? UNCTRL (c) : '?');
+	  waddch (w, c_unctrl (c));
 	}
       else if (c == '\t')
 	{
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index b6b6fb7acf5..9cf21f390c8 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -364,8 +364,8 @@ tui_ensure_readline_initialized ()
   rl_bind_key_in_map ('a', tui_rl_switch_mode, tui_ctlx_keymap);
   rl_bind_key_in_map ('A', tui_rl_switch_mode, emacs_ctlx_keymap);
   rl_bind_key_in_map ('A', tui_rl_switch_mode, tui_ctlx_keymap);
-  rl_bind_key_in_map (CTRL ('A'), tui_rl_switch_mode, emacs_ctlx_keymap);
-  rl_bind_key_in_map (CTRL ('A'), tui_rl_switch_mode, tui_ctlx_keymap);
+  rl_bind_key_in_map (c_ctrl ('A'), tui_rl_switch_mode, emacs_ctlx_keymap);
+  rl_bind_key_in_map (c_ctrl ('A'), tui_rl_switch_mode, tui_ctlx_keymap);
   rl_bind_key_in_map ('1', tui_rl_delete_other_windows, emacs_ctlx_keymap);
   rl_bind_key_in_map ('1', tui_rl_delete_other_windows, tui_ctlx_keymap);
   rl_bind_key_in_map ('2', tui_rl_change_windows, emacs_ctlx_keymap);
diff --git a/gdb/utils.c b/gdb/utils.c
index 6908256de4d..b073bb9fdf3 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3733,6 +3733,85 @@ extract_single_filename_arg (const char *args)
   return filename;
 }
 
+/* See utils.h.  */
+
+unsigned char
+c_unctrl (unsigned char c)
+{
+  if (!c_iscntrl (c))
+    return c;
+
+  unsigned char res = c;
+  if (res >= 0x40)
+    {
+      /* Map 0x7f (^?) to 0x3f (?).  */
+      res -= 0x40;
+    }
+  else
+    {
+      /* Map 0x03 (^C) to 0x43 (C).  */
+      res += 0x40;
+    }
+
+  return res;
+}
+
+/* See utils.h.  */
+
+unsigned char
+c_ctrl (unsigned char c)
+{
+  unsigned char res = c;
+  if (res < 0x40)
+    {
+      /* Map 0x3f (?) to 0x7f (^?).  */
+      res += 0x40;
+    }
+  else
+    {
+      res = c_toupper (res);
+
+      /* Map 0x43 (C) to 0x03 (^C).  */
+      res -= 0x40;
+    }
+
+  return c_iscntrl (res) ? res : c;
+}
+
+#if GDB_SELF_TEST
+static void
+test_c_ctrl_unctrl ()
+{
+  /* Basic check.  */
+  SELF_CHECK (c_ctrl ('C') == 0x03);
+  SELF_CHECK (c_ctrl ('c') == 0x03);
+  SELF_CHECK (c_unctrl (0x03) == 'C');
+
+  /* Function c_iscntrl considers ^? to be a control character, but for some
+     reason, readline's CTRL_CHAR doesn't, so CTRL/UNCTRL don't handle it.
+     Check that c_ctrl/c_unctrl do handle it.  */
+  SELF_CHECK (c_ctrl ('?') == 0x7f);
+  SELF_CHECK (c_unctrl (0x7f) == '?');
+
+  /* Consistency check.  */
+  for (unsigned int i = 0; i < 0x100; i++)
+    {
+      unsigned char ch = i;
+      unsigned char unctrl_ch = c_unctrl (ch);
+      if (!c_iscntrl (ch))
+	{
+	  SELF_CHECK (unctrl_ch == ch);
+	  continue;
+	}
+
+      SELF_CHECK (!c_iscntrl (unctrl_ch));
+      SELF_CHECK (!c_islower (unctrl_ch));
+      SELF_CHECK (c_ctrl (unctrl_ch) == ch);
+      SELF_CHECK (c_ctrl (c_tolower (unctrl_ch)) == ch);
+    }
+}
+#endif
+
 #if GDB_SELF_TEST
 static void
 test_assign_set_return_if_changed ()
@@ -3834,5 +3913,6 @@ When set, debugging messages will be marked with seconds and microseconds."),
   selftests::register_test ("pager", test_pager);
   selftests::register_test ("assign_set_return_if_changed",
 			    test_assign_set_return_if_changed);
+  selftests::register_test ("c_ctrl_unctrl", test_c_ctrl_unctrl);
 #endif
 }
diff --git a/gdb/utils.h b/gdb/utils.h
index 7cab021e7a9..ecef7d13c20 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -515,4 +515,12 @@ struct deferred_warnings final : public warning_hook_handler_type
   std::vector<string_file> m_warnings;
 };
 
+/* Re-implementation of readline's CTRL/UNCTRL (compatible with readline's
+   CTRL_CHAR), designed to be compatible with gnulib's c_iscntrl.
+   While gnulib's c-ctype.h still uses int for character parameters and
+   results, we use unsigned char here for maximum clarity.  */
+
+extern unsigned char c_ctrl (unsigned char c);
+extern unsigned char c_unctrl (unsigned char c);
+
 #endif /* GDB_UTILS_H */

base-commit: 4054f280399225fc40ac42423246ac9c5de6f857
-- 
2.51.0

Re: [PATCH v2] [gdb] Add c_ctrl/c_unctrl

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Use c_iscntrl/c_ctrl/c_unctrl instead of CTRL_CHAR/CTRL/UNCTRL, removing
Tom> redundant RUBOUT handling code.

This looks good to me but I had a small comment on the self-test.

Tom> +  /* Consistency check.  */
Tom> +  for (unsigned int i = 0; i < 0x100; i++)
Tom> +    {
Tom> +      unsigned char ch = i;
Tom> +      unsigned char unctrl_ch = c_unctrl (ch);

The main problem with the <ctype.h> functions is that they accept an int
but are undefined for values outside of 'unsigned char'.  So calling
them with an ordinary 'char' value is bad, because on some platforms
char might be signed and so sign extension will apply.

So I think it would be valuable to test this.  A simple way might be to
change this loop to iterate from -128 .. 255 and just pass that value.
This would effectively emulate both signed and unsigned char I think.

Tom

Re: [PATCH v2] [gdb] Add c_ctrl/c_unctrl

[Tom de Vries]

On 3/25/26 6:40 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Use c_iscntrl/c_ctrl/c_unctrl instead of CTRL_CHAR/CTRL/UNCTRL, removing
> Tom> redundant RUBOUT handling code.
> 
> This looks good to me but I had a small comment on the self-test.
> 
> Tom> +  /* Consistency check.  */
> Tom> +  for (unsigned int i = 0; i < 0x100; i++)
> Tom> +    {
> Tom> +      unsigned char ch = i;
> Tom> +      unsigned char unctrl_ch = c_unctrl (ch);
> 
> The main problem with the <ctype.h> functions is that they accept an int
> but are undefined for values outside of 'unsigned char'.  So calling
> them with an ordinary 'char' value is bad, because on some platforms
> char might be signed and so sign extension will apply.
> 

OK, I understand that.

> So I think it would be valuable to test this.  A simple way might be to
> change this loop to iterate from -128 .. 255 and just pass that value.
> This would effectively emulate both signed and unsigned char I think.

I'm not sure I understand that.

I intentionally made c_ctrl and c_unctrl accept and produce unsigned 
char to avoid this problem.

AFAIU, I did the change you propose:
...
diff --git a/gdb/utils.c b/gdb/utils.c
index b073bb9fdf3..b9d7163b1b5 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3794,9 +3794,8 @@ test_c_ctrl_unctrl ()
    SELF_CHECK (c_unctrl (0x7f) == '?');

    /* Consistency check.  */
-  for (unsigned int i = 0; i < 0x100; i++)
+  for (int ch = -128; ch < 256; ch++)
      {
-      unsigned char ch = i;
        unsigned char unctrl_ch = c_unctrl (ch);
        if (!c_iscntrl (ch))
  	{
...
and then the selftest fails here:
...
(gdb)
#3  0x00000000010e93b9 in test_c_ctrl_unctrl ()
     at /data/vries/gdb/leap-16-0/build/../../src/gdb/utils.c:3802
3802		  SELF_CHECK (unctrl_ch == ch);
(gdb) p ch
$1 = -128
(gdb) p unctrl_ch
$3 = 128 '\200'
(gdb)
...

I could make this pass somehow, but I don't know how because I don't 
understand what we're actually trying to test here.

That is, we're already testing all input values.  What else is there to 
test?

Thanks,
- Tom

Re: [PATCH v2] [gdb] Add c_ctrl/c_unctrl

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I could make this pass somehow, but I don't know how because I don't
Tom> understand what we're actually trying to test here.

Tom> That is, we're already testing all input values.  What else is there
Tom> to test?

The point is that the function should be a black box and that sign
extension of chars (on some platforms at least) is the case that
required this.

But let's just go ahead.  I don't really care that much.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH v2] [gdb] Add c_ctrl/c_unctrl

[Tom Tromey]

Tom> I could make this pass somehow, but I don't know how because I don't
Tom> understand what we're actually trying to test here.

Tom> That is, we're already testing all input values.  What else is there
Tom> to test?

> The point is that the function should be a black box and that sign
> extension of chars (on some platforms at least) is the case that
> required this.

> But let's just go ahead.  I don't really care that much.

Sorry about this.  This didn't really come out right.

What I mean is that the benefit of black-box testing is probably
minimal.  So I think there's not much point to doing what I suggested.

Tom

Re: [PATCH v2] [gdb] Add c_ctrl/c_unctrl

[Tom de Vries]

On 3/25/26 9:50 PM, Tom Tromey wrote:
> Tom> I could make this pass somehow, but I don't know how because I don't
> Tom> understand what we're actually trying to test here.
> 
> Tom> That is, we're already testing all input values.  What else is there
> Tom> to test?
> 
>> The point is that the function should be a black box and that sign
>> extension of chars (on some platforms at least) is the case that
>> required this.
> 
>> But let's just go ahead.  I don't really care that much.
> 
> Sorry about this.  This didn't really come out right.
> 

Hi Tom,

no worries, I understood it as a technical comment.

> What I mean is that the benefit of black-box testing is probably
> minimal.  So I think there's not much point to doing what I suggested.

I've pushed the patch, but I may submit a follow-up patch implementing 
(my current understanding of) your suggestion.

Thanks for the review.

- Tom

Re: [PATCH v2] [gdb] Add c_ctrl/c_unctrl

[Tom de Vries]

On 3/26/26 10:57 AM, Tom de Vries wrote:
> 
> I've pushed the patch, but I may submit a follow-up patch implementing 
> (my current understanding of) your suggestion.

I've posted a follow-up patch ( 
https://sourceware.org/pipermail/gdb-patches/2026-March/226238.html ).

Thanks,
- Tom