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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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).

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));
+}
+
+/* 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.";
+
+	  /* 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.  */
+
+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.  */
+    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;
 
   /* 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 ());
+    }
+
   /* Restore our environment in case a vforked child clob'd it.  */
   environ = save_our_env;
 
@@ -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,
+                                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,
+                                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 */
-- 
2.24.1