Re: [PATCH v3 5/6] Share fork_inferior et al with gdbserver

[Pedro Alves <palves@redhat.com>]

Hi Sergio,

I think I'll likely have more comments, but this large, so
here's a first pass.

> diff --git a/gdb/common/common-fork-child.c b/gdb/common/common-fork-child.c
> new file mode 100644
> index 0000000..f4d7866
> --- /dev/null
> +++ b/gdb/common/common-fork-child.c
> @@ -0,0 +1,593 @@
> +/* Fork a Unix child process, and set up to debug it, for GDB and GDBserver.
> +
> +   Copyright (C) 1990-2017 Free Software Foundation, Inc.
> +
> +   Originally contributed by Cygnus Support.

We should just take the opportunity to drop that per recent policy.
Plenty of files added by Cygnus (nowadays Red Hat) don't have that marker,
this won't be missed, IMO.

> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +

> +int
> +fork_inferior (char *exec_file_arg, char *allargs, char **env,
> +	       void (*traceme_fun) (void), void (*init_trace_fun) (int),
> +	       void (*pre_trace_fun) (void), char *shell_file_arg,
> +	       void (*exec_fun)(const char *file, char * const *argv,
> +				char * const *env))
> +{

> +  /* Retain a copy of our environment variables, since the child will
> +     replace the value of environ and if we're vforked, we have to
> +     restore it.  */
> +  save_our_env = environ;
> +
> +  /* Likewise the current UI.  */
> +  save_ui = current_ui;

Hmm, making this compile on gdbserver is of course a hack,
since there's not such thing as a "UI" concept on gdbserver...

> +
> +  /* Tell the terminal handling subsystem what tty we plan to run on;
> +     it will just record the information for later.  */
> +  new_tty_prefork (inferior_io_terminal);

I wonder about calling here instead a more generalized hook, and
moving the save_ui saving and the new_tty_prefork calls to
that hook's gdb implementation.  Might be easier to do that after
the series is in...

> +
> +  /* It is generally good practice to flush any possible pending stdio
> +     output prior to doing a fork, to avoid the possibility of both
> +     the parent and child flushing the same data after the fork.  */
> +  gdb_flush_out_err ();
> +
> +  /* If there's any initialization of the target layers that must
> +     happen to prepare to handle the child we're about fork, do it
> +     now...  */
> +  if (pre_trace_fun != NULL)
> +    (*pre_trace_fun) ();
> +


> +++ b/gdb/common/common-top.h
> @@ -0,0 +1,31 @@
> +/* Common top level stuff for GDB and GDBserver.
> +
> +   Copyright (C) 1986-2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef COMMON_TOP_H
> +#define COMMON_TOP_H
> +
> +/* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
> +   It always exists and is created automatically when GDB starts
> +   up.  */
> +extern struct ui *main_ui;
> +
> +/* The current UI.  */
> +extern struct ui *current_ui;
> +

Not exactly thrilled with moving this to common.

> +#endif /* ! COMMON_TOP_H */

