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

Pedro Alves
2017-06-07T12:29:12Z

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