Re: [PATCH] gdb: consider null terminator for length arguments of value_cstring calls

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-07-15 10:29 a.m., Pedro Alves wrote:> On 2021-07-14 2:50 p.m., Simon Marchi wrote:
>>> For C, it makes sence to me to return a null terminated string.  Though I wonder
>>> whether the string you get should depend on language?  I mean, see this:
>>>
>>>  (gdb) set language c
>>>  (gdb) ptype "foo"
>>>  type = char [4]
>>>  (gdb) set language ada
>>>  (gdb) ptype "foo"
>>>  type = array (1 .. 3) of character
>>>
>>> Whatever we decide, I think we should update the manual to make it explicit.
>>
>> Agreed, I think the result of:
>>
>>   (gdb) ptype "foo"
>>
>> should be the same as
>>
>>   (gdb) set args foo
>>   (gdb) ptype $_gdb_setting("args")
>>
>> for every language.
>>
>> For Ada though, I get:
> 
> ...
> 
>> Let's pretend I didn't see this and try with a non-Asan GDB:
>>
>>   $ ./gdb -q -nx --data-directory=data-directory -ex "set language ada" -ex "set args foo" -ex 'ptype $_gdb_setting("args")' -ex 'print $_gdb_setting("args")' -batch
>>   type = <4-byte integer>
>>   $1 = (102, 111, 111)
>>
>>   $ ./gdb -q -nx --data-directory=data-directory -ex "set language ada" -ex "set args foo" -ex 'ptype $_gdb_setting("args")' -ex 'print $_gdb_setting("args")' -batch
>>   type = <4-byte integer>
>>   $1 = (102, 111, 111, 0)
>>
>> Not sure what the 4-byte integer refers to.  But in any case, the result
>> doesn't look good.
> 
> That "4-byte integer" thing is a red herring and a known issue -- it also happens in C:
> 
> $ $g -q -nx  -ex "set language c" -ex "set args foo" -ex 'ptype $_gdb_setting("args")' -ex 'print $_gdb_setting("args")' -batch
> type = int
> $1 = "foo"
> 
> See settings.exp:
> 
> # Verifies that $_gdb_setting (SETTING) gives a value whose ptype matches EXPECTED.
> proc check_type {setting expected} {
>     with_test_prefix "check_type $setting $expected" {
>         gdb_test "print \$_gdb_maint_setting(\"$setting\")"
>         gdb_test "ptype $" "$expected"
> 
>         # Currently, GDB ptype <internal function> always tells it is type int.
>         # ptype should better report an error such as:
>         #   "No type information for GDB functions"
>         # Test 'type int', so as to make it fail if ptype is changed.
>         gdb_test "ptype \$_gdb_maint_setting(\"$setting\")" \
>             "type = int"
>     }
> }
> 
> So instead, try (current master):
> 
> $ ./gdb -q -nx --data-directory=data-directory -ex "set language c" -ex "set args foo" -ex 'print $_gdb_setting("args")' -ex 'ptype $' -batch
> $1 = "foo"
> type = char [3]
> 
>>
>> The created type (using value_cstring vs value_string) should probably
>> depend on the language.  I tried swapping value_cstring for
>> value_string, but that wasn't enough.  So, it seems there's work to do,
>> but I don't think I'm the right person to do it as I don't know much
>> about Ada (and don't have much time to dig into it).
> 
> I think what we need is a language hook to return a string struct value from
> a const char * / len.  For C, the implementation would use value_cstring,
> for Ada it would use value_string.  The testcase can cycle through
> all languages and make sure that "print/ptype "foo" and 
> print/ptype \$_gdb_maint_setting return the same thing.
> 
> I gave this a try, see patch below, on top of your v2.  I've also put it in the 
> users/palves/value_string branch on sourceware. 
> 
> Note I went the other direction and made value_cstring work with characters instead of
> bytes, and made it add the \0 itself.  Thus I undid the guile change to use nullptr
> instead of &len, as well as the strlen()+1 changes.
> 
> After this, the only value_cstring calls left are in the C parser, and in the
> language method:
> 
>  $ grep value_cstring -rn
>  value.h:792:extern struct value *value_cstring (const char *ptr, ssize_t len,
>  c-lang.c:680:   result = value_cstring ((const char *) obstack_base (&output),
>  valops.c:1753:value_cstring (const char *ptr, ssize_t len, struct type *char_type)
>  language.c:985:  return value_cstring (ptr, len, type);
> 
> Note how the the current python and guile code were already depending on
> the language, but in an odd way.  E.g., in Python:
> 
> -#define builtin_type_pychar \
> -  language_string_char_type (python_language, python_gdbarch)
> 
> -           value = value_cstring (s.get (), strlen (s.get ()) + 1,
> -                                  builtin_type_pychar);
> 
> 
> I don't know what that was supposed to do when the language was not C?
> 
> The test runs into a few pre-existing oddities:
> 
>  (gdb) set language modula-2 
>  (gdb) p "foo"
>  strings are not implemented
> 
>  (gdb) p $_gdb_maint_setting("test-settings string")
>  ',' or ')' expected
> 
>  (gdb) set language unknown 
>  (gdb) p "foo"
>  Aborted (core dumped)
> 
> I have no idea how rust one is just weird, but then again if parsing worked, I don't
> think the call would work anyway, since Rust strings are a {ptr,len}
> aggregate, not a plain char array.
> 
> I filed PR gdb/28093 for the crash.

Your patch is much more complete than mine and does more the right
thing, so I don't think it's worth merging my patch (or if there are
some useful bits, they can be integrated in your patch).

Simon