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

[Pedro Alves <palves@redhat.com>]

So here's the patch showing what I meant in:
 https://sourceware.org/ml/gdb-patches/2017-06/msg00169.html

I'll be happy with the resulting patch once if you fold this in.
Could you do that, retest and repost?

I think we could get rid of the new common-inferior.h
header too, but I'll leave that as is, for now at least.

From 1d66adbd79ab97415d519f18906257e99ce49706 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 7 Jun 2017 13:18:14 +0100
Subject: [PATCH] Fixes

---
 gdb/common/common-utils.c   |  24 ----------
 gdb/common/common-utils.h   |  16 -------
 gdb/fork-child.c            |   9 ++++
 gdb/gdbserver/configure     |   4 +-
 gdb/gdbserver/configure.ac  |   4 +-
 gdb/gdbserver/configure.srv |   6 +--
 gdb/gdbserver/fork-child.c  | 113 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/linux-low.c   |   4 +-
 gdb/gdbserver/lynx-low.c    |   2 +-
 gdb/gdbserver/server.c      | 100 ++-------------------------------------
 gdb/gdbserver/server.h      |  18 ++++---
 gdb/gdbserver/spu-low.c     |   3 +-
 gdb/gdbserver/utils.c       |   9 ----
 gdb/inf-ptrace.c            |   2 +-
 gdb/inferior.h              |   1 -
 gdb/nat/fork-inferior.c     |  26 ++++++++++
 gdb/nat/fork-inferior.h     |  16 +++++++
 gdb/utils.c                 |   9 ----
 18 files changed, 185 insertions(+), 181 deletions(-)
 create mode 100644 gdb/gdbserver/fork-child.c

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index fbec0da..793ab3b 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -352,27 +352,3 @@ stringify_argv (const std::vector<char *> &args)
 
   return ret;
 }
-
-/* See common/common-inferior.h.  */
-
-void
-trace_start_error (const char *fmt, ...)
-{
-  va_list ap;
-
-  va_start (ap, fmt);
-  warning ("Could not trace the inferior process.\nError: ");
-  vwarning (fmt, ap);
-  va_end (ap);
-
-  gdb_flush_out_err ();
-  _exit (0177);
-}
-
-/* See common/common-inferior.h.  */
-
-void
-trace_start_error_with_name (const char *string)
-{
-  trace_start_error ("%s: %s", string, safe_strerror (errno));
-}
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 38505e0..787bac9 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -112,20 +112,4 @@ extern void free_vector_argv (std::vector<char *> &v);
    joining all the arguments with a whitespace separating them.  */
 extern std::string stringify_argv (const std::vector<char *> &argv);
 
-/* Flush both stdout and stderr.  This function needs to be
-   implemented differently on GDB and GDBserver.  */
-extern void gdb_flush_out_err ();
-
-/* Report an error that happened when starting to trace the inferior
-   (i.e., when the "traceme_fun" callback is called on fork_inferior)
-   and bail out.  This function does not return.  */
-extern void trace_start_error (const char *fmt, ...)
-  ATTRIBUTE_NORETURN;
-
-/* 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)
-  ATTRIBUTE_NORETURN;
-
 #endif
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 39e49b5..60985d8 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -43,6 +43,15 @@ get_exec_wrapper ()
   return exec_wrapper;
 }
 
+/* See nat/fork-inferior.h.  */
+
+void
+gdb_flush_out_err ()
+{
+  gdb_flush (main_ui->m_gdb_stdout);
+  gdb_flush (main_ui->m_gdb_stderr);
+}
+
 /* The ui structure that will be saved on 'prefork_hook' and
    restored on 'postfork_hook'.  */
 static struct ui *saved_ui = NULL;
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 96befef..b314c41 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -8422,9 +8422,7 @@ fi
 
 if $want_ipa ; then
    if $have_ipa ; then
-     # Needed because safe_strerror's definition is host-dependent
-     strerror_obj="`echo $srv_host_obs | sed 's/\(.*-strerror\)\.o/\1-ipa.o/'`"
-     IPA_DEPFILES="$ipa_obj $strerror_obj"
+     IPA_DEPFILES="$ipa_obj"
      extra_libraries="$extra_libraries libinproctrace.so"
    else
      as_fn_error "inprocess agent not supported for this target" "$LINENO" 5
diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
index 8aa85db..4ea7913 100644
--- a/gdb/gdbserver/configure.ac
+++ b/gdb/gdbserver/configure.ac
@@ -462,9 +462,7 @@ esac],
 
 if $want_ipa ; then
    if $have_ipa ; then
-     # Needed because safe_strerror's definition is host-dependent
-     strerror_obj="`echo $srv_host_obs | sed 's/\(.*-strerror\)\.o/\1-ipa.o/'`"
-     IPA_DEPFILES="$ipa_obj $strerror_obj"
+     IPA_DEPFILES="$ipa_obj"
      extra_libraries="$extra_libraries libinproctrace.so"
    else
      AC_MSG_ERROR([inprocess agent not supported for this target])
diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
index 056ac27..43f90c9 100644
--- a/gdb/gdbserver/configure.srv
+++ b/gdb/gdbserver/configure.srv
@@ -43,7 +43,7 @@ srv_amd64_linux_xmlfiles="i386/amd64-linux.xml i386/amd64-avx-linux.xml i386/amd
 
 # Linux object files.  This is so we don't have to repeat
 # these files over and over again.
-srv_linux_obj="linux-low.o linux-osdata.o linux-procfs.o linux-ptrace.o linux-waitpid.o linux-personality.o linux-namespaces.o fork-inferior.o"
+srv_linux_obj="linux-low.o linux-osdata.o linux-procfs.o linux-ptrace.o linux-waitpid.o linux-personality.o linux-namespaces.o fork-child.o fork-inferior.o"
 
 # Input is taken from the "${target}" variable.
 
@@ -131,7 +131,7 @@ case "${target}" in
 			ipa_obj="${ipa_i386_linux_regobj} linux-i386-ipa.o"
 			;;
   i[34567]86-*-lynxos*)	srv_regobj="i386.o"
-			srv_tgtobj="lynx-low.o lynx-i386-low.o fork-inferior.o"
+			srv_tgtobj="lynx-low.o lynx-i386-low.o fork-child.o fork-inferior.o"
 			srv_xmlfiles="i386/i386.xml"
 			srv_xmlfiles="${srv_xmlfiles} i386/32bit-core.xml"
 			srv_xmlfiles="${srv_xmlfiles} i386/32bit-sse.xml"
@@ -338,7 +338,7 @@ case "${target}" in
 			srv_linux_thread_db=yes
 			;;
   spu*-*-*)		srv_regobj=reg-spu.o
-			srv_tgtobj="spu-low.o fork-inferior.o"
+			srv_tgtobj="spu-low.o fork-child.o fork-inferior.o"
 			;;
   tic6x-*-uclinux)	srv_regobj="tic6x-c64xp-linux.o"
 			srv_regobj="${srv_regobj} tic6x-c64x-linux.o"
