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

Sergio Durigan Junior
2017-04-12T00:24:55Z

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