Re: [PATCH v6 3/4] Share fork_inferior et al with gdbserver

[Pedro Alves <palves at redhat dot com>]

On 05/04/2017 06:29 AM, Sergio Durigan Junior wrote:

> diff --git a/gdb/common/common-inferior.h b/gdb/common/common-inferior.h
> new file mode 100644
> index 0000000..206a36a
> --- /dev/null
> +++ b/gdb/common/common-inferior.h
> @@ -0,0 +1,75 @@
> +/* Variables that describe the inferior process running under GDB and
> +   GDBserver: Where it is, why it stopped, and how to step it.

Stale comment.

> +
> +   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_INFERIOR_H
> +#define COMMON_INFERIOR_H
> +
> +/* Number of traps that happen between exec'ing the shell to run an
> +   inferior and when we finally get to the inferior code, not counting
> +   the exec for the shell.  This is 1 on all supported
> +   implementations.  */
> +#define START_INFERIOR_TRAPS_EXPECTED 1

The copy in gdb/inferior.h should be removed instead of being left
duplicated.

> +
> +/* Whether to start up the debuggee under a shell.
> +
> +   If startup-with-shell is set, GDB's "run" will attempt to start up
> +   the debuggee under a shell.  This also happens when using GDBserver
> +   under extended remote mode.
> +
> +   This is in order for argument-expansion to occur.  E.g.,
> +
> +   (gdb) run *
> +
> +   The "*" gets expanded by the shell into a list of files.
> +
> +   While this is a nice feature, it may be handy to bypass the shell
> +   in some cases.  To disable this feature, do "set startup-with-shell
> +   false".
> +
> +   The catch-exec traps expected during start-up will be one more if
> +   the target is started up with a shell.  */
> +extern int startup_with_shell;
> +
> +/* Perform any necessary tasks before a fork/vfork takes place.  ARGS
> +   is a string containing all the arguments received by the inferior.
> +   This function is mainly used by fork_inferior.  */
> +extern void prefork_hook (const char *args);
> +
> +/* Perform any necessary tasks after a fork/vfork takes place.  This
> +   function is mainly used by fork_inferior.  */
> +extern void postfork_hook (pid_t pid);
> +
> +/* Perform any necessary tasks *on the child* after a fork/vfork takes
> +   place.  DEBUG_FORK is the number of seconds that we should sleep
> +   before exec'ing (see fork_inferior).
> +
> +   This function is mainly used by fork_inferior.  */
> +extern void postfork_child_hook (int debug_fork);

Wouldn't it better to put these in nat/fork-inferior.h instead?
Likewise START_INFERIOR_TRAPS_EXPECTED, maybe?

> +
> +/* Return the exec wrapper to be used when starting the inferior, or NULL
> +   otherwise.  */
> +extern const char *get_exec_wrapper (void);

Should really drop the "(void)" in all new code throughout the series.

> +
> +/* Return the name of the executable file as a string.
> +   ERR nonzero means get error if there is none specified;
> +   otherwise return 0 in that case.  */
> +extern char *get_exec_file (int err);
> +
> +#endif /* ! COMMON_INFERIOR_H */


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

  std::string ret;

> +
> +  if (!args.empty ())
> +    {
> +      for (auto s : args)
> +	if (s != NULL)
> +	  ret += s + std::string (" ");

Avoid unnecessary temporaries:

          {
	    ret += s;
            ret += ' ';
          }

> +
> +      /* Erase the last whitespace.  */
> +      ret.erase (ret.end () - 1);
> +    }
> +
> +  return ret;
> +}



>  
> -/* Accept NTRAPS traps from the inferior.  */
> +/* See common/common-inferior.h.  */
>  
>  void
> -startup_inferior (int ntraps)
> +postfork_child_hook (int debug_fork)
>  {
> -  int pending_execs = ntraps;
> -  int terminal_initted = 0;
> -  ptid_t resume_ptid;
> -
> -  if (startup_with_shell)
> -    {
> -      /* One trap extra for exec'ing the shell.  */
> -      pending_execs++;
> -    }
> +  /* This is set to the result of setpgrp, which if vforked, will be
> +     visible to you in the parent process.  It's only used by humans
> +     for debugging.  */
> +  static int debug_setpgrp = 657473;
>  
> -  if (target_supports_multi_process ())
> -    resume_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
> -  else
> -    resume_ptid = minus_one_ptid;
> +  /* Make sure we switch to main_ui here in order to be able to
> +     use the fprintf_unfiltered/warning/error functions.  */
> +  current_ui = main_ui;
>  
> -  /* The process was started by the fork that created it, but it will
> -     have stopped one instruction after execing the shell.  Here we
> -     must get it up to actual execution of the real program.  */
> +  /* Close all file descriptors except those that gdb inherited
> +     (usually 0/1/2), so they don't leak to the inferior.  Note
> +     that this closes the file descriptors of all secondary
> +     UIs.  */
> +  close_most_fds ();

Any reason this closing isn't done in the common code instead of
having it done on both hook implementations?

>  
> -  if (exec_wrapper)
> -    pending_execs++;
> +  if (debug_fork)
> +    sleep (debug_fork);

Similarly, why not leave this sleep to the common code?  It's
missing on the gdbserver side, AFAICS.

> +	  case TARGET_WAITKIND_SPURIOUS:
> +	  case TARGET_WAITKIND_LOADED:
> +	  case TARGET_WAITKIND_FORKED:
> +	  case TARGET_WAITKIND_VFORKED:
> +	  case TARGET_WAITKIND_SYSCALL_ENTRY:
> +	  case TARGET_WAITKIND_SYSCALL_RETURN:
> +	  case TARGET_WAITKIND_VFORK_DONE:
> +	  case TARGET_WAITKIND_IGNORE:
> +	  case TARGET_WAITKIND_NO_HISTORY:
> +	  case TARGET_WAITKIND_NO_RESUMED:
> +	  case TARGET_WAITKIND_THREAD_CREATED:
> +	  case TARGET_WAITKIND_THREAD_EXITED:

You added several of these at the same time (but they don't really
make sense here).  Did you run into some of them in gdbserver?

> index 4ea7913..8aa85db 100644
> --- a/gdb/gdbserver/configure.ac
> +++ b/gdb/gdbserver/configure.ac
> @@ -462,7 +462,9 @@ esac],
>  
>  if $want_ipa ; then
>     if $have_ipa ; then
> -     IPA_DEPFILES="$ipa_obj"
> +     # Needed because safe_strerror's definition is host-dependent