diff --git a/gdb/gdbserver/fork-child.c b/gdb/gdbserver/fork-child.c
new file mode 100644
index 0000000..a1a8ff1
--- /dev/null
+++ b/gdb/gdbserver/fork-child.c
@@ -0,0 +1,113 @@
+/* Fork a Unix child process, and set up to debug it, for GDBserver.
+   Copyright (C) 1989-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/>.  */
+
+#include "server.h"
+#include "job-control.h"
+#include "nat/fork-inferior.h"
+
+#ifdef SIGTTOU
+/* A file descriptor for the controlling terminal.  */
+static int terminal_fd;
+
+/* TERMINAL_FD's original foreground group.  */
+static pid_t old_foreground_pgrp;
+
+/* Hand back terminal ownership to the original foreground group.  */
+
+static void
+restore_old_foreground_pgrp (void)
+{
+  tcsetpgrp (terminal_fd, old_foreground_pgrp);
+}
+#endif
+
+/* See nat/fork-inferior.h.  */
+
+void
+prefork_hook (const char *args)
+{
+  if (debug_threads)
+    {
+      debug_printf ("args: %s\n", args);
+      debug_flush ();
+    }
+
+#ifdef SIGTTOU
+  signal (SIGTTOU, SIG_DFL);
+  signal (SIGTTIN, SIG_DFL);
+#endif
+
+  /* Clear this so the backend doesn't get confused, thinking
+     CONT_THREAD died, and it needs to resume all threads.  */
+  cont_thread = null_ptid;
+}
+
+/* See nat/fork-inferior.h.  */
+
+void
+postfork_hook (pid_t pid)
+{
+}
+
+/* See nat/fork-inferior.h.  */
+
+void
+postfork_child_hook ()
+{
+  /* 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;
+
+  debug_setpgrp = gdb_setpgid ();
+  if (debug_setpgrp == -1)
+    perror (_("setpgrp failed in child"));
+}
+
+/* See nat/fork-inferior.h.  */
+
+void
+gdb_flush_out_err ()
+{
+  fflush (stdout);
+  fflush (stderr);
+}
+
+/* See server.h.  */
+
+void
+post_fork_inferior (int pid, const char *program)
+{
+#ifdef SIGTTOU
+  signal (SIGTTOU, SIG_IGN);
+  signal (SIGTTIN, SIG_IGN);
+  terminal_fd = fileno (stderr);
+  old_foreground_pgrp = tcgetpgrp (terminal_fd);
+  tcsetpgrp (terminal_fd, pid);
+  atexit (restore_old_foreground_pgrp);
+#endif
+
+  startup_inferior (pid, START_INFERIOR_TRAPS_EXPECTED,
+		    &last_status, &last_ptid);
+  current_thread->last_resume_kind = resume_stop;
+  current_thread->last_status = last_status;
+  signal_pid = pid;
+  target_post_create_inferior ();
+  fprintf (stderr, "Process %s created; pid = %d\n", program, pid);
+  fflush (stderr);
+}
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index ab6670b..7fbf744 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1009,7 +1009,7 @@ linux_create_inferior (const char *program,
   new_lwp = add_lwp (ptid);
   new_lwp->must_set_ptrace_flags = 1;
 
-  post_fork_inferior (pid, program, startup_inferior);
+  post_fork_inferior (pid, program);
 
   return pid;
 }
@@ -6058,8 +6058,6 @@ linux_look_up_symbols (void)
 static void
 linux_request_interrupt (void)
 {
-  extern unsigned long signal_pid;
-
   /* Send a SIGINT to the process group.  This acts just like the user
      typed a ^C on the controlling terminal.  */
   kill (-signal_pid, SIGINT);
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 382d71c..35160d6 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -262,7 +262,7 @@ lynx_create_inferior (const char *program,
 		       environ_vector (get_environ ()), lynx_ptrace_fun,
 		       NULL, NULL, NULL, NULL);
 
-  post_fork_inferior (pid, program, startup_inferior);
+  post_fork_inferior (pid, program);
 
   lynx_add_process (pid, 0);
   /* Do not add the process thread just yet, as we do not know its tid.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 5a76927..3ab042d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -37,7 +37,6 @@
 #include "hostio.h"
 #include <vector>
 #include "common-inferior.h"
-#include "nat/fork-inferior.h"
 #include "job-control.h"
 #include "environ.h"
 
@@ -108,22 +107,6 @@ int program_signals_p;
 
 unsigned long signal_pid;
 
-#ifdef SIGTTOU
-/* A file descriptor for the controlling terminal.  */
-int terminal_fd;
-
-/* TERMINAL_FD's original foreground group.  */
-pid_t old_foreground_pgrp;
-
-/* Hand back terminal ownership to the original foreground group.  */
-
-static void
-restore_old_foreground_pgrp (void)
-{
-  tcsetpgrp (terminal_fd, old_foreground_pgrp);
-}
-#endif
-
 /* Set if you want to disable optional thread related packets support
    in gdbserver, for the sake of testing GDB against stubs that don't
    support them.  */
@@ -133,8 +116,8 @@ int disable_packet_qC;
 int disable_packet_qfThreadInfo;
 
 /* Last status reported to GDB.  */
-static struct target_waitstatus last_status;
-static ptid_t last_ptid;
+struct target_waitstatus last_status;
+ptid_t last_ptid;
 
 char *own_buf;
 static unsigned char *mem_buf;
@@ -258,7 +241,7 @@ target_running (void)
 const char *
 get_exec_wrapper ()
 {
-  return wrapper_argv.size () > 0 ? wrapper_argv.c_str () : NULL;
+  return !wrapper_argv.empty () ? wrapper_argv.c_str () : NULL;
 }
 
 /* See common/common-inferior.h.  */
@@ -280,81 +263,6 @@ get_environ ()
   return our_environ;
 }
 
-/* See nat/fork-inferior.h.  */
-
-void
-prefork_hook (const char *args)
-{
-  if (debug_threads)
-    {
-      debug_printf ("args: %s\n", args);
-      debug_flush ();
-    }
-
-#ifdef SIGTTOU
-  signal (SIGTTOU, SIG_DFL);
-  signal (SIGTTIN, SIG_DFL);
-#endif
-
-  /* Clear this so the backend doesn't get confused, thinking
-     CONT_THREAD died, and it needs to resume all threads.  */
-  cont_thread = null_ptid;
-}
-
-/* See server.h.  */
-
-void
-post_fork_inferior (int pid, const char *program,
-		    ptid_t (*my_startup_inferior) (pid_t pid, int ntraps,
-						   struct target_waitstatus
-						   *mystatus,
-						   ptid_t *myptid))
-
-{
-  /* Number of traps to be expected by startup_inferior.  We always
-     expect at least one trap for the main executable.  */
-  int num_traps = START_INFERIOR_TRAPS_EXPECTED;
-
-#ifdef SIGTTOU
-  signal (SIGTTOU, SIG_IGN);
-  signal (SIGTTIN, SIG_IGN);
-  terminal_fd = fileno (stderr);
-  old_foreground_pgrp = tcgetpgrp (terminal_fd);
-  tcsetpgrp (terminal_fd, pid);
-  atexit (restore_old_foreground_pgrp);
-#endif
-
-  my_startup_inferior (pid, num_traps, &last_status, &last_ptid);
-  current_thread->last_resume_kind = resume_stop;
-  current_thread->last_status = last_status;
-  signal_pid = pid;
-  target_post_create_inferior ();
-  fprintf (stderr, "Process %s created; pid = %d\n", program, pid);
-  fflush (stderr);
-}
-
-/* See nat/fork-inferior.h.  */
-
-void
-postfork_hook (pid_t pid)
-{
-}
-
-/* See nat/fork-inferior.h.  */
-
-void
-postfork_child_hook ()
-{
-  /* 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;
-
-  debug_setpgrp = gdb_setpgid ();
-  if (debug_setpgrp == -1)
-    perror (_("setpgrp failed in child"));
-}
-
 static int
 attach_inferior (int pid)
 {
@@ -3612,7 +3520,7 @@ captured_main (int argc, char *argv[])
 	      next_arg++;
 	    }
 
-	  if (wrapper_argv.size () > 0)
+	  if (!wrapper_argv.empty ())
 	    {
 	      /* Erase the last whitespace.  */
 	      wrapper_argv.erase (wrapper_argv.end () - 1);
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 96566fe..4de4244 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -149,19 +149,17 @@ extern int in_queued_stop_replies (ptid_t ptid);
 #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, PROGRAM is its name, and
-   MY_STARTUP_INFERIOR is the function that should be called to start
-   the inferior and consume its first events.  In most cases, this
-   function should be "startup_inferior" itself.  */
-extern void post_fork_inferior (int pid, const char *program,
-				ptid_t (*my_startup_inferior)
-				(pid_t pid, int ntraps,
-				 struct target_waitstatus *mystatus,
-				 ptid_t *myptid));
+   signals and call startup_inferior to start the inferior and consume
+   its first events.  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 ();
 
+extern target_waitstatus last_status;
+extern ptid_t last_ptid;
+extern unsigned long signal_pid;
+
 #endif /* SERVER_H */
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index c71e8ba..0f770a0 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -27,7 +27,6 @@
 #include <sys/syscall.h>
 #include "filestuff.h"
 #include "hostio.h"
-#include "common-inferior.h"
 #include "nat/fork-inferior.h"
 
 /* Some older glibc versions do not define this.  */
@@ -293,7 +292,7 @@ spu_create_inferior (const char *program,
 		       environ_vector (get_environ ()), spu_ptrace_fun,
 		       NULL, NULL, NULL, NULL);
 
-  post_fork_inferior (pid, program, startup_inferior);
+  post_fork_inferior (pid, program);
 
   proc = add_process (pid, 0);
   proc->tdesc = tdesc_spu;
diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
index 3f96a6c..307d15a 100644
--- a/gdb/gdbserver/utils.c
+++ b/gdb/gdbserver/utils.c
@@ -137,12 +137,3 @@ pfildes (gdb_fildes_t fd)
   return plongest (fd);
 #endif
 }
-
-/* See common/common-utils.h.  */
-
-void
-gdb_flush_out_err ()
-{
-  fflush (stdout);
-  fflush (stderr);
-}
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 2a01dd2..af181f0 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -96,7 +96,7 @@ inf_ptrace_create_inferior (struct target_ops *ops,
 			    char **env, int from_tty)
 {
   pid_t pid;
-  ptid_t ptid;  
+  ptid_t ptid;
 
   /* Do not change either targets above or the same target if already present.
      The reason is the target stack is shared across multiple inferiors.  */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index cdaeb11..6376952 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -45,7 +45,6 @@ struct inferior;
 
 #include "progspace.h"
 #include "registry.h"
-#include "common-inferior.h"
 
 #include "symfile-add-flags.h"
 #include "common/refcounted-object.h"
diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 7fd93ef..0913409 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -358,9 +358,11 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
      state, this doesn't work.  Also note that the vfork(2) call might
      actually be a call to fork(2) due to the fact that autoconf will
      ``#define vfork fork'' on certain platforms.  */
+#if !(defined(__UCLIBC__) && defined(HAS_NOMMU))
   if (pre_trace_fun || debug_fork)
     pid = fork ();
   else
+#endif
     pid = vfork ();
 
   if (pid < 0)
@@ -567,3 +569,27 @@ startup_inferior (pid_t pid, int ntraps,
 
   return resume_ptid;
 }
+
+/* See nat/fork-inferior.h.  */
+
+void
+trace_start_error (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  warning ("Could not trace the inferior process.\nError: ");
+  vwarning (fmt, ap);
+  va_end (ap);
+
+  gdb_flush_out_err ();
+  _exit (0177);
+}
+
+/* See nat/fork-inferior.h.  */
+
+void
+trace_start_error_with_name (const char *string)
+{
+  trace_start_error ("%s: %s", string, safe_strerror (errno));
+}
diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
index 39bdd8d..10e3832 100644
--- a/gdb/nat/fork-inferior.h
+++ b/gdb/nat/fork-inferior.h
@@ -87,4 +87,20 @@ extern void postfork_hook (pid_t pid);
    place.  This function is mainly used by fork_inferior.  */
 extern void postfork_child_hook ();
 
+/* Flush both stdout and stderr.  This function needs to be
+   implemented differently on GDB and GDBserver.  */
+extern void gdb_flush_out_err ();
+
+/* Report an error that happened when starting to trace the inferior
+   (i.e., when the "traceme_fun" callback is called on fork_inferior)
+   and bail out.  This function does not return.  */
+extern void trace_start_error (const char *fmt, ...)
+  ATTRIBUTE_NORETURN;
+
+/* 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)
+  ATTRIBUTE_NORETURN;
+
 #endif /* ! FORK_INFERIOR_H */
diff --git a/gdb/utils.c b/gdb/utils.c
index bd0a0d8..c61557e 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3391,15 +3391,6 @@ strip_leading_path_elements (const char *path, int n)
   return p;
 }
 
-/* See common/common-utils.h.  */
-
-void
-gdb_flush_out_err ()
-{
-  gdb_flush (main_ui->m_gdb_stdout);
-  gdb_flush (main_ui->m_gdb_stderr);
-}
-
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_utils;
 
-- 
2.5.5