This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v5 3/5] C++-fy and prepare for sharing fork_inferior
- From: Pedro Alves <palves at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Date: Fri, 7 Apr 2017 19:30:14 +0100
- Subject: Re: [PATCH v5 3/5] C++-fy and prepare for sharing fork_inferior
- Authentication-results: sourceware.org; auth=none
- References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170330014915.4894-1-sergiodj@redhat.com> <20170330014915.4894-4-sergiodj@redhat.com>
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