>  /* Implement the "unset exec-wrapper" command.  */
> @@ -585,7 +73,7 @@ Show the wrapper for running programs."), NULL,
>             &unsetlist);
>  
>    add_setshow_boolean_cmd ("startup-with-shell", class_support,
> -			   &startup_with_shell, _("\
> +			   (int *) &startup_with_shell, _("\

That's invalid C/C++.  Don't do that.

>  Set use of shell to start subprocesses.  The default is on."), _("\
>  Show use of shell to start subprocesses."), NULL,
>  			   NULL,


> --- a/gdb/gdbserver/inferiors.c
> +++ b/gdb/gdbserver/inferiors.c
> @@ -29,6 +29,8 @@ struct thread_info *current_thread;
>  
>  #define get_thread(inf) ((struct thread_info *)(inf))
>  
> +ptid_t inferior_ptid;

What do we need this for?  gdbserver already has
a "current thread" global.  Another one looks like asking
for out-of-sync trouble.

>  /* See common/common-gdbthread.h.  */
>  
>  void
>  init_thread_list (void)
>  {
> -  /* To be implemented.  */
> +  /* Placeholder needed for fork_inferior.  No action is needed.  */

Pick one of the comments, and move it to the right patch.

>  }
>  

> -/* Add a process to the common process list, and set its private
> -   data.  */
> +/* Update process represented by PID with necessary info.  */
>  
>  static struct process_info *
> -linux_add_process (int pid, int attached)
> +linux_update_process (int pid)

I'm not sure I understand the need for this yet.  I need
to look deeper.  "update what?  why?"  Or maybe the
comments should be improved.  :-)


>  {
> -  struct process_info *proc;
> +  struct process_info *proc = find_process_pid (pid);
>  
> -  proc = add_process (pid, attached);
> +  gdb_assert (proc != NULL);
>    proc->priv = XCNEW (struct process_info_private);
>  
>    if (the_low_target.new_process != NULL)
> @@ -432,6 +433,16 @@ linux_add_process (int pid, int attached)
>    return proc;
>  }
>  

> +/* Update the lwp associated to thread represented by PTID.  */
> +
> +static struct lwp_info *
> +update_thread_lwp (ptid_t ptid)

Ditto.

>  /* Start an inferior process and returns its pid.
>     ALLARGS is a vector of program-name and args. */
>  
>  static int
> -linux_create_inferior (char *program, char **allargs)
> +linux_create_inferior (std::vector<char *> &program_argv)

"const std::vector<char *> &" ?  Same comment for the whole
call chain, of course.

>  {
>    struct lwp_info *new_lwp;
>    int pid;
>    ptid_t ptid;
>    struct cleanup *restore_personality
>      = maybe_disable_address_space_randomization (disable_randomization);
> +  std::string program_args = stringify_argv (program_argv);
>  
> -#if defined(__UCLIBC__) && defined(HAS_NOMMU)
> -  pid = vfork ();
> -#else
> -  pid = fork ();
> -#endif
> -  if (pid < 0)
> -    perror_with_name ("fork");
> +  pre_fork_inferior (program_argv);
>  
> -  if (pid == 0)
> -    {
> -      close_most_fds ();
> -      ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0);
> -
> -      setpgid (0, 0);
> -
> -      /* If gdbserver is connected to gdb via stdio, redirect the inferior's
> -	 stdout to stderr so that inferior i/o doesn't corrupt the connection.
> -	 Also, redirect stdin to /dev/null.  */
> -      if (remote_connection_is_stdio ())
> -	{
> -	  close (0);
> -	  open ("/dev/null", O_RDONLY);
> -	  dup2 (2, 1);
> -	  if (write (2, "stdin/stdout redirected\n",
> -		     sizeof ("stdin/stdout redirected\n") - 1) < 0)
> -	    {
> -	      /* Errors ignored.  */;
> -	    }
> -	}
> -
> -      restore_original_signals_state ();
> -
> -      execv (program, allargs);
> -      if (errno == ENOENT)
> -	execvp (program, allargs);
> -
> -      fprintf (stderr, "Cannot exec %s: %s.\n", program,
> -	       strerror (errno));
> -      fflush (stderr);
> -      _exit (0177);
> -    }
> +  pid = fork_inferior (program_argv[0],
> +		       (char *) program_args.c_str (),

Can we constify fork_inferior's parameters to avoid such
cast hacks ?

> +		       environ_vector (get_environ ()), linux_ptrace_fun,
> +		       NULL, NULL, NULL, NULL);
>  
>    do_cleanups (restore_personality);
>  
> -  linux_add_process (pid, 0);
> +  linux_update_process (pid);
>  
>    ptid = ptid_build (pid, pid, 0);
> -  new_lwp = add_lwp (ptid);
> +  new_lwp = update_thread_lwp (ptid);
>    new_lwp->must_set_ptrace_flags = 1;
>  
> +  post_fork_inferior (pid, program_argv);
> +
>    return pid;
>  }
>  

