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

[Michael Weghorn <m.weghorn@posteo.de>]

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