Re: [PATCH v2 3/5] Expand 'fork_inferior' to check whether 'traceme_fun' succeeded

[Pedro Alves <palves@redhat.com>]

On 3/17/20 3:47 PM, Sergio Durigan Junior via Gdb-patches wrote:
> This patch is one important piece of the series.  It expands
> 'fork_inferior' in order to deal with new steps in the process of
> initializing the inferior.  We now have to:
> 
> - Create a pipe that will be used to communicate with our
>   fork (pre-exec), and which the fork will use to pass back to us the
>   errno value of the 'traceme_fun' call.
> 
> - Close this pipe after it is used.
> 
> - Check the errno value passed back from the fork, and report any
>   problems in the initialization to the user.
> 
> I thought about and implemented a few designs for all of this, but
> ended up sticking with the function overload one.  'fork_inferior' is
> now two functions: one that will take a traceme function like
> '(*traceme_fun) ()' --- i.e., the original 'fork_inferior' behaviour,
> and other that will take a function like '(*traceme_fun) (int
> trace_pipe_write)'.  Depending on which function it takes, we know
> whether the user does not want us to check whether the 'traceme_fun'
> call was successful (former) or if she does (latter).

Pedantically: user -> caller.  she -> it.  "user" normally refers to
a person who uses gdb.

> All in all, the patch is not complicated to understand and keeps the
> interface clean enough so that we don't have to update every caller of
> 'fork_inferior' (which was a problem with previous designs I tried).
> 
> The subsequent patch will build on top of this one and implement the
> errno-passing-via-pipe on the GNU/Linux target.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	 * nat/fork-inferior.c: Include "gdbsupport/scoped_pipe.h".
> 	 (default_trace_me_fail_reason): New function.
> 	 (trace_me_fail_reason): New variable.
> 	 (write_trace_errno_to_pipe): New function.
> 	 (read_trace_errno_from_pipe): Likewise.
> 	 (check_child_trace_me_errno): Likewise.
> 	 (traceme_info): New struct.
> 	 (fork_inferior_1): Renamed from 'fork_inferior'.
> 	 (fork_inferior): New overloads.
> 	 (trace_start_error_with_name): Add "append" parameter.
> 	 * nat/fork-inferior.h (fork_inferior): Expand comment.
> 	 Add overload declaration.
> 	 (trace_start_error_with_name): Add "append" parameter.
> 	 (trace_me_fail_reason): New variable.
> 	 (check_child_trace_me_errno): New function.
> 	 (write_trace_errno_to_pipe): Likewise.
> ---
>  gdb/nat/fork-inferior.c | 231 ++++++++++++++++++++++++++++++++++++----
>  gdb/nat/fork-inferior.h |  87 ++++++++++++---
>  2 files changed, 288 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
> index 1185ef8998..223ff44195 100644
> --- a/gdb/nat/fork-inferior.c
> +++ b/gdb/nat/fork-inferior.c
> @@ -27,6 +27,7 @@
>  #include "gdbsupport/pathstuff.h"
>  #include "gdbsupport/signals-state-save-restore.h"
>  #include "gdbsupport/gdb_tilde_expand.h"
> +#include "gdbsupport/scoped_pipe.h"
>  #include <vector>
>  
>  extern char **environ;
> @@ -262,16 +263,157 @@ execv_argv::init_for_shell (const char *exec_file,
>    m_argv.push_back (NULL);
>  }
>  
> -/* See nat/fork-inferior.h.  */
> +/* Default implementation of 'trace_me_fail_reason'.  Always return
> +   an empty string.  */
>  
> -pid_t
> -fork_inferior (const char *exec_file_arg, const std::string &allargs,
> -	       char **env, void (*traceme_fun) (),
> -	       gdb::function_view<void (int)> init_trace_fun,
> -	       void (*pre_trace_fun) (),
> -	       const char *shell_file_arg,
> -               void (*exec_fun)(const char *file, char * const *argv,
> -                                char * const *env))
> +static std::string
> +default_trace_me_fail_reason (int err)
> +{
> +  return {};
> +}
> +
> +/* See fork-inferior.h.  */
> +
> +std::string (*trace_me_fail_reason) (int err)
> +  = default_trace_me_fail_reason;
> +
> +/* See fork-inferior.h.  */
> +
> +void
> +write_trace_errno_to_pipe (int writepipe, int trace_errno)
> +{
> +  ssize_t writeret;
> +
> +  do
> +    {
> +      writeret = write (writepipe, &trace_errno, sizeof (trace_errno));
> +    }
> +  while (writeret < 0 && (errno == EAGAIN || errno == EINTR));
> +
> +  if (writeret < 0)
> +    error (_("Could not write trace errno: %s"), safe_strerror (errno));

I'll have a comment about this error in a following patch.


> +}
> +
> +/* Helper function to read TRACE_ERRNO from READPIPE, which handles
> +   EINTR/EAGAIN and throws and exception if there was an error.  */
> +
> +static int
> +read_trace_errno_from_pipe (int readpipe)
> +{
> +  ssize_t readret;
> +  int trace_errno;
> +
> +  do
> +    {
> +      readret = read (readpipe, &trace_errno, sizeof (trace_errno));
> +    }
> +  while (readret < 0 && (errno == EAGAIN || errno == EINTR));
> +
> +  if (readret < 0)
> +    error (_("Could not read trace errno: %s"), safe_strerror (errno));
> +
> +  return trace_errno;
> +}
> +
> +/* See fork-inferior.h.  */
> +
> +void
> +check_child_trace_me_errno (int readpipe)
> +{
> +  fd_set rset;
> +  struct timeval timeout;
> +  int ret;
> +
> +  /* Make sure we have a valid 'trace_me_fail_reason' function
> +     defined.  */
> +  gdb_assert (trace_me_fail_reason != nullptr);
> +
> +  FD_ZERO (&rset);
> +  FD_SET (readpipe, &rset);
> +
> +  /* Five seconds should be plenty of time to wait for the child's
> +     reply.  */
> +  timeout.tv_sec = 5;
> +  timeout.tv_usec = 0;
> +
> +  do
> +    {
> +      ret = select (readpipe + 1, &rset, NULL, NULL, &timeout);
> +    }
> +  while (ret < 0 && (errno == EAGAIN || errno == EINTR));
> +
> +  if (ret < 0)
> +    perror_with_name ("select");
> +  else if (ret == 0)
> +    error (_("Timeout while waiting for child's trace errno"));
> +  else
> +    {
> +      int child_errno;
> +
> +      child_errno = read_trace_errno_from_pipe (readpipe);
> +
> +      if (child_errno != 0)
> +	{
> +	  /* The child can't use TRACE_TRACEME.  We have to check whether
> +	     we know the reason for the failure, and then error out.  */
> +	  std::string reason = trace_me_fail_reason (child_errno);
> +
> +	  if (reason.empty ())
> +	    reason = "Could not determine reason for trace failure.";

Missing i18n.

> +
> +	  /* The child is supposed to display a warning containing the
> +	     safe_strerror message before us, so we just display the
> +	     possible reason for the failure.  */
> +	  error ("%s", reason.c_str ());
> +	}
> +    }
> +}
> +
> +/* Helper struct for fork_inferior_1, containing information on
> +   whether we should check if TRACEME_FUN was successfully called or
> +   not.  */

