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

[Tom de Vries <tdevries@suse.de>]

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