Why do we end up needing safe_strerror in the IPA in the first place?

> +     strerror_obj="`echo $srv_host_obs | sed 's/\(.*-strerror\)\.o/\1-ipa.o/'`"
> +     IPA_DEPFILES="$ipa_obj $strerror_obj"
>       extra_libraries="$extra_libraries libinproctrace.so"
>     else
>       AC_MSG_ERROR([inprocess agent not supported for this target])



> -#if defined(__UCLIBC__) && defined(HAS_NOMMU)
> -  pid = vfork ();

Does fork_inferior end up always using vfork on no-MMU
ports somehow?

> -#else
> -  pid = fork ();
> -#endif

> +const char *
> +get_exec_wrapper (void)
>  {
> -  std::vector<char *> new_argv;
> +  static std::string ret;
> +  static bool initialized_p = false;
>  
> -  if (!wrapper_argv.empty ())
> -    new_argv.insert (new_argv.begin (),
> -		     wrapper_argv.begin (),
> -		     wrapper_argv.end ());
> +  if (wrapper_argv.empty ())
> +    return NULL;
>  
> -  for (int i = 0; argv[i] != NULL; ++i)
> -    new_argv.push_back (argv[i]);
> +  if (!initialized_p)
> +    {
> +      for (auto s : wrapper_argv)
> +	ret += s + std::string (" ");

Avoid unnecessary temporaries.

>  
> -  new_argv.push_back (NULL);
> +      /* Erase the last whitespace.  */
> +      ret.erase (ret.end () - 1);
> +
> +      initialized_p = true;
> +    }
> +
> +  return ret.c_str ();
> +}

I'm actually having a bit of trouble understanding
why do we need the wrapper_argv global plus this initialized_p
indirection, instead of just building the exec wrapper string
in captured_main.