"was successfully called" seems ambiguous -- I first read it as,
"whether we managed to call TRACEME_FUN", which is not what you meant,
since we always call it.

It'd suggest:

 "should check whether TRACEME_FUN succeeded tracing the child"

> +
> +struct traceme_info
> +{
> +  /* True if we should check whether the call to 'traceme_fun
> +     (TRACE_ME...)' succeeded or not. */
> +  bool check_trace_me_fail_reason;
> +
> +  union
> +  {
> +    /* The non-check version of TRACEME_FUN.  It will be set if
> +       CHECK_TRACEME_FAIL_REASON is false.
> +
> +       This function will usually just perform the call to whatever
> +       trace function needed to start tracing the inferior (e.g.,
> +       ptrace).  */
> +    void (*traceme_fun_nocheck) ();
> +
> +    /* The check version of TRACEME_FUN.  It will be set if
> +       CHECK_TRACEME_FAIL_REASON is true.
> +
> +       This function will usually perform the call to whatever trace
> +       function needed to start tracing the inferior, but will also
> +       write its errno value to TRACE_ERRNO_PIPE, so that
> +       fork_inferior_1 can check whether it suceeded.  */

Typo: "suceeded" -> "succeeded".

I'd drop the "usually" both places.  It's not usually, it's always.

> +    void (*traceme_fun_check) (int trace_errno_pipe);
> +  } u;
> +};
> +
> +/* Helper function.
> +
> +   Depending on the value of TRACEME_INFO.CHECK_TRACEME_FAIL_REASON,
> +   this function will check whether the call to TRACEME_FUN succeeded
> +   or not.  */
> +
> +static pid_t
> +fork_inferior_1 (const char *exec_file_arg, const std::string &allargs,
> +		 char **env, const struct traceme_info traceme_info,
> +		 gdb::function_view<void (int)> init_trace_fun,
> +		 void (*pre_trace_fun) (),
> +		 const char *shell_file_arg,
> +		 void (*exec_fun)(const char *file, char * const *argv,
> +				  char * const *env))
>  {
>    pid_t pid;
>    /* Set debug_fork then attach to the child while it sleeps, to debug.  */
> @@ -283,6 +425,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>    int save_errno;
>    const char *inferior_cwd;
>    std::string expanded_inferior_cwd;
> +  scoped_pipe trace_pipe;

You could wrap this in a gdb::optional to avoid creating the pipe
unless necessary.  Like:

  gdb::optional<scoped_pipe> trace_pipe;
  if (traceme_info.check_trace_me_fail_reason)
    trace_pipe.emplace ();

>  
>    /* If no exec file handed to us, get it from the exec-file command
>       -- with a good, common error message if none is specified.  */
> @@ -365,12 +508,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>  
>    if (pid == 0)
>      {
> -      /* 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 ();
> -
>        /* Change to the requested working directory if the user
>  	 requested it.  */
>        if (inferior_cwd != NULL)
> @@ -392,7 +529,10 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>           for the inferior.  */
>  
>        /* "Trace me, Dr. Memory!"  */
> -      (*traceme_fun) ();
> +      if (traceme_info.check_trace_me_fail_reason)
> +	(*traceme_info.u.traceme_fun_check) (trace_pipe.get_write_end ());
> +      else
> +	(*traceme_info.u.traceme_fun_nocheck) ();
>  
>        /* The call above set this process (the "child") as debuggable
>          by the original gdb process (the "parent").  Since processes
> @@ -403,6 +543,12 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>          saying "not parent".  Sorry; you'll have to use print
>          statements!  */
>  
> +      /* 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, and the trace errno pipe (if it's open).  */
> +      close_most_fds ();
> +
>        restore_original_signals_state ();
>  
>        /* There is no execlpe call, so we have to set the environment
> @@ -431,6 +577,13 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>        _exit (0177);
>      }
>  
> +  if (traceme_info.check_trace_me_fail_reason)
> +    {
> +      /* Check the trace errno, and inform the user about the reason
> +	 of the failure, if there was any.  */
> +      check_child_trace_me_errno (trace_pipe.get_read_end ());
> +    }
> +

check_child_trace_me_errno can throw an error.  So it should be done after
we've restored all global state.  See the environ restore just below ...

>    /* Restore our environment in case a vforked child clob'd it.  */
>    environ = save_our_env;

... here.  You'd miss restoring this.

>  
> @@ -448,6 +601,48 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>    return pid;
>  }
>  
> +/* See fork-inferior.h.  */
> +
> +pid_t
> +fork_inferior (const char *exec_file_arg, const std::string &allargs,
> +	       char **env, void (*traceme_fun) (),
> +	       gdb::function_view<void (int)> init_trace_fun,
> +	       void (*pre_trace_fun) (),
> +	       const char *shell_file_arg,
> +               void (*exec_fun)(const char *file, char * const *argv,

Something odd with indentation here.  Likely tabs vs spaces.

> +                                char * const *env))
> +{
> +  struct traceme_info traceme_info;
> +
> +  traceme_info.check_trace_me_fail_reason = false;
> +  traceme_info.u.traceme_fun_nocheck = traceme_fun;
> +
> +  return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info,
> +			  init_trace_fun, pre_trace_fun, shell_file_arg,
> +			  exec_fun);
> +}
> +
> +/* See fork-inferior.h.  */
> +
> +pid_t
> +fork_inferior (const char *exec_file_arg, const std::string &allargs,
> +	       char **env, void (*traceme_fun) (int trace_errno_pipe),
> +	       gdb::function_view<void (int)> init_trace_fun,
> +	       void (*pre_trace_fun) (),
> +	       const char *shell_file_arg,
> +               void (*exec_fun)(const char *file, char * const *argv,

Ditto re. indentation.


> +                                char * const *env))
> +{
> +  struct traceme_info traceme_info;
> +
> +  traceme_info.check_trace_me_fail_reason = true;
> +  traceme_info.u.traceme_fun_check = traceme_fun;
> +
> +  return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info,
> +			  init_trace_fun, pre_trace_fun, shell_file_arg,
> +			  exec_fun);
> +}
> +
>  /* See nat/fork-inferior.h.  */
>  
>  ptid_t
> @@ -592,7 +787,7 @@ trace_start_error (const char *fmt, ...)
>  /* See nat/fork-inferior.h.  */
>  
>  void
> -trace_start_error_with_name (const char *string)
> +trace_start_error_with_name (const char *string, const char *append)
>  {
> -  trace_start_error ("%s: %s", string, safe_strerror (errno));
> +  trace_start_error ("%s: %s%s", string, safe_strerror (errno), append);
>  }
> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
> index cf6f137edd..b67215353f 100644
> --- a/gdb/nat/fork-inferior.h
> +++ b/gdb/nat/fork-inferior.h
> @@ -32,17 +32,41 @@ struct process_stratum_target;
>  #define START_INFERIOR_TRAPS_EXPECTED 1
>  
>  /* 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.  */
> +   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.
> +
> +   This function does not check whether the call to TRACEME_FUN
> +   succeeded or not.  */
>  extern pid_t fork_inferior (const char *exec_file_arg,
>  			    const std::string &allargs,
> -			    char **env, void (*traceme_fun) (),
> +			    char **env,
> +			    void (*traceme_fun) (),
> +			    gdb::function_view<void (int)> init_trace_fun,
> +			    void (*pre_trace_fun) (),
> +			    const char *shell_file_arg,
> +			    void (*exec_fun) (const char *file,
> +					      char * const *argv,
> +					      char * const *env));
> +
> +/* Like fork_inferior above, but check whether the call to TRACEME_FUN
> +   succeeded or not.  */
> +extern pid_t fork_inferior (const char *exec_file_arg,
> +			    const std::string &allargs,
> +			    char **env,
> +			    void (*traceme_fun) (int trace_errno_pipe),
>  			    gdb::function_view<void (int)> init_trace_fun,
>  			    void (*pre_trace_fun) (),
>  			    const char *shell_file_arg,
> @@ -82,9 +106,48 @@ extern void trace_start_error (const char *fmt, ...)
>    ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
>  
>  /* Like "trace_start_error", but the error message is constructed by
> -   combining STRING with the system error message for errno.  This
> -   function does not return.  */
> -extern void trace_start_error_with_name (const char *string)
> +   combining STRING with the system error message for errno, and
> +   (optionally) with APPEND.  This function does not return.  */
> +extern void trace_start_error_with_name (const char *string,
> +					 const char *append = "")
>    ATTRIBUTE_NORETURN;
>  
> +/* Pointer to function which can be called by
> +   'check_child_trace_me_errno' when we need to determine the reason
> +   of a e.g. 'ptrace (PTRACE_ME, ...)' failure.  ERR is the ERRNO
> +   value set by the failing ptrace call.
> +
> +   By default, the function returns an empty string (see
> +   fork-inferior.c).
> +
> +   This pointer can be overriden by targets that want to personalize
> +   the error message printed when trace fails (see linux-nat.c or
> +   gdbserver/linux-low.c, for example).  */
> +extern std::string (*trace_me_fail_reason) (int err);
> +
> +/* Check the "trace me" errno (generated when executing e.g. 'ptrace
> +   (PTRACE_ME, ...)') of the child process that was created by
> +   GDB/GDBserver when creating an inferior.  The errno value will be
> +   passed via a pipe (see 'fork_inferior'), and READPIPE is the read
> +   end of the pipe.
> +
> +   If possible (i.e., if 'trace_me_fail_reason' is defined by the
> +   target), then we also try to determine the possible reason for a
> +   failure.
> +
> +   The idea is to wait a few seconds (via 'select') until something is
> +   written on READPIPE.  When that happens, we check if the child's
> +   trace errno is different than 0.  If it is, we call the function
> +   'trace_me_fail_reason' and try to obtain the reason for the
> +   failure, and then throw an exception (with the reason as the
> +   exception's message).
> +
> +   If nothing is written on the pipe, or if 'select' fails, we also
> +   throw exceptions.  */
> +extern void check_child_trace_me_errno (int readpipe);
> +
> +/* Helper function to write TRACE_ERRNO to WRITEPIPE, which handles
> +   EINTR/EAGAIN and throws an exception if there was an error.  */
> +extern void write_trace_errno_to_pipe (int writepipe, int trace_errno);
> +
>  #endif /* NAT_FORK_INFERIOR_H */
> 

Thanks,
Pedro Alves