Re: [PATCH v5 3/5] C++-fy and prepare for sharing fork_inferior

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, April 07 2017, Pedro Alves wrote:

> On 03/30/2017 02:49 AM, Sergio Durigan Junior wrote:
>> As a preparation for the next patch, which will move fork_inferior
>> from GDB to common/ (and therefore share it with gdbserver), it is
>> interesting to convert a few functions to C++.
>
> I've meanwhile realized that fork_inferior should move to "nat/",
> not "common/" ...  This is code only used by the native targets.
> Sorry for not pointing it out sooner...  :-/.

Alright.  I'll address that in the next version, thanks for the
heads-up.  I won't rename the file in this case, leaving it as
"nat/fork-child.c".

>> This patch touches functions related to parsing command-line arguments
>> to the inferior (see gdb/fork-child.c:breakup_args), the way the
>> arguments are stored on fork_inferior (using std::vector instead of
>> char **), and the code responsible for dealing with argv also on
>> gdbserver.
>> 
>> I've taken this opportunity and decided to constify a few arguments to
>> fork_inferior/create_inferior as well, in order to make the code
>> cleaner.  And now, on gdbserver, we're using xstrdup everywhere and
>> aren't checking for memory allocation failures anymore, as requested
>> by Pedro.
>
> It'd be good to say here _why_ that's the right thing to do.

Sure, I'll copy-and-adjust your rationale from the last message.

> I.e., write to the future hacker doing archaeology.
>
>> --- a/gdb/fork-child.c
>> +++ b/gdb/fork-child.c
>> @@ -1,4 +1,4 @@
>> -/* Fork a Unix child process, and set up to debug it, for GDB.
>> + /* Fork a Unix child process, and set up to debug it, for GDB.
>
> Spurious whitespace change.

Fixed.

>>  static void
>> -breakup_args (char *scratch, char **argv)
>> +breakup_args (const std::string &scratch, std::vector<char *> &argv)
>>  {
>
> ...
>
>> +
>> +      std::string arg = scratch.substr (cur_pos, next_sep - cur_pos);
>> +
>
> This creates a temporary string (heap allocates) ...
>
>> +      argv.push_back (xstrdup (arg.c_str ()));
>
> ... and here you create yet another copy.
>
> You should be able to avoid it by using e.g., savestring:
>
>     char *arg = savestring (scratch.c_str () + cur_pos, next_sep - cur_pos);
>     argv.push_back (arg);

Fair enough.  I had my mind on "C++-only mode" when writing this code.

>> +
>> +      cur_pos = next_sep;
>>      }
>>  
>> -  /* Null-terminate the vector.  */
>> -  *argv = NULL;
>> +  /* NULL-terminating the vector.  */
>
> FYI, the non-gerund version of the text reads as more
> natural English to me.

Reverted.

>> +  argv.push_back (NULL);
>>  }
>>  
>
>
>> --- a/gdb/go32-nat.c
>> +++ b/gdb/go32-nat.c
>> @@ -631,8 +631,9 @@ go32_kill_inferior (struct target_ops *ops)
>>  }
>>  
>>  static void
>> -go32_create_inferior (struct target_ops *ops, char *exec_file,
>> -		      char *args, char **env, int from_tty)
>> +go32_create_inferior (struct target_ops *ops,
>> +		      const char *exec_file,
>> +		      const std::string &allargs, char **env, int from_tty)
>>  {
>>    extern char **environ;
>>    jmp_buf start_state;
>> @@ -641,6 +642,7 @@ go32_create_inferior (struct target_ops *ops, char *exec_file,
>>    size_t cmdlen;
>>    struct inferior *inf;
>>    int result;
>> +  char *args = (char *) allargs.c_str ();
>
> AFAICS, this could be const.

Right, fixed.

> Note that when you really need to append a single character,
> it's a tiny bit more efficient to write what you mean:
>
>         shell_command += ' ';
>
> instead of appending a string that happens to have one character:
>
> +         shell_command += " ";
> +         shell_command += "'";
>
> because the later means you're calling the operator+= overload that
> needs to handle / count the string length.
>
>
> I saw a few of those in the patch.

Hm, indeed.  Fixed.

> Anyway, with those addressed and with any missing xstrdup
> that -Wwrite-string may have flagged added, this is good to
> go.
>
> I believe this stands on its own and doesn't have any dependency
> on the previous patches, so please go ahead and push.

OK, thanks.  I'll work on patch 1/5 and get that out of the way first,
and the I'll push this in.

Thanks!

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/