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

[Sergio Durigan Junior]

On Tuesday, April 11 2017, I wrote:

> 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.

Oh, well.  I went ahead and pushed it now.  No reason to wait.

Here's what I pushed.

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/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-C-fy-and-prepare-for-sharing-fork_inferior.patch
Type: text/x-patch
Size: 34521 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20170412/d4ef3ffe/attachment.bin>