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

Tom de Vries
2026-03-25T18:12:57Z

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