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

Pedro Alves
2017-04-07T18:30:23Z

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

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

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.

> 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);

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

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

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.

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.

Thanks,
Pedro Alves