Re: [PATCH v2 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

Michael Weghorn
2020-05-12T15:57:59Z

On 07/05/2020 21.49, Simon Marchi wrote:
> Hi Michael,
>
> I think the patch looks good, there are just a few nits to fix, and a suggestion.
>
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 8f7482347c..3ac64b3164 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -151,12 +151,9 @@ get_inferior_args (void)
>> {
>> if (current_inferior ()->argc != 0)
>> {
>> - char *n;
>> -
>> - n = construct_inferior_arguments (current_inferior ()->argc,
>> + std::string n = construct_inferior_arguments (current_inferior ()->argc,
>> current_inferior ()->argv);
>
> Align the last line with the parenthesis, like it was before the change.

Done in the new version:
https://sourceware.org/pipermail/gdb-patches/2020-May/168347.html

>
>> - set_inferior_args (n);
>> - xfree (n);
>> + set_inferior_args (n.c_str());
>
> Space before parenthesis.
>

Also done.

>> }
>>
>> if (current_inferior ()->args == NULL)
>> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
>> index 71b9a11e02..3f117d5ef0 100644
>> --- a/gdbsupport/common-inferior.cc
>> +++ b/gdbsupport/common-inferior.cc
>> @@ -28,15 +28,15 @@ bool startup_with_shell = true;
>> /* Compute command-line string given argument vector. This does the
>> same shell processing as fork_inferior. */
>>
>> -char *
>> -construct_inferior_arguments (int argc, char **argv)
>> +std::string
>> +construct_inferior_arguments (int argc, char * const *argv)
>> {
>> - char *result;
>> + gdb_assert (argc >= 0);
>> + if (argc == 0 || argv[0] == NULL) {
>> + return "";
>> + }
>
> Omit the braces when there is just one single line statement.
>
> When there are braces, places them like this:
>
> if (condition)
> {
> stmt1;
> stmt2;
> }
>
> Did you actually encounter the case (argc > 0 && argv[0] == NULL)? In
> other words, does it really constitute a valid input to this function?
>

The gdbserver code used to add an extra NULL arg, so that case would
have been hit by just replacing the existing 'stringify_argv' calls by
'construct_inferior_args' (as done in the follow-up patch [1]) and
starting a program without args (like 'gdbserver localhost:50505
./main'). I had only added the patch to actually remove that extra arg
later [2]. With that patch in place that should actually not have
happened any more, so was indeed unnecessary.

In any case, I have removed that in the next version along with the
switch to using a std::string for the result right away as suggested.

[1] https://sourceware.org/pipermail/gdb-patches/2020-April/168038.html
[2] https://sourceware.org/pipermail/gdb-patches/2020-April/168040.html

>>
>> - /* ARGC should always be at least 1, but we double check this
>> - here. This is also needed to silence -Werror-stringop
>> - warnings. */
>> - gdb_assert (argc > 0);
>> + char *result;
>>
>> if (startup_with_shell)
>> {
>> @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
>> }
>> }
>>
>> - return result;
>> + std::string str_result(result);
>
> Space before parenthesis.

Done.

>
> If we are going to return a string anyway, I'd vote for building the result
> into an std::string directly. You can remove the part that tries to guess
> the length of the string, and use the appropriate std::string methods /
> operators to append to it.
>
> If you do this, I'm thinking that the `argc == 0` check at the beginning of
> the function might be unnecessary, as the code could handle it naturally:
> all the loops will be skipped and we'll return an empty string.

True, thanks for the hint. I have adapted this accordingly.

Michael