> @@ -2848,6 +2893,7 @@ handle_v_run (char *own_buf)
>  {


> +	  /* FIXME: Fail request if out of memory instead of dying.  */
>  	  size_t len = (next_p - p) / 2;

Did you really mean to add that comment?


> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index d5fee38..5087614 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -132,6 +132,7 @@ extern int in_queued_stop_replies (ptid_t ptid);
>  #include "utils.h"
>  #include "debug.h"
>  #include "gdb_vecs.h"
> +#include <vector>
>  

Why?

>  /* Maximum number of bytes to read/write at once.  The value here
>     is chosen to fill up a packet (the headers account for the 32).  */
> @@ -148,4 +149,13 @@ extern int in_queued_stop_replies (ptid_t ptid);
>  /* Definition for any syscall, used for unfiltered syscall reporting.  */
>  #define ANY_SYSCALL (-2)
>  
> +/* After fork_inferior has been called, we need to adjust a few
> +   signals and call startup_inferior.  This is done here.  PID is the
> +   pid of the new inferior, and PROGRAM is its name.  */
> +extern void post_fork_inferior (int pid, const char *program);
> +
> +/* Get the 'struct gdb_environ *' being used in the current
> +   session.  */
> +extern struct gdb_environ *get_environ (void);
> +
>  #endif /* SERVER_H */

> +
> +/* See target/target.h.  */
> +
> +void
> +target_terminal_init (void)
> +{
> +  /* To be implemented.  */
> +}
> +
> +/* See target/target.h.  */
> +
> +void
> +target_terminal_inferior (void)
> +{
> +  /* To be implemented.  */
> +}
> +
> +/* See target/target.h.  */
> +
> +void
> +target_terminal_ours (void)
> +{
> +  /* To be implemented.  */
> +}

s/To be implemented/Not necessary/ or some such.

> diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
> index b4ded31..48804c5 100644
> --- a/gdb/gdbserver/utils.h
> +++ b/gdb/gdbserver/utils.h
> @@ -19,6 +19,8 @@
>  #ifndef UTILS_H
>  #define UTILS_H
>  
> +#include <vector>
> +

Why?

>  char *paddress (CORE_ADDR addr);
>  char *pfildes (gdb_fildes_t fd);
>  

>  static int
> -win32_create_inferior (char *program, char **program_args)
> +win32_create_inferior (const char *program,
> +		       const std::vector<char *> &program_args)
>  {
>  #ifndef USE_WIN32API
>    char real_path[PATH_MAX];
> @@ -627,6 +627,8 @@ win32_create_inferior (char *program, char **program_args)
>    int argc;
>    PROCESS_INFORMATION pi;
>    DWORD err;
> +  std::string str_program_args = stringify_argv (program_argv);
> +  char *args = (char *) str_program_args.c_str ();

Can it be const?

> +
> +/* When executing a command under the given shell, return non-zero if
> +   the '!' character should be escaped when embedded in a quoted
> +   command-line argument.  */
> +
> +static int
> +escape_bang_in_quoted_argument (const char *shell_file)
> +{
> +  const int shell_file_len = strlen (shell_file);
> +
> +  /* Bang should be escaped only in C Shells.  For now, simply check
> +     that the shell name ends with 'csh', which covers at least csh
> +     and tcsh.  This should be good enough for now.  */
> +
> +  if (shell_file_len < 3)
> +    return 0;
> +
> +  if (shell_file[shell_file_len - 3] == 'c'
> +      && shell_file[shell_file_len - 2] == 's'
> +      && shell_file[shell_file_len - 1] == 'h')
> +    return 1;
> +
> +  return 0;
> +}

This function lost some edits done in the execv_argv patch.

bool + size_t.

Please run a diff of the old gdb/fork-child.c against the
new nat/fork-inferior.c to make sure nothing else was lost.


> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
> new file mode 100644
> index 0000000..6a8528e
> --- /dev/null
> +++ b/gdb/nat/fork-inferior.h
> @@ -0,0 +1,51 @@
...

> +
> +#ifndef FORK_CHILD_H
> +#define FORK_CHILD_H

Incorrectly named multiple-inclusion guard.

> +
> +#include <vector>

Why?  Nothing uses std::vector here.

> +
> +/* Accept NTRAPS traps from the inferior.
> +
> +   Return the ptid of the inferior being started.  */
> +extern ptid_t startup_inferior (pid_t pid, int ntraps,
> +				struct target_waitstatus *mystatus,
> +				ptid_t *myptid);
> +
> +/* Start an inferior Unix child process and sets inferior_ptid to its
> +   pid.  EXEC_FILE is the file to run.  ALLARGS is a string containing
> +   the arguments to the program.  ENV is the environment vector to
> +   pass.  SHELL_FILE is the shell file, or NULL if we should pick
> +   one.  EXEC_FUN is the exec(2) function to use, or NULL for the default
> +   one.  */
> +
> +/* This function is NOT reentrant.  Some of the variables have been
> +   made static to ensure that they survive the vfork call.  */
> +extern pid_t fork_inferior (const char *exec_file_arg,
> +			    const std::string &allargs,

Should include <string>.

> +			    char **env, void (*traceme_fun) (void),
> +			    void (*init_trace_fun) (int),
> +			    void (*pre_trace_fun) (void),
> +			    const char *shell_file_arg,
> +			    void (*exec_fun) (const char *file,
> +					      char * const *argv,
> +					      char * const *env));
> +
> +#endif /* ! FORK_CHILD_H */

>  
> diff --git a/gdb/target.h b/gdb/target.h
> index a971adf..6c995a1 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1538,17 +1538,6 @@ extern int target_terminal_is_inferior (void);
>  
>  extern int target_terminal_is_ours (void);
>  
> -/* Initialize the terminal settings we record for the inferior,
> -   before we actually run the inferior.  */
> -
> -extern void target_terminal_init (void);
> -
> -/* Put the inferior's terminal settings into effect.  This is
> -   preparation for starting or resuming the inferior.  This is a no-op
> -   unless called with the main UI as current UI.  */
> -
> -extern void target_terminal_inferior (void);
> -
>  /* Put some of our terminal settings into effect, enough to get proper
>     results from our output, but do not change into or out of RAW mode
>     so that no input is discarded.  This is a no-op if terminal_ours
> @@ -1557,12 +1546,6 @@ extern void target_terminal_inferior (void);
>  
>  extern void target_terminal_ours_for_output (void);
>  
> -/* Put our terminal settings into effect.  First record the inferior's
> -   terminal settings so they can be restored properly later.  This is
> -   a no-op unless called with the main UI as current UI.  */
> -
> -extern void target_terminal_ours (void);
> -

Please leave breakcrumb comments behind.

>  /* Return true if the target stack has a non-default
>    "to_terminal_ours" method.  */
>  

Thanks,
Pedro Alves