> +/* See common/common-inferior.h.  */
> +
> +char *
> +get_exec_wrapper (void)
>  {
> -  char **new_argv = argv;
> +  static std::string ret;
> +  static int initialized_p = 0;

bool.

> +
> +  if (wrapper_argv.empty ())
> +    return NULL;
>  
> -  if (wrapper_argv != NULL)
> +  if (!initialized_p)
>      {
> -      int i, count = 1;
> -
> -      for (i = 0; wrapper_argv[i] != NULL; i++)
> -	count++;
> -      for (i = 0; argv[i] != NULL; i++)
> -	count++;
> -      new_argv = XALLOCAVEC (char *, count);
> -      count = 0;
> -      for (i = 0; wrapper_argv[i] != NULL; i++)
> -	new_argv[count++] = wrapper_argv[i];
> -      for (i = 0; argv[i] != NULL; i++)
> -	new_argv[count++] = argv[i];
> -      new_argv[count] = NULL;
> +      for (std::vector<char *>::iterator i = wrapper_argv.begin ();
> +	   i != wrapper_argv.end ();
> +	   ++i)
> +	ret += *i + std::string (" ");

      for (auto arg : wrapper_argv)
	ret += arg + " ";

> +
> +      /* Erasing the last whitespace.  */

"Erase".

> +      ret.erase (ret.end () - 1);
> +
> +      initialized_p = 1;
>      }
>  
> +  return (char *) ret.c_str ();

Can the function return const instead?

> +}
> +
> +/* See common/common-inferior.h.  */
> +
> +char *
> +get_exec_file (int err)
> +{
> +  if (err && program_argv.empty ())
> +    error (_("Could not get the exec file."));

I think this error message could be improved.  If I see
that on the gdbserver terminal, I'll have no clue what
it means.  "get from where??"  How about just saying
the same thing gdb says, sans the "Use the ..." part?

> +  return program_argv[0];
> +}
> +
> +/* See server.h.  */
> +
> +struct gdb_environ *
> +get_environ (void)
> +{
> +  return our_environ;
> +}
> +
> +/* See server.h.  */
> +
> +void
> +pre_fork_inferior (std::vector<char *> &argv)
> +{
>    if (debug_threads)
>      {
> -      int i;
> -      for (i = 0; new_argv[i]; ++i)
> -	debug_printf ("new_argv[%d] = \"%s\"\n", i, new_argv[i]);
> +      int idx = 0;
> +
> +      for (char *&i : argv)

Reference to pointer is pointless, it's like pointer to
pointer.  This should be fine:

      for (char *i : argv)

I'd suggest naming the variable something else, since
it's not really an iterator.  Like e.g.,:

      for (char *str : argv)
      for (char *arg : argv)

> +	{
> +	  debug_printf ("new_argv[%d] = \"%s\"\n", idx, i);
> +	  ++idx;

OTOH, if you're needing/maintaining an index anyway,
this ends up clearer IMO:

      for (int idx = 0; idx < argv.size(); idx++)
        debug_printf ("new_argv[%d] = \"%s\"\n", idx, argv[idx]);

> +	}
>        debug_flush ();
>      }
>  



>  handle_v_run (char *own_buf)
>  {
> -  char *p, *next_p, **new_argv;
> -  int i, new_argc;
> +  char *p, *next_p;
> +  std::vector<char *> new_argv;
> +  int new_argc;
> +  int i;
>  
>    new_argc = 0;
>    for (p = own_buf + strlen ("vRun;"); p && *p; p = strchr (p, ';'))
> @@ -2870,62 +2893,91 @@ handle_v_run (char *own_buf)
>        new_argc++;
>      }
>  
> -  new_argv = (char **) calloc (new_argc + 2, sizeof (char *));
> -  if (new_argv == NULL)
> -    {
> -      write_enn (own_buf);
> -      return 0;
> -    }
> -
> -  i = 0;
> -  for (p = own_buf + strlen ("vRun;"); *p; p = next_p)
> +  for (i = 0, p = own_buf + strlen ("vRun;"); *p; p = next_p, ++i)
>      {
>        next_p = strchr (p, ';');
>        if (next_p == NULL)
>  	next_p = p + strlen (p);
>  
> -      if (i == 0 && p == next_p)
> -	new_argv[i] = NULL;
> +      if (p == next_p)
> +	new_argv.push_back ("''");
>        else
>  	{
>  	  /* FIXME: Fail request if out of memory instead of dying.  */
> -	  new_argv[i] = (char *) xmalloc (1 + (next_p - p) / 2);
> -	  hex2bin (p, (gdb_byte *) new_argv[i], (next_p - p) / 2);
> -	  new_argv[i][(next_p - p) / 2] = '\0';
> +	  size_t len = 1 + (next_p - p) / 2;
> +	  char *s = (char *) xmalloc (len);
> +	  char *ss = (char *) xmalloc (len * 2);
> +	  char *tmp_s, *tmp_ss;
> +	  int need_quote;
> +
> +	  hex2bin (p, (gdb_byte *) s, (next_p - p) / 2);
> +	  s[(next_p - p) / 2] = '\0';
> +
> +	  tmp_s = s;
> +	  tmp_ss = ss;
> +	  need_quote = 0;
> +	  while (*tmp_s != '\0')
> +	    {
> +	      switch (*tmp_s)
> +		{
> +		case '\n':
> +		  *tmp_ss = '\'';
> +		  ++tmp_ss;
> +		  need_quote = 1;
> +		  break;
> +
> +		case '\'':
> +		  *tmp_ss = '\\';
> +		  ++tmp_ss;
> +		  break;
> +
> +		default:
> +		  break;
> +		}
> +
> +	      *tmp_ss = *tmp_s;
> +	      ++tmp_ss;
> +	      ++tmp_s;
> +	    }
> +
> +	  if (need_quote)
> +	    *tmp_ss++ = '\'';

Hmm, is this quoting stuff being moved from somewhere,
or it is new?

> +
> +	  *tmp_ss = '\0';
> +	  new_argv.push_back (ss);
> +	  xfree (s);
>  	}
>  
>        if (*next_p)
>  	next_p++;
> -      i++;
>      }
> -  new_argv[i] = NULL;

>  
> +  /* Gather information about the environment.  */
> +  our_environ = make_environ ();
> +  init_environ (our_environ);
> +
>    initialize_async_io ();
>    initialize_low ();
> +  /* This is called when initializing inflow on GDB.  */

What is this comment supposed be to telling?

> +  have_job_control ();
>    initialize_event_loop ();
>    if (target_supports_tracepoints ())
>      initialize_tracepoint ();
> @@ -3695,13 +3758,11 @@ captured_main (int argc, char *argv[])
>        int i, n;
>  
>        n = argc - (next_arg - argv);
> -      program_argv = XNEWVEC (char *, n + 1);
>        for (i = 0; i < n; i++)
> -	program_argv[i] = xstrdup (next_arg[i]);
> -      program_argv[i] = NULL;
> +	program_argv.push_back (xstrdup (next_arg[i]));
>  

> -#define create_inferior(program, args) \
> -  (*the_target->create_inferior) (program, args)
> +#define create_inferior(program) \
> +  (*the_target->create_inferior) (program)

program_argv, I suppose.

> +/* See common/common-utils.h.  */
> +
> +std::string
> +stringify_argv (std::vector<char *> &argv)
> +{
> +  std::string ret ("");

   std::string ret;

> +
> +  for (std::vector<char *>::iterator i = argv.begin () + 1;
> +       i != argv.end ();
> +       ++i)
> +    ret += *i + std::string (" ");

Should we remove the last empty space?

> +
> +  return ret;
> +}
> diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
> index b4ded31..a30d99a 100644
> --- a/gdb/gdbserver/utils.h
> +++ b/gdb/gdbserver/utils.h
> @@ -19,7 +19,17 @@
>  #ifndef UTILS_H
>  #define UTILS_H
>  
> +#include <vector>
> +
>  char *paddress (CORE_ADDR addr);
>  char *pfildes (gdb_fildes_t fd);
>  
> +/* Works like FREEARGV, but with std::vector.  */
> +extern void free_vector_argv (std::vector<char *> &v);

At some point, freeargv is eliminated, and this comment will
stay behind.  Can we expand it a bit?

> +
> +/* Given a vector of arguments ARGV, return a string equivalent to
> +   joining all the arguments (starting from ARGV + 1) with a
> +   whitespace separating them.  */
> +extern std::string stringify_argv (std::vector<char *> &argv);

const ?

This "starting from ARGV + 1" contract was surprising to me.
I only noticed this when I saw the implementation.

Does it really make sense to put the program name inside the
vector instead of on a separate argument if we're always going
to treat argv[0] differently?

I.e., would:

- ...create_inferior (char *program, char **program_args)
+ ...create_inferior (char *program, std::vector<char *> &program_args)

make more sense?

>  static int
> -win32_create_inferior (char *program, char **program_args)
> +win32_create_inferior (std::vector<char *> &program_argv)
>  {
>  #ifndef USE_WIN32API
>    char real_path[PATH_MAX];
> @@ -627,6 +625,9 @@ win32_create_inferior (char *program, char **program_args)
>    int argc;
>    PROCESS_INFORMATION pi;
>    DWORD err;
> +  char *program = program_argv[0];
> +  std::string program_args = stringify_argv (program_argv);
> +  char *args = (char *) program_args.c_str ();
>  
>    /* win32_wait needs to know we're not attaching.  */
>    attaching = 0;
> @@ -636,6 +637,8 @@ win32_create_inferior (char *program, char **program_args)
>  
>    flags = DEBUG_PROCESS | DEBUG_ONLY_THIS_PROCESS;
>  
> +  pre_fork_inferior (program, argv);

Hmm, fork on Windows?

> +
>  #ifndef USE_WIN32API
>    orig_path = NULL;
>    path_ptr = getenv ("PATH");
> @@ -652,18 +655,6 @@ win32_create_inferior (char *program, char **program_args)
>    program = real_path;
>  #endif
>  
> -  argslen = 1;
> -  for (argc = 1; program_args[argc]; argc++)
> -    argslen += strlen (program_args[argc]) + 1;
> -  args = (char *) alloca (argslen);
> -  args[0] = '\0';
> -  for (argc = 1; program_args[argc]; argc++)
> -    {
> -      /* FIXME: Can we do better about quoting?  How does Cygwin
> -	 handle this?  */
> -      strcat (args, " ");
> -      strcat (args, program_args[argc]);
> -    }
>    OUTMSG2 (("Command line is \"%s\"\n", args));
>  
>  #ifdef CREATE_NEW_PROCESS_GROUP
> @@ -704,6 +695,8 @@ win32_create_inferior (char *program, char **program_args)
>  
>    do_initial_child_stuff (pi.hProcess, pi.dwProcessId, 0);
>  
> +  post_fork_inferior (current_process_id, program_argv);
> +
>    return current_process_id;
>  }
>  


> @@ -39,8 +39,14 @@ set spawn_id [remote_spawn target "$gdbserver stdio non-existing-program"]
>  set msg "gdbserver exits cleanly"
>  set saw_exiting 0
>  expect {
> -    # This is what we get on ptrace-based targets.
> -    -re "stdin/stdout redirected.*No program to debug\r\nExiting\r\n$" {
> +    # This is what we get on ptrace-based targets with
> +    # startup-with-shell disabled.
> +    -re "stdin/stdout redirected.*gdbserver: Cannot exec non-existing-program\r\ngdbserver: Error: No such file or directory\r\n\r\nDuring startup program exited with code 127\.\r\nExiting\r\n$" {
> +	set saw_exiting 1
> +	exp_continue

Shouldn't this be a part of the next patch?

> +    }
> +    # Likewise, but with startup-with-shell enabled.
> +    -re "stdin/stdout redirected.*exec: non-existing-program: not found\r\nDuring startup program exited with code 127\.\r\nExiting\r\n$" {
>  	set saw_exiting 1
>  	exp_continue
>      }

Thanks,
Pedro Alves