This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]