[PATCH v2 2/3] Pass inferior down to target_detach and to_detach

[PATCH v2 2/3] Pass inferior down to target_detach and to_detach

[Simon Marchi]

From: Simon Marchi <simon.marchi@ericsson.com>

New in v2:

- Add assert in target_detach.

The to_detach target_ops method implementations are currently expected
to work on current_inferior/inferior_ptid.  In order to make things more
explicit, and remove some "shadow" parameter passing through globals,
this patch adds an "inferior" parameter to to_detach.  Implementations
will be expected to use this instead of relying on the global.  However,
to keep things simple, this patch only does the minimum that is
necessary to add the parameter.  The following patch gives an example of
how one such implementation would be adapted.  If the approach is deemed
good, we can then look into adapting more implementations.  Until then,
they'll continue to work as they do currently.

gdb/ChangeLog:

	* target.h (struct target_ops) <to_detach>: Add inferior
	parameter.
	(target_detach): Likewise.
	* target.c (dispose_inferior): Pass inferior down.
	(target_detach): Pass inferior down.  Assert that it is equal to
	the current inferior.
	* aix-thread.c (aix_thread_detach): Pass inferior down.
	* corefile.c (core_file_command): Pass current_inferior() down.
	* corelow.c (core_detach): Add inferior parameter.
	* darwin-nat.c (darwin_detach): Likewise.
	* gnu-nat.c (gnu_detach): Likewise.
	* inf-ptrace.c (inf_ptrace_detach): Likewise.
	* infcmd.c (detach_command): Pass current_inferior() down to
	target_detach.
	* infrun.c (follow_fork_inferior): Pass parent_inf to
	target_detach.
	(handle_vfork_child_exec_or_exit): Pass inf->vfork_parent to
	target_detach.
	* linux-nat.c (linux_nat_detach): Add inferior parameter.
	* linux-thread-db.c (thread_db_detach): Likewise.
	* nto-procfs.c (procfs_detach): Likewise.
	* procfs.c (procfs_detach): Likewise.
	* record.c (record_detach): Likewise.
	* record.h (struct inferior): Forward-declare.
	(record_detach): Add inferior parameter.
	* remote-sim.c (gdbsim_detach): Likewise.
	* remote.c (remote_detach_1): Likewise.
	(remote_detach): Likewise.
	(extended_remote_detach): Likewise.
	* sol-thread.c (sol_thread_detach): Likewise.
	* target-debug.h (target_debug_print_inferior_p): New macro.
	* target-delegates.c: Re-generate.
	* top.c (kill_or_detach): Pass inferior down to target_detach.
	* windows-nat.c (windows_detach): Add inferior parameter.
---
 gdb/aix-thread.c       |  4 ++--
 gdb/corefile.c         |  2 +-
 gdb/corelow.c          |  2 +-
 gdb/darwin-nat.c       |  3 +--
 gdb/gnu-nat.c          |  2 +-
 gdb/inf-ptrace.c       |  2 +-
 gdb/infcmd.c           |  2 +-
 gdb/infrun.c           |  4 ++--
 gdb/linux-nat.c        |  2 +-
 gdb/linux-thread-db.c  |  4 ++--
 gdb/nto-procfs.c       |  2 +-
 gdb/procfs.c           |  2 +-
 gdb/record.c           |  4 ++--
 gdb/record.h           |  3 ++-
 gdb/remote-sim.c       |  4 ++--
 gdb/remote.c           | 10 +++++-----
 gdb/sol-thread.c       |  4 ++--
 gdb/target-debug.h     |  2 ++
 gdb/target-delegates.c | 14 ++++++++------
 gdb/target.c           | 13 ++++++++++---
 gdb/target.h           |  4 ++--
 gdb/top.c              |  2 +-
 gdb/windows-nat.c      |  2 +-
 23 files changed, 52 insertions(+), 41 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 0f0404f013..09d5175d90 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -981,12 +981,12 @@ aix_thread_inferior_created (struct target_ops *ops, int from_tty)
 /* Detach from the process attached to by aix_thread_attach().  */
 
 static void
-aix_thread_detach (struct target_ops *ops, int from_tty)
+aix_thread_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   struct target_ops *beneath = find_target_beneath (ops);
 
   pd_disable ();
-  beneath->to_detach (beneath, from_tty);
+  beneath->to_detach (beneath, inf, from_tty);
 }
 
 /* Tell the inferior process to continue running thread PID if != -1
diff --git a/gdb/corefile.c b/gdb/corefile.c
index c5e4f91a07..abf9ea675d 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -68,7 +68,7 @@ core_file_command (const char *filename, int from_tty)
   gdb_assert (core_target != NULL);
 
   if (!filename)
-    (core_target->to_detach) (core_target, from_tty);
+    (core_target->to_detach) (core_target, current_inferior (), from_tty);
   else
     (core_target->to_open) (filename, from_tty);
 }
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 250f46aa05..190f0162d9 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -464,7 +464,7 @@ core_open (const char *arg, int from_tty)
 }
 
 static void
-core_detach (struct target_ops *ops, int from_tty)
+core_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   unpush_target (ops);
   reinit_frame_cache ();
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 1441a6ad6d..7a862cdeae 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1938,10 +1938,9 @@ darwin_attach (struct target_ops *ops, const char *args, int from_tty)
    previously attached.  It *might* work if the program was
    started via fork.  */
 static void
-darwin_detach (struct target_ops *ops, int from_tty)
+darwin_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
-  struct inferior *inf = current_inferior ();
   darwin_inferior *priv = get_darwin_inferior (inf);
   kern_return_t kret;
   int res;
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 0debb02ee6..8271565a04 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2255,7 +2255,7 @@ gnu_attach (struct target_ops *ops, const char *args, int from_tty)
    previously attached.  It *might* work if the program was
    started via fork.  */
 static void
-gnu_detach (struct target_ops *ops, int from_tty)
+gnu_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   int pid;
 
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 44216c45d6..aa8b17f41a 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -244,7 +244,7 @@ inf_ptrace_post_attach (struct target_ops *self, int pid)
 /* Detach from the inferior.  If FROM_TTY is non-zero, be chatty about it.  */
 
 static void
-inf_ptrace_detach (struct target_ops *ops, int from_tty)
+inf_ptrace_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 8865244fd9..4ad37aa275 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2976,7 +2976,7 @@ detach_command (const char *args, int from_tty)
 
   disconnect_tracing ();
 
-  target_detach (from_tty);
+  target_detach (current_inferior (), from_tty);
 
   /* The current inferior process was just detached successfully.  Get
      rid of breakpoints that no longer make sense.  Note we don't do
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d240208172..45fe36a717 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -606,7 +606,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 				target_pid_to_str (process_ptid));
 	    }
 
-	  target_detach (0);
+	  target_detach (parent_inf, 0);
 	}
 
       /* Note that the detach above makes PARENT_INF dangling.  */
@@ -976,7 +976,7 @@ handle_vfork_child_exec_or_exit (int exec)
 		}
 	    }
 
-	  target_detach (0);
+	  target_detach (inf->vfork_parent, 0);
 
 	  /* Put it back.  */
 	  inf->pspace = pspace;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 99d12f2526..70f4c084dc 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1497,7 +1497,7 @@ detach_callback (struct lwp_info *lp, void *data)
 }
 
 static void
-linux_nat_detach (struct target_ops *ops, int from_tty)
+linux_nat_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   int pid;
   struct lwp_info *main_lwp;
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 9dcfc0e98b..873f929144 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1090,13 +1090,13 @@ record_thread (struct thread_db_info *info,
 }
 
 static void
-thread_db_detach (struct target_ops *ops, int from_tty)
+thread_db_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   struct target_ops *target_beneath = find_target_beneath (ops);
 
   delete_thread_db_info (ptid_get_pid (inferior_ptid));
 
-  target_beneath->to_detach (target_beneath, from_tty);
+  target_beneath->to_detach (target_beneath, inf, from_tty);
 
   /* NOTE: From this point on, inferior_ptid is null_ptid.  */
 
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index 2fd2dcf79a..6408c21799 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -943,7 +943,7 @@ procfs_xfer_partial (struct target_ops *ops, enum target_object object,
    on signals, etc.  We'd better not have left any breakpoints
    in the program or it'll die when it hits one.  */
 static void
-procfs_detach (struct target_ops *ops, int from_tty)
+procfs_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   int pid;
 
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 491970a787..2d5ec65c9b 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -1924,7 +1924,7 @@ procfs_attach (struct target_ops *ops, const char *args, int from_tty)
 }
 
 static void
-procfs_detach (struct target_ops *ops, int from_tty)
+procfs_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   int pid = ptid_get_pid (inferior_ptid);
 
diff --git a/gdb/record.c b/gdb/record.c
index 74a14dc07b..9a476142e9 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -187,7 +187,7 @@ record_disconnect (struct target_ops *t, const char *args, int from_tty)
 /* See record.h.  */
 
 void
-record_detach (struct target_ops *t, int from_tty)
+record_detach (struct target_ops *t, inferior *inf, int from_tty)
 {
   gdb_assert (t->to_stratum == record_stratum);
 
@@ -196,7 +196,7 @@ record_detach (struct target_ops *t, int from_tty)
   record_stop (t);
   record_unpush (t);
 
-  target_detach (from_tty);
+  target_detach (inf, from_tty);
 }
 
 /* See record.h.  */
diff --git a/gdb/record.h b/gdb/record.h
index a0ca84dff3..21fafeba82 100644
--- a/gdb/record.h
+++ b/gdb/record.h
@@ -24,6 +24,7 @@
 #include "common/enum-flags.h"
 
 struct cmd_list_element;
+struct inferior;
 
 extern unsigned int record_debug;
 
@@ -88,7 +89,7 @@ extern void record_goto (const char *arg);
 extern void record_disconnect (struct target_ops *, const char *, int);
 
 /* The default "to_detach" target method for record targets.  */
-extern void record_detach (struct target_ops *, int);
+extern void record_detach (struct target_ops *, inferior *, int);
 
 /* The default "to_mourn_inferior" target method for record targets.  */
 extern void record_mourn_inferior (struct target_ops *);
diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index edc70d5b3e..aed7a34b42 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -77,7 +77,7 @@ static void gdbsim_open (const char *args, int from_tty);
 
 static void gdbsim_close (struct target_ops *self);
 
-static void gdbsim_detach (struct target_ops *ops, int from_tty);
+static void gdbsim_detach (struct target_ops *ops, inferior *inf, int from_tty);
 
 static void gdbsim_prepare_to_store (struct target_ops *self,
 				     struct regcache *regcache);
@@ -820,7 +820,7 @@ gdbsim_close (struct target_ops *self)
    Use this when you want to detach and do something else with your gdb.  */
 
 static void
-gdbsim_detach (struct target_ops *ops, int from_tty)
+gdbsim_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   if (remote_debug)
     fprintf_unfiltered (gdb_stdlog, "gdbsim_detach\n");
diff --git a/gdb/remote.c b/gdb/remote.c
index 01b236fcd3..5ac84df0a0 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5126,7 +5126,7 @@ remote_detach_pid (int pid)
    one.  */
 
 static void
-remote_detach_1 (int from_tty)
+remote_detach_1 (int from_tty, inferior *inf)
 {
   int pid = ptid_get_pid (inferior_ptid);
   struct remote_state *rs = get_remote_state ();
@@ -5162,15 +5162,15 @@ remote_detach_1 (int from_tty)
 }
 
 static void
-remote_detach (struct target_ops *ops, int from_tty)
+remote_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
-  remote_detach_1 (from_tty);
+  remote_detach_1 (from_tty, inf);
 }
 
 static void
-extended_remote_detach (struct target_ops *ops, int from_tty)
+extended_remote_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
-  remote_detach_1 (from_tty);
+  remote_detach_1 (from_tty, inf);
 }
 
 /* Target follow-fork function for remote targets.  On entry, and
diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index e724c7e0fa..2c7dd0e15d 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -348,14 +348,14 @@ lwp_to_thread (ptid_t lwp)
    program was started via the normal ptrace (PTRACE_TRACEME).  */
 
 static void
-sol_thread_detach (struct target_ops *ops, int from_tty)
+sol_thread_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   struct target_ops *beneath = find_target_beneath (ops);
 
   sol_thread_active = 0;
   inferior_ptid = pid_to_ptid (ptid_get_pid (main_ph.ptid));
   unpush_target (ops);
-  beneath->to_detach (beneath, from_tty);
+  beneath->to_detach (beneath, inf, from_tty);
 }
 
 /* Resume execution of process PTID.  If STEP is nozero, then just
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index d7d99eff5d..d7fc001975 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -170,6 +170,8 @@
   target_debug_do_print (host_address_to_string (X.get ()))
 #define target_debug_print_gdb_array_view_const_int(X)	\
   target_debug_do_print (host_address_to_string (X.data ()))
+#define target_debug_print_inferior_p(inf) \
+  target_debug_do_print (host_address_to_string (inf))
 
 static void
 target_debug_print_struct_target_waitstatus_p (struct target_waitstatus *status)
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 6bb7d7fba7..bc847916b4 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -28,26 +28,28 @@ debug_post_attach (struct target_ops *self, int arg1)
 }
 
 static void
-delegate_detach (struct target_ops *self, int arg1)
+delegate_detach (struct target_ops *self, inferior *arg1, int arg2)
 {
   self = self->beneath;
-  self->to_detach (self, arg1);
+  self->to_detach (self, arg1, arg2);
 }
 
 static void
-tdefault_detach (struct target_ops *self, int arg1)
+tdefault_detach (struct target_ops *self, inferior *arg1, int arg2)
 {
 }
 
 static void
-debug_detach (struct target_ops *self, int arg1)
+debug_detach (struct target_ops *self, inferior *arg1, int arg2)
 {
   fprintf_unfiltered (gdb_stdlog, "-> %s->to_detach (...)\n", debug_target.to_shortname);
-  debug_target.to_detach (&debug_target, arg1);
+  debug_target.to_detach (&debug_target, arg1, arg2);
   fprintf_unfiltered (gdb_stdlog, "<- %s->to_detach (", debug_target.to_shortname);
   target_debug_print_struct_target_ops_p (&debug_target);
   fputs_unfiltered (", ", gdb_stdlog);
-  target_debug_print_int (arg1);
+  target_debug_print_inferior_p (arg1);
+  fputs_unfiltered (", ", gdb_stdlog);
+  target_debug_print_int (arg2);
   fputs_unfiltered (")\n", gdb_stdlog);
 }
 
diff --git a/gdb/target.c b/gdb/target.c
index b01e1ac645..ce630f43de 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2108,7 +2108,7 @@ dispose_inferior (struct inferior *inf, void *args)
       if (target_has_execution)
 	target_kill ();
       else
-	target_detach (0);
+	target_detach (inf, 0);
     }
 
   return 0;
@@ -2144,8 +2144,15 @@ target_preopen (int from_tty)
 /* See target.h.  */
 
 void
-target_detach (int from_tty)
+target_detach (inferior *inf, int from_tty)
 {
+  /* As long as some to_detach implementations rely on the current_inferior
+     (either directly, or indirectly, like through target_gdbarch or by
+     reading memory), INF needs to be the current inferior.  When that
+     requirement will become no longer true, then we can remove this
+     assertion.  */
+  gdb_assert (inf == current_inferior ());
+
   if (gdbarch_has_global_breakpoints (target_gdbarch ()))
     /* Don't remove global breakpoints here.  They're removed on
        disconnection from the target.  */
@@ -2157,7 +2164,7 @@ target_detach (int from_tty)
 
   prepare_for_detach ();
 
-  current_target.to_detach (&current_target, from_tty);
+  current_target.to_detach (&current_target, inf, from_tty);
 }
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 51841a1cbb..52361baa36 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -440,7 +440,7 @@ struct target_ops
     void (*to_attach) (struct target_ops *ops, const char *, int);
     void (*to_post_attach) (struct target_ops *, int)
       TARGET_DEFAULT_IGNORE ();
-    void (*to_detach) (struct target_ops *ops, int)
+    void (*to_detach) (struct target_ops *ops, inferior *, int)
       TARGET_DEFAULT_IGNORE ();
     void (*to_disconnect) (struct target_ops *, const char *, int)
       TARGET_DEFAULT_NORETURN (tcomplain ());
@@ -1326,7 +1326,7 @@ extern void target_announce_detach (int from_tty);
    in the program or it'll die when it hits one.  FROM_TTY says whether to be
    verbose or not.  */
 
-extern void target_detach (int from_tty);
+extern void target_detach (inferior *inf, int from_tty);
 
 /* Disconnect from the current target without resuming it (leaving it
    waiting for a debugger).  */
diff --git a/gdb/top.c b/gdb/top.c
index 1e6d5137b8..a6166bd280 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1485,7 +1485,7 @@ kill_or_detach (struct inferior *inf, void *args)
       if (target_has_execution)
 	{
 	  if (inf->attach_flag)
-	    target_detach (qt->from_tty);
+	    target_detach (inf, qt->from_tty);
 	  else
 	    target_kill ();
 	}
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 4023089a4d..5fb143e00d 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1931,7 +1931,7 @@ windows_attach (struct target_ops *ops, const char *args, int from_tty)
 }
 
 static void
-windows_detach (struct target_ops *ops, int from_tty)
+windows_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   int detached = 1;
 
-- 
2.15.1

[PATCH v2 1/3] Remove args from target detach

[Simon Marchi]

From: Simon Marchi <simon.marchi@ericsson.com>

New in v2:

- Remove signo parameter from do_detach in procfs.c.

I was looking into adding a parameter to target_detach, and was
wondering what the args parameter was.  It seems like in the distant
past, it was possible to specify a signal number when detaching.  That
signal was injected in the process before it was detached.  There is an
example of code handling this in linux_nat_detach.  With today's GDB, I
can't get this to work.  Doing "detach 15" (15 == SIGTERM) doesn't work,
because detach is a prefix command and doesn't recognize the sub-command
15.  Doing "detach inferiors 15" doesn't work because it expects a list
of inferior id to detach.  Therefore, I don't think there's a way of
invoking detach_command with a non-NULL args.  I also didn't find any
documentation related to this feature.

I assume that this feature stopped working when detach was made a prefix
command, which is in f73adfeb8bae36885e6ea248d12223ab0d5eb9cb (sorry,
there's no commit title) from 2006.  Given that this feature was broken
for such a long time and we haven't heard anything (AFAIK, I did not
find any related bug), I think it's safe to remove it, as well as the
args parameter to target_detach.  If someone wants to re-introduce it, I
would suggest rethinking the user interface, and in particular would
suggest using signal name instead of numbers.

I tried to fix all the impacted code, but I might have forgotten some
spots.  It shouldn't be hard to fix if that's the case.  I also couldn't
build-test everything I changed, especially the nto and solaris stuff.

gdb/ChangeLog:

	* target.h (struct target_ops) <to_detach>: Remove args
	parameter.
	(target_detach): Likewise.
	* target.c (dispose_inferior): Adjust.
	(target_detach): Remove args parameter, adjust.
	* aix-thread.c (aix_thread_detach): Adjust.
	* corefile.c (core_file_command): Adjust.
	* corelow.c (core_detach): Adjust.
	* darwin-nat.c (darwin_detach): Adjust.
	* gnu-nat.c (gnu_detach): Adjust.
	* inf-ptrace.c (inf_ptrace_detach): Adjust.
	* infcmd.c (detach_command): Adjust
	* infrun.c (follow_fork_inferior): Adjust.
	(handle_vfork_child_exec_or_exit): Adjust.
	* linux-fork.c (linux_fork_detach): Remove args parameter.
	* linux-fork.h (linux_fork_detach): Likewise.
	* linux-nat.c (linux_nat_detach): Likewise, and adjust.
	* linux-thread-db.c (thread_db_detach): Likewise.
	* nto-procfs.c (procfs_detach): Likewise.
	* procfs.c (procfs_detach): Likewise.
	(do_detach): Remove signo parameter.
	* record.c (record_detach): Remove args parameter.
	* record.h (record_detach): Likewise.
	* remote-sim.c (gdbsim_detach): Likewise.
	* remote.c (remote_detach_1): Likewise.
	(remote_detach): Likewise.
	(extended_remote_detach): Likewise.
	* sol-thread.c (sol_thread_detach): Likewise.
	* target-delegates.c: Re-generate.
	* top.c (struct qt_args) <args>: Remove field.
	(kill_or_detach): Don't pass args.
	(quit_force): Don't set args.
	* windows-nat.c (windows_detach): Remove args parameter.
---
 gdb/aix-thread.c       |  4 ++--
 gdb/corefile.c         |  2 +-
 gdb/corelow.c          |  4 +---
 gdb/darwin-nat.c       |  2 +-
 gdb/gnu-nat.c          |  2 +-
 gdb/inf-ptrace.c       | 10 +++-------
 gdb/infcmd.c           |  2 +-
 gdb/infrun.c           |  4 ++--
 gdb/linux-fork.c       |  2 +-
 gdb/linux-fork.h       |  2 +-
 gdb/linux-nat.c        | 15 ++++-----------
 gdb/linux-thread-db.c  |  4 ++--
 gdb/nto-procfs.c       |  8 ++------
 gdb/procfs.c           | 23 ++++++++---------------
 gdb/record.c           |  4 ++--
 gdb/record.h           |  2 +-
 gdb/remote-sim.c       | 12 +++++-------
 gdb/remote.c           | 13 +++++--------
 gdb/sol-thread.c       |  4 ++--
 gdb/target-delegates.c | 14 ++++++--------
 gdb/target.c           |  8 ++++----
 gdb/target.h           |  9 ++++-----
 gdb/top.c              |  4 +---
 gdb/windows-nat.c      |  2 +-
 24 files changed, 61 insertions(+), 95 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index f15845f7bf..0f0404f013 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -981,12 +981,12 @@ aix_thread_inferior_created (struct target_ops *ops, int from_tty)
 /* Detach from the process attached to by aix_thread_attach().  */
 
 static void
-aix_thread_detach (struct target_ops *ops, const char *args, int from_tty)
+aix_thread_detach (struct target_ops *ops, int from_tty)
 {
   struct target_ops *beneath = find_target_beneath (ops);
 
   pd_disable ();
-  beneath->to_detach (beneath, args, from_tty);
+  beneath->to_detach (beneath, from_tty);
 }
 
 /* Tell the inferior process to continue running thread PID if != -1
diff --git a/gdb/corefile.c b/gdb/corefile.c
index e39df42c63..c5e4f91a07 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -68,7 +68,7 @@ core_file_command (const char *filename, int from_tty)
   gdb_assert (core_target != NULL);
 
   if (!filename)
-    (core_target->to_detach) (core_target, filename, from_tty);
+    (core_target->to_detach) (core_target, from_tty);
   else
     (core_target->to_open) (filename, from_tty);
 }
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 3a5256cb17..250f46aa05 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -464,10 +464,8 @@ core_open (const char *arg, int from_tty)
 }
 
 static void
-core_detach (struct target_ops *ops, const char *args, int from_tty)
+core_detach (struct target_ops *ops, int from_tty)
 {
-  if (args)
-    error (_("Too many arguments"));
   unpush_target (ops);
   reinit_frame_cache ();
   if (from_tty)
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index a51d8b11d6..1441a6ad6d 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1938,7 +1938,7 @@ darwin_attach (struct target_ops *ops, const char *args, int from_tty)
    previously attached.  It *might* work if the program was
    started via fork.  */
 static void
-darwin_detach (struct target_ops *ops, const char *args, int from_tty)
+darwin_detach (struct target_ops *ops, int from_tty)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
   struct inferior *inf = current_inferior ();
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index f2d5fcaa50..0debb02ee6 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2255,7 +2255,7 @@ gnu_attach (struct target_ops *ops, const char *args, int from_tty)
    previously attached.  It *might* work if the program was
    started via fork.  */
 static void
-gnu_detach (struct target_ops *ops, const char *args, int from_tty)
+gnu_detach (struct target_ops *ops, int from_tty)
 {
   int pid;
 
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index c91f15a345..44216c45d6 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -241,18 +241,14 @@ inf_ptrace_post_attach (struct target_ops *self, int pid)
 
 #endif
 
-/* Detach from the inferior, optionally passing it the signal
-   specified by ARGS.  If FROM_TTY is non-zero, be chatty about it.  */
+/* Detach from the inferior.  If FROM_TTY is non-zero, be chatty about it.  */
 
 static void
-inf_ptrace_detach (struct target_ops *ops, const char *args, int from_tty)
+inf_ptrace_detach (struct target_ops *ops, int from_tty)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
-  int sig = 0;
 
   target_announce_detach (from_tty);
-  if (args)
-    sig = atoi (args);
 
 #ifdef PT_DETACH
   /* We'd better not have left any breakpoints in the program or it'll
@@ -260,7 +256,7 @@ inf_ptrace_detach (struct target_ops *ops, const char *args, int from_tty)
      previously attached to the inferior.  It *might* work if we
      started the process ourselves.  */
   errno = 0;
-  ptrace (PT_DETACH, pid, (PTRACE_TYPE_ARG3)1, sig);
+  ptrace (PT_DETACH, pid, (PTRACE_TYPE_ARG3)1, 0);
   if (errno != 0)
     perror_with_name (("ptrace"));
 #else
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 976276b694..8865244fd9 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2976,7 +2976,7 @@ detach_command (const char *args, int from_tty)
 
   disconnect_tracing ();
 
-  target_detach (args, from_tty);
+  target_detach (from_tty);
 
   /* The current inferior process was just detached successfully.  Get
      rid of breakpoints that no longer make sense.  Note we don't do
diff --git a/gdb/infrun.c b/gdb/infrun.c
index e1d11234e0..d240208172 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -606,7 +606,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 				target_pid_to_str (process_ptid));
 	    }
 
-	  target_detach (NULL, 0);
+	  target_detach (0);
 	}
 
       /* Note that the detach above makes PARENT_INF dangling.  */
@@ -976,7 +976,7 @@ handle_vfork_child_exec_or_exit (int exec)
 		}
 	    }
 
-	  target_detach (NULL, 0);
+	  target_detach (0);
 
 	  /* Put it back.  */
 	  inf->pspace = pspace;
diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 3b0e41a43a..8561157d3d 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -407,7 +407,7 @@ linux_fork_mourn_inferior (void)
    the first available.  */
 
 void
-linux_fork_detach (const char *args, int from_tty)
+linux_fork_detach (int from_tty)
 {
   /* OK, inferior_ptid is the one we are detaching from.  We need to
      delete it from the fork_list, and switch to the next available
diff --git a/gdb/linux-fork.h b/gdb/linux-fork.h
index 2c4f530503..b1e3ba3e4a 100644
--- a/gdb/linux-fork.h
+++ b/gdb/linux-fork.h
@@ -22,6 +22,6 @@ extern struct fork_info *add_fork (pid_t);
 extern struct fork_info *find_fork_pid (pid_t);
 extern void linux_fork_killall (void);
 extern void linux_fork_mourn_inferior (void);
-extern void linux_fork_detach (const char *, int);
+extern void linux_fork_detach (int);
 extern int forks_exist_p (void);
 extern int linux_fork_checkpointing_p (int);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index a47c49609b..99d12f2526 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1497,7 +1497,7 @@ detach_callback (struct lwp_info *lp, void *data)
 }
 
 static void
-linux_nat_detach (struct target_ops *ops, const char *args, int from_tty)
+linux_nat_detach (struct target_ops *ops, int from_tty)
 {
   int pid;
   struct lwp_info *main_lwp;
@@ -1527,21 +1527,14 @@ linux_nat_detach (struct target_ops *ops, const char *args, int from_tty)
 	 from, but there are other viable forks to debug.  Detach from
 	 the current fork, and context-switch to the first
 	 available.  */
-      linux_fork_detach (args, from_tty);
+      linux_fork_detach (from_tty);
     }
   else
     {
-      int signo;
-
       target_announce_detach (from_tty);
 
-      /* Pass on any pending signal for the last LWP, unless the user
-	 requested detaching with a different signal (most likely 0,
-	 meaning, discard the signal).  */
-      if (args != NULL)
-	signo = atoi (args);
-      else
-	signo = get_detach_signal (main_lwp);
+      /* Pass on any pending signal for the last LWP.  */
+      int signo = get_detach_signal (main_lwp);
 
       detach_one_lwp (main_lwp, &signo);
 
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 299c5ed34a..9dcfc0e98b 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1090,13 +1090,13 @@ record_thread (struct thread_db_info *info,
 }
 
 static void
-thread_db_detach (struct target_ops *ops, const char *args, int from_tty)
+thread_db_detach (struct target_ops *ops, int from_tty)
 {
   struct target_ops *target_beneath = find_target_beneath (ops);
 
   delete_thread_db_info (ptid_get_pid (inferior_ptid));
 
-  target_beneath->to_detach (target_beneath, args, from_tty);
+  target_beneath->to_detach (target_beneath, from_tty);
 
   /* NOTE: From this point on, inferior_ptid is null_ptid.  */
 
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index eab96b940c..2fd2dcf79a 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -943,18 +943,14 @@ procfs_xfer_partial (struct target_ops *ops, enum target_object object,
    on signals, etc.  We'd better not have left any breakpoints
    in the program or it'll die when it hits one.  */
 static void
-procfs_detach (struct target_ops *ops, const char *args, int from_tty)
+procfs_detach (struct target_ops *ops, int from_tty)
 {
-  int siggnal = 0;
   int pid;
 
   target_announce_detach ();
 
-  if (args)
-    siggnal = atoi (args);
-
   if (siggnal)
-    SignalKill (nto_node (), ptid_get_pid (inferior_ptid), 0, siggnal, 0, 0);
+    SignalKill (nto_node (), ptid_get_pid (inferior_ptid), 0, 0, 0, 0);
 
   close (ctl_fd);
   ctl_fd = -1;
diff --git a/gdb/procfs.c b/gdb/procfs.c
index a939dedb00..491970a787 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -1829,7 +1829,7 @@ proc_iterate_over_threads (procinfo *pi,
    friends.  */
 
 static ptid_t do_attach (ptid_t ptid);
-static void do_detach (int signo);
+static void do_detach ();
 static void proc_trace_syscalls_1 (procinfo *pi, int syscallnum,
 				   int entry_or_exit, int mode, int from_tty);
 
@@ -1924,14 +1924,10 @@ procfs_attach (struct target_ops *ops, const char *args, int from_tty)
 }
 
 static void
-procfs_detach (struct target_ops *ops, const char *args, int from_tty)
+procfs_detach (struct target_ops *ops, int from_tty)
 {
-  int sig = 0;
   int pid = ptid_get_pid (inferior_ptid);
 
-  if (args)
-    sig = atoi (args);
-
   if (from_tty)
     {
       const char *exec_file;
@@ -1945,7 +1941,7 @@ procfs_detach (struct target_ops *ops, const char *args, int from_tty)
       gdb_flush (gdb_stdout);
     }
 
-  do_detach (sig);
+  do_detach ();
 
   inferior_ptid = null_ptid;
   detach_inferior (pid);
@@ -2023,16 +2019,13 @@ do_attach (ptid_t ptid)
 }
 
 static void
-do_detach (int signo)
+do_detach ()
 {
   procinfo *pi;
 
   /* Find procinfo for the main process.  */
   pi = find_procinfo_or_die (ptid_get_pid (inferior_ptid),
 			     0); /* FIXME: threads */
-  if (signo)
-    if (!proc_set_current_signal (pi, signo))
-      proc_warn (pi, "do_detach, set_current_signal", __LINE__);
 
   if (!proc_set_traced_signals (pi, &pi->saved_sigset))
     proc_warn (pi, "do_detach, set_traced_signal", __LINE__);
@@ -2049,15 +2042,15 @@ do_detach (int signo)
   if (!proc_set_held_signals (pi, &pi->saved_sighold))
     proc_warn (pi, "do_detach, set_held_signals", __LINE__);
 
-  if (signo || (proc_flags (pi) & (PR_STOPPED | PR_ISTOP)))
-    if (signo || !(pi->was_stopped) ||
-	query (_("Was stopped when attached, make it runnable again? ")))
+  if (proc_flags (pi) & (PR_STOPPED | PR_ISTOP))
+    if (!(pi->was_stopped)
+	|| query (_("Was stopped when attached, make it runnable again? ")))
       {
 	/* Clear any pending signal.  */
 	if (!proc_clear_current_fault (pi))
 	  proc_warn (pi, "do_detach, clear_current_fault", __LINE__);
 
-	if (signo == 0 && !proc_clear_current_signal (pi))
+	if (!proc_clear_current_signal (pi))
 	  proc_warn (pi, "do_detach, clear_current_signal", __LINE__);
 
 	if (!proc_set_run_on_last_close (pi))
diff --git a/gdb/record.c b/gdb/record.c
index 767b9f6afd..74a14dc07b 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -187,7 +187,7 @@ record_disconnect (struct target_ops *t, const char *args, int from_tty)
 /* See record.h.  */
 
 void
-record_detach (struct target_ops *t, const char *args, int from_tty)
+record_detach (struct target_ops *t, int from_tty)
 {
   gdb_assert (t->to_stratum == record_stratum);
 
@@ -196,7 +196,7 @@ record_detach (struct target_ops *t, const char *args, int from_tty)
   record_stop (t);
   record_unpush (t);
 
-  target_detach (args, from_tty);
+  target_detach (from_tty);
 }
 
 /* See record.h.  */
diff --git a/gdb/record.h b/gdb/record.h
index 8dae4c6775..a0ca84dff3 100644
--- a/gdb/record.h
+++ b/gdb/record.h
@@ -88,7 +88,7 @@ extern void record_goto (const char *arg);
 extern void record_disconnect (struct target_ops *, const char *, int);
 
 /* The default "to_detach" target method for record targets.  */
-extern void record_detach (struct target_ops *, const char *, int);
+extern void record_detach (struct target_ops *, int);
 
 /* The default "to_mourn_inferior" target method for record targets.  */
 extern void record_mourn_inferior (struct target_ops *);
diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index fe36f10f52..edc70d5b3e 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -77,8 +77,7 @@ static void gdbsim_open (const char *args, int from_tty);
 
 static void gdbsim_close (struct target_ops *self);
 
-static void gdbsim_detach (struct target_ops *ops, const char *args,
-			   int from_tty);
+static void gdbsim_detach (struct target_ops *ops, int from_tty);
 
 static void gdbsim_prepare_to_store (struct target_ops *self,
 				     struct regcache *regcache);
@@ -815,17 +814,16 @@ gdbsim_close (struct target_ops *self)
 /* Takes a program previously attached to and detaches it.
    The program may resume execution (some targets do, some don't) and will
    no longer stop on signals, etc.  We better not have left any breakpoints
-   in the program or it'll die when it hits one.  ARGS is arguments
-   typed by the user (e.g. a signal to send the process).  FROM_TTY
-   says whether to be verbose or not.  */
+   in the program or it'll die when it hits one.  FROM_TTY says whether to be
+   verbose or not.  */
 /* Terminate the open connection to the remote debugger.
    Use this when you want to detach and do something else with your gdb.  */
 
 static void
-gdbsim_detach (struct target_ops *ops, const char *args, int from_tty)
+gdbsim_detach (struct target_ops *ops, int from_tty)
 {
   if (remote_debug)
-    fprintf_unfiltered (gdb_stdlog, "gdbsim_detach: args \"%s\"\n", args);
+    fprintf_unfiltered (gdb_stdlog, "gdbsim_detach\n");
 
   unpush_target (ops);		/* calls gdbsim_close to do the real work */
   if (from_tty)
diff --git a/gdb/remote.c b/gdb/remote.c
index a1cd9ae1df..01b236fcd3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5126,16 +5126,13 @@ remote_detach_pid (int pid)
    one.  */
 
 static void
-remote_detach_1 (const char *args, int from_tty)
+remote_detach_1 (int from_tty)
 {
   int pid = ptid_get_pid (inferior_ptid);
   struct remote_state *rs = get_remote_state ();
   struct thread_info *tp = find_thread_ptid (inferior_ptid);
   int is_fork_parent;
 
-  if (args)
-    error (_("Argument given to \"detach\" when remotely debugging."));
-
   if (!target_has_execution)
     error (_("No process to detach from."));
 
@@ -5165,15 +5162,15 @@ remote_detach_1 (const char *args, int from_tty)
 }
 
 static void
-remote_detach (struct target_ops *ops, const char *args, int from_tty)
+remote_detach (struct target_ops *ops, int from_tty)
 {
-  remote_detach_1 (args, from_tty);
+  remote_detach_1 (from_tty);
 }
 
 static void
-extended_remote_detach (struct target_ops *ops, const char *args, int from_tty)
+extended_remote_detach (struct target_ops *ops, int from_tty)
 {
-  remote_detach_1 (args, from_tty);
+  remote_detach_1 (from_tty);
 }
 
 /* Target follow-fork function for remote targets.  On entry, and
diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index 7c08e39652..e724c7e0fa 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -348,14 +348,14 @@ lwp_to_thread (ptid_t lwp)
    program was started via the normal ptrace (PTRACE_TRACEME).  */
 
 static void
-sol_thread_detach (struct target_ops *ops, const char *args, int from_tty)
+sol_thread_detach (struct target_ops *ops, int from_tty)
 {
   struct target_ops *beneath = find_target_beneath (ops);
 
   sol_thread_active = 0;
   inferior_ptid = pid_to_ptid (ptid_get_pid (main_ph.ptid));
   unpush_target (ops);
-  beneath->to_detach (beneath, args, from_tty);
+  beneath->to_detach (beneath, from_tty);
 }
 
 /* Resume execution of process PTID.  If STEP is nozero, then just
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index aaf11d81b8..6bb7d7fba7 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -28,28 +28,26 @@ debug_post_attach (struct target_ops *self, int arg1)
 }
 
 static void
-delegate_detach (struct target_ops *self, const char *arg1, int arg2)
+delegate_detach (struct target_ops *self, int arg1)
 {
   self = self->beneath;
-  self->to_detach (self, arg1, arg2);
+  self->to_detach (self, arg1);
 }
 
 static void
-tdefault_detach (struct target_ops *self, const char *arg1, int arg2)
+tdefault_detach (struct target_ops *self, int arg1)
 {
 }
 
 static void
-debug_detach (struct target_ops *self, const char *arg1, int arg2)
+debug_detach (struct target_ops *self, int arg1)
 {
   fprintf_unfiltered (gdb_stdlog, "-> %s->to_detach (...)\n", debug_target.to_shortname);
-  debug_target.to_detach (&debug_target, arg1, arg2);
+  debug_target.to_detach (&debug_target, arg1);
   fprintf_unfiltered (gdb_stdlog, "<- %s->to_detach (", debug_target.to_shortname);
   target_debug_print_struct_target_ops_p (&debug_target);
   fputs_unfiltered (", ", gdb_stdlog);
-  target_debug_print_const_char_p (arg1);
-  fputs_unfiltered (", ", gdb_stdlog);
-  target_debug_print_int (arg2);
+  target_debug_print_int (arg1);
   fputs_unfiltered (")\n", gdb_stdlog);
 }
 
diff --git a/gdb/target.c b/gdb/target.c
index 748d5bb7a2..b01e1ac645 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2108,7 +2108,7 @@ dispose_inferior (struct inferior *inf, void *args)
       if (target_has_execution)
 	target_kill ();
       else
-	target_detach (NULL, 0);
+	target_detach (0);
     }
 
   return 0;
@@ -2141,10 +2141,10 @@ target_preopen (int from_tty)
   target_pre_inferior (from_tty);
 }
 
-/* Detach a target after doing deferred register stores.  */
+/* See target.h.  */
 
 void
-target_detach (const char *args, int from_tty)
+target_detach (int from_tty)
 {
   if (gdbarch_has_global_breakpoints (target_gdbarch ()))
     /* Don't remove global breakpoints here.  They're removed on
@@ -2157,7 +2157,7 @@ target_detach (const char *args, int from_tty)
 
   prepare_for_detach ();
 
-  current_target.to_detach (&current_target, args, from_tty);
+  current_target.to_detach (&current_target, from_tty);
 }
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 4781fbc59c..51841a1cbb 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -440,7 +440,7 @@ struct target_ops
     void (*to_attach) (struct target_ops *ops, const char *, int);
     void (*to_post_attach) (struct target_ops *, int)
       TARGET_DEFAULT_IGNORE ();
-    void (*to_detach) (struct target_ops *ops, const char *, int)
+    void (*to_detach) (struct target_ops *ops, int)
       TARGET_DEFAULT_IGNORE ();
     void (*to_disconnect) (struct target_ops *, const char *, int)
       TARGET_DEFAULT_NORETURN (tcomplain ());
@@ -1323,11 +1323,10 @@ extern void target_announce_detach (int from_tty);
 /* Takes a program previously attached to and detaches it.
    The program may resume execution (some targets do, some don't) and will
    no longer stop on signals, etc.  We better not have left any breakpoints
-   in the program or it'll die when it hits one.  ARGS is arguments
-   typed by the user (e.g. a signal to send the process).  FROM_TTY
-   says whether to be verbose or not.  */
+   in the program or it'll die when it hits one.  FROM_TTY says whether to be
+   verbose or not.  */
 
-extern void target_detach (const char *, int);
+extern void target_detach (int from_tty);
 
 /* Disconnect from the current target without resuming it (leaving it
    waiting for a debugger).  */
diff --git a/gdb/top.c b/gdb/top.c
index 4749cf561d..1e6d5137b8 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1461,7 +1461,6 @@ set_prompt (const char *s)
 
 struct qt_args
 {
-  char *args;
   int from_tty;
 };
 
@@ -1486,7 +1485,7 @@ kill_or_detach (struct inferior *inf, void *args)
       if (target_has_execution)
 	{
 	  if (inf->attach_flag)
-	    target_detach (qt->args, qt->from_tty);
+	    target_detach (qt->from_tty);
 	  else
 	    target_kill ();
 	}
@@ -1577,7 +1576,6 @@ quit_force (int *exit_arg, int from_tty)
   else if (return_child_result)
     exit_code = return_child_result_value;
 
-  qt.args = NULL;
   qt.from_tty = from_tty;
 
   /* We want to handle any quit errors and exit regardless.  */
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 29b983296e..4023089a4d 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1931,7 +1931,7 @@ windows_attach (struct target_ops *ops, const char *args, int from_tty)
 }
 
 static void
-windows_detach (struct target_ops *ops, const char *args, int from_tty)
+windows_detach (struct target_ops *ops, int from_tty)
 {
   int detached = 1;
 
-- 
2.15.1

[PATCH v2 3/3] Make linux_nat_detach/thread_db_detach use the inferior parameter

[Simon Marchi]

From: Simon Marchi <simon.marchi@ericsson.com>

No changes in v2.

This patch makes these two functions actually use the inferior parameter
added by the previous patch, instead of reading inferior_ptid.  I chose
these two, because they are the one actually used when I detach on my
GNU/Linux system, so they were easy to test.

I took the opportunity to pass the inferior being detached to
inf_ptrace_detach_success, so it could use it too.  From there, it made
sense to add an overload of detach_inferior that takes the inferior
directly rather than the pid, to avoid having to pass inf->pid only for
the callee to look up the inferior structure by pid.

gdb/ChangeLog:

	* inf-ptrace.c (inf_ptrace_detach): Adjust call to
	inf_ptrace_detach_success.
	(inf_ptrace_detach_success): Add inferior parameter, use it
	instead of inferior_ptid, pass it to detach_inferior.
	* inf-ptrace.h (inf_ptrace_detach_success): Add inferior
	parameter.
	* inferior.c (detach_inferior): Add overload that takes an
	inferior object.
	* inferior.h (detach_inferior): Likewise.
	* linux-nat.c (linux_nat_detach): Use the inf parameter, don't
	use inferior_ptid, adjust call to inf_ptrace_detach_success.
	* linux-thread-db.c (thread_db_detach): Use inf parameter.
---
 gdb/inf-ptrace.c      |  8 +++-----
 gdb/inf-ptrace.h      |  2 +-
 gdb/inferior.c        | 15 +++++++++++++--
 gdb/inferior.h        |  3 +++
 gdb/linux-nat.c       |  8 +++-----
 gdb/linux-thread-db.c |  2 +-
 6 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index aa8b17f41a..72aa33480f 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -263,18 +263,16 @@ inf_ptrace_detach (struct target_ops *ops, inferior *inf, int from_tty)
   error (_("This system does not support detaching from a process"));
 #endif
 
-  inf_ptrace_detach_success (ops);
+  inf_ptrace_detach_success (ops, inf);
 }
 
 /* See inf-ptrace.h.  */
 
 void
-inf_ptrace_detach_success (struct target_ops *ops)
+inf_ptrace_detach_success (struct target_ops *ops, inferior *inf)
 {
-  pid_t pid = ptid_get_pid (inferior_ptid);
-
   inferior_ptid = null_ptid;
-  detach_inferior (pid);
+  detach_inferior (inf);
 
   inf_child_maybe_unpush_target (ops);
 }
diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
index c5bd757360..d10f64ae56 100644
--- a/gdb/inf-ptrace.h
+++ b/gdb/inf-ptrace.h
@@ -40,6 +40,6 @@ extern pid_t get_ptrace_pid (ptid_t);
 
 
 /* Cleanup the inferior after a successful ptrace detach.  */
-extern void inf_ptrace_detach_success (struct target_ops *ops);
+extern void inf_ptrace_detach_success (struct target_ops *ops, inferior *inf);
 
 #endif
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 0b8f340b63..38b7369275 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -253,10 +253,13 @@ exit_inferior_num_silent (int num)
   exit_inferior_1 (inf, 1);
 }
 
+/* See inferior.h.  */
+
 void
-detach_inferior (int pid)
+detach_inferior (inferior *inf)
 {
-  struct inferior *inf = find_inferior_pid (pid);
+  /* Save the pid, since exit_inferior_1 will reset it.  */
+  int pid = inf->pid;
 
   exit_inferior_1 (inf, 0);
 
@@ -264,6 +267,14 @@ detach_inferior (int pid)
     printf_unfiltered (_("[Inferior %d detached]\n"), pid);
 }
 
+/* See inferior.h.  */
+
+void
+detach_inferior (int pid)
+{
+  detach_inferior (find_inferior_pid (pid));
+}
+
 void
 inferior_appeared (struct inferior *inf, int pid)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 01a12f2290..a87ffe0054 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -458,6 +458,9 @@ extern struct inferior *add_inferior_silent (int pid);
 extern void delete_inferior (struct inferior *todel);
 
 /* Delete an existing inferior list entry, due to inferior detaching.  */
+extern void detach_inferior (inferior *inf);
+
+/* Same as the above, but with the inferior specified by PID.  */
 extern void detach_inferior (int pid);
 
 extern void exit_inferior (int pid);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 70f4c084dc..b83066cb04 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1499,10 +1499,8 @@ detach_callback (struct lwp_info *lp, void *data)
 static void
 linux_nat_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
-  int pid;
   struct lwp_info *main_lwp;
-
-  pid = ptid_get_pid (inferior_ptid);
+  int pid = inf->pid;
 
   /* Don't unregister from the event loop, as there may be other
      inferiors running. */
@@ -1517,7 +1515,7 @@ linux_nat_detach (struct target_ops *ops, inferior *inf, int from_tty)
   iterate_over_lwps (pid_to_ptid (pid), detach_callback, NULL);
 
   /* Only the initial process should be left right now.  */
-  gdb_assert (num_lwps (ptid_get_pid (inferior_ptid)) == 1);
+  gdb_assert (num_lwps (pid) == 1);
 
   main_lwp = find_lwp_pid (pid_to_ptid (pid));
 
@@ -1538,7 +1536,7 @@ linux_nat_detach (struct target_ops *ops, inferior *inf, int from_tty)
 
       detach_one_lwp (main_lwp, &signo);
 
-      inf_ptrace_detach_success (ops);
+      inf_ptrace_detach_success (ops, inf);
     }
 }
 
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 873f929144..794c97b48a 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1094,7 +1094,7 @@ thread_db_detach (struct target_ops *ops, inferior *inf, int from_tty)
 {
   struct target_ops *target_beneath = find_target_beneath (ops);
 
-  delete_thread_db_info (ptid_get_pid (inferior_ptid));
+  delete_thread_db_info (inf->pid);
 
   target_beneath->to_detach (target_beneath, inf, from_tty);
 
-- 
2.15.1

Re: [PATCH v2 1/3] Remove args from target detach

[Pedro Alves]

On 01/19/2018 04:16 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@ericsson.com>
> 
> New in v2:
> 
> - Remove signo parameter from do_detach in procfs.c.

OK for the series.

Thanks,
Pedro Alves

Re: [PATCH v2 1/3] Remove args from target detach

[Simon Marchi]

On 2018-01-19 11:35, Pedro Alves wrote:
> On 01/19/2018 04:16 PM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> 
>> New in v2:
>> 
>> - Remove signo parameter from do_detach in procfs.c.
> 
> OK for the series.
> 
> Thanks,
> Pedro Alves

Thanks, pushed.

Simon

[Regression] Segfault on native-extended-gdbserver + fork (was: Re: [PATCH v2 3/3] Make linux_nat_detach/thread_db_detach use the inferior parameter)

[Sergio Durigan Junior]

On Friday, January 19 2018, Simon Marchi wrote:

> From: Simon Marchi <simon.marchi@ericsson.com>
>
> No changes in v2.
>
> This patch makes these two functions actually use the inferior parameter
> added by the previous patch, instead of reading inferior_ptid.  I chose
> these two, because they are the one actually used when I detach on my
> GNU/Linux system, so they were easy to test.
>
> I took the opportunity to pass the inferior being detached to
> inf_ptrace_detach_success, so it could use it too.  From there, it made
> sense to add an overload of detach_inferior that takes the inferior
> directly rather than the pid, to avoid having to pass inf->pid only for
> the callee to look up the inferior structure by pid.

Hey Simon,

While working on something else, I noticed a regression introduced by
this patch.  Consider the following example program:

  #include <unistd.h>

  int
  main (int argc, char *argv[])
  {
    fork ();

    return 0;
  }

When running it under gdbserver:

  # ./gdb/gdbserver/gdbserver --multi --once :2345

And debugging it under GDB:

  # ./gdb/gdb -q -batch -ex 'set remote exec-file ./a.out' -ex 'tar extended-remote :2345' -ex r ./a.out
  Starting program:
  ...
  [Detaching after fork from child process 16102.]
  Segmentation fault (core dumped)

The problem happens on inferior.c:detach_inferior:

  void
  detach_inferior (inferior *inf)
  {
    /* Save the pid, since exit_inferior_1 will reset it.  */
    int pid = inf->pid;
              ^^^^^^^^^

    exit_inferior_1 (inf, 0);

    if (print_inferior_events)
      printf_unfiltered (_("[Inferior %d detached]\n"), pid);
  }

When this code is called from remote.c:remote_follow_fork, the PID is
valid but there is not 'inferior' associated with it, which means that
'inf == NULL'.

I've been thinking about the proper fix to this, and arrived at the
patch attached (without a ChangeLog entry; will add that if the patch
seems OK for you).  Since we will still want to print inferior events
even if 'inf == NULL', I've duplicated the print on the "detach_inferior
(int pid)" version.  Other than that, the patch is basically restoring
the old behaviour of just skipping the detach procedure if there's no
inferior object.

I'm running a regression test on BuildBot to make sure no regressions
are introduced.  I was going to write a testcase to exercise this
scenario, but we already have one, gdb.base/foll-vfork.exp.  The
failures were marked as ERROR's by dejagnu, which may explain why they
were missed...?  Not sure.  Oh, and this regression is not present in
the 8.1 branch.

WDYT?

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 38b7369275..94432a92b1 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -272,7 +272,15 @@ detach_inferior (inferior *inf)
 void
 detach_inferior (int pid)
 {
-  detach_inferior (find_inferior_pid (pid));
+  inferior *inf = find_inferior_pid (pid);
+
+  if (inf != NULL)
+    detach_inferior (inf);
+  else
+    {
+      if (print_inferior_events)
+	printf_unfiltered (_("[Inferior %d detached]\n"), pid);
+    }
 }
 
 void

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Simon Marchi]

On 2018-01-28 01:32, Sergio Durigan Junior wrote:
> Hey Simon,
> 
> While working on something else, I noticed a regression introduced by
> this patch.

Hi Sergio,

Thanks for reporting and fixing this!

> Consider the following example program:
> 
>   #include <unistd.h>
> 
>   int
>   main (int argc, char *argv[])
>   {
>     fork ();
> 
>     return 0;
>   }
> 
> When running it under gdbserver:
> 
>   # ./gdb/gdbserver/gdbserver --multi --once :2345
> 
> And debugging it under GDB:
> 
>   # ./gdb/gdb -q -batch -ex 'set remote exec-file ./a.out' -ex 'tar
> extended-remote :2345' -ex r ./a.out
>   Starting program:
>   ...
>   [Detaching after fork from child process 16102.]
>   Segmentation fault (core dumped)
> 
> The problem happens on inferior.c:detach_inferior:
> 
>   void
>   detach_inferior (inferior *inf)
>   {
>     /* Save the pid, since exit_inferior_1 will reset it.  */
>     int pid = inf->pid;
>               ^^^^^^^^^
> 
>     exit_inferior_1 (inf, 0);
> 
>     if (print_inferior_events)
>       printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>   }
> 
> When this code is called from remote.c:remote_follow_fork, the PID is
> valid but there is not 'inferior' associated with it, which means that
> 'inf == NULL'.

Ah yeah, I hadn't thought about that.

> I've been thinking about the proper fix to this, and arrived at the
> patch attached (without a ChangeLog entry; will add that if the patch
> seems OK for you).  Since we will still want to print inferior events
> even if 'inf == NULL', I've duplicated the print on the 
> "detach_inferior
> (int pid)" version.  Other than that, the patch is basically restoring
> the old behaviour of just skipping the detach procedure if there's no
> inferior object.

I'm fine with this, but I was curious about what happens in Pedro's 
multi-target branch.  I remember he said that the detach_inferior(int) 
version disappears in that branch, though I can't find where he said 
that.  But looking at the branch I can see it's indeed the case:

   
https://github.com/palves/gdb/blob/palves/multi-target/gdb/inferior.c#L250

So I was wondering what remote_follow_fork calls in that case, since it 
can't call the detach_inferior(inferior *) version without an inferior.  
Apparently it calls a new remote_detach_pid function:

   
https://github.com/palves/gdb/blob/palves/multi-target/gdb/remote.c#L5859

This means (I just tried it) that it won't show the "[Inferior %d 
detached]\n" message in that case.  So what I would suggest is putting

   if (print_inferior_events)
     printf_unfiltered (_("[Inferior %d detached]\n"), pid);

in its own function, called by both versions of detach_inferior for now 
(bonus, it de-duplicates the printing of the message).  In the 
multi-target branch, remote_target::follow_fork (renamed from 
remote_follow_fork) can call this function in the case where we don't 
have an inferior object.

> I'm running a regression test on BuildBot to make sure no regressions
> are introduced.  I was going to write a testcase to exercise this
> scenario, but we already have one, gdb.base/foll-vfork.exp.  The
> failures were marked as ERROR's by dejagnu, which may explain why they
> were missed...?  Not sure.  Oh, and this regression is not present in
> the 8.1 branch.
> 
> WDYT?

That's fine with me with the printing of the message in its own 
function, as explained above.

> --
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
> Please send encrypted e-mail if possible
> http://sergiodj.net/
> 
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 38b7369275..94432a92b1 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -272,7 +272,15 @@ detach_inferior (inferior *inf)
>  void
>  detach_inferior (int pid)
>  {
> -  detach_inferior (find_inferior_pid (pid));
> +  inferior *inf = find_inferior_pid (pid);
> +
> +  if (inf != NULL)
> +    detach_inferior (inf);
> +  else
> +    {
> +      if (print_inferior_events)
> +	printf_unfiltered (_("[Inferior %d detached]\n"), pid);
> +    }
>  }
> 
>  void

Thanks,

Simon

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Pedro Alves]

On 01/28/2018 04:50 PM, Simon Marchi wrote:
> On 2018-01-28 01:32, Sergio Durigan Junior wrote:

> I'm fine with this, but I was curious about what happens in Pedro's multi-target branch.  I remember he said that the detach_inferior(int) version disappears in that branch, though I can't find where he said that.  But looking at the branch I can see it's indeed the case:
> 
>   https://github.com/palves/gdb/blob/palves/multi-target/gdb/inferior.c#L250
> 
> So I was wondering what remote_follow_fork calls in that case, since it can't call the detach_inferior(inferior *) version without an inferior.  Apparently it calls a new remote_detach_pid function:
> 
>   https://github.com/palves/gdb/blob/palves/multi-target/gdb/remote.c#L5859

remote_detach_pid is not new.  It exists in master.  What that url shows
is that I commented out the detach_inferior call in the branch.

Because in this case, we'd detaching a remote process that the core
of gdb never learned about.

> 
> This means (I just tried it) that it won't show the "[Inferior %d detached]\n" message in that case.  So what I would suggest is putting
> 
>   if (print_inferior_events)
>     printf_unfiltered (_("[Inferior %d detached]\n"), pid);
> 
> in its own function, called by both versions of detach_inferior for now (bonus, it de-duplicates the printing of the message).  In the multi-target branch, remote_target::follow_fork (renamed from remote_follow_fork) can call this function in the case where we don't have an inferior object.

But why would we want to print that?  We will have already printed

  "Detaching after fork from child process PID."

from the common code.  When native debugging, in this scenario,
we don't call detach_inferior either, right?  Can't see why
we'd want to call it for remote.

I think we should just remove that detach_inferior call, like in
the multi-target branch.

Thanks,
Pedro Alves

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Simon Marchi]

On 2018-01-29 11:00, Pedro Alves wrote:
> On 01/28/2018 04:50 PM, Simon Marchi wrote:
>> On 2018-01-28 01:32, Sergio Durigan Junior wrote:
> 
>> I'm fine with this, but I was curious about what happens in Pedro's 
>> multi-target branch.  I remember he said that the detach_inferior(int) 
>> version disappears in that branch, though I can't find where he said 
>> that.  But looking at the branch I can see it's indeed the case:
>> 
>>   
>> https://github.com/palves/gdb/blob/palves/multi-target/gdb/inferior.c#L250
>> 
>> So I was wondering what remote_follow_fork calls in that case, since 
>> it can't call the detach_inferior(inferior *) version without an 
>> inferior.  Apparently it calls a new remote_detach_pid function:
>> 
>>   
>> https://github.com/palves/gdb/blob/palves/multi-target/gdb/remote.c#L5859
> 
> remote_detach_pid is not new.  It exists in master.  What that url 
> shows
> is that I commented out the detach_inferior call in the branch.
> 
> Because in this case, we'd detaching a remote process that the core
> of gdb never learned about.

Oops I read that wrong.

>> 
>> This means (I just tried it) that it won't show the "[Inferior %d 
>> detached]\n" message in that case.  So what I would suggest is putting
>> 
>>   if (print_inferior_events)
>>     printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>> 
>> in its own function, called by both versions of detach_inferior for 
>> now (bonus, it de-duplicates the printing of the message).  In the 
>> multi-target branch, remote_target::follow_fork (renamed from 
>> remote_follow_fork) can call this function in the case where we don't 
>> have an inferior object.
> 
> But why would we want to print that?  We will have already printed
> 
>   "Detaching after fork from child process PID."
> 
> from the common code.  When native debugging, in this scenario,
> we don't call detach_inferior either, right?  Can't see why
> we'd want to call it for remote.

It's true that it's a bit of a lie to say "[Inferior PID detached]" if 
there never actually was an inferior for that PID.  Since we never print 
"[Inferior PID detached]" on native in that case, I am fine with 
removing the call from remote.c.  Sergio, that would fix the crash you 
found I think?

Simon

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Pedro Alves]

On 01/29/2018 04:25 PM, Simon Marchi wrote:

> It's true that it's a bit of a lie to say "[Inferior PID detached]" if there never actually was an inferior for that PID.  Since we never print "[Inferior PID detached]" on native in that case, I am fine with removing the call from remote.c.  Sergio, that would fix the crash you found I think?

A tangent:

We should probably change that message from:

 [Inferior PID detached]

to something like:

 [Inferior ID (process PID) detached]

I.e.:
 [Inferior 24822 detached]
vs:
 [Inferior 1 (process 24822) detached]

In patch form, something like this:

From 6c1db47bc19669d9c84024d09f8a63b5eb78b6c2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 29 Jan 2018 16:41:25 +0000
Subject: [PATCH] change output

---
 gdb/inferior.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 38b7369275b..2986b510314 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -234,7 +234,8 @@ exit_inferior (int pid)
   exit_inferior_1 (inf, 0);
 
   if (print_inferior_events)
-    printf_unfiltered (_("[Inferior %d exited]\n"), pid);
+    printf_unfiltered (_("[Inferior %d (%s) exited]\n"),
+		       inf->num, target_pid_to_str (ptid_t (pid)));
 }
 
 void
@@ -264,7 +265,8 @@ detach_inferior (inferior *inf)
   exit_inferior_1 (inf, 0);
 
   if (print_inferior_events)
-    printf_unfiltered (_("[Inferior %d detached]\n"), pid);
+    printf_unfiltered (_("[Inferior %d (%s) detached]\n"),
+		       inf->num, target_pid_to_str (ptid_t (pid)));
 }
 
 /* See inferior.h.  */
-- 
2.14.3

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Simon Marchi]

On 2018-01-29 11:58, Pedro Alves wrote:
> On 01/29/2018 04:25 PM, Simon Marchi wrote:
> 
>> It's true that it's a bit of a lie to say "[Inferior PID detached]" if 
>> there never actually was an inferior for that PID.  Since we never 
>> print "[Inferior PID detached]" on native in that case, I am fine with 
>> removing the call from remote.c.  Sergio, that would fix the crash you 
>> found I think?
> 
> A tangent:
> 
> We should probably change that message from:
> 
>  [Inferior PID detached]
> 
> to something like:
> 
>  [Inferior ID (process PID) detached]
> 
> I.e.:
>  [Inferior 24822 detached]
> vs:
>  [Inferior 1 (process 24822) detached]
> 
> In patch form, something like this:
> 
> From 6c1db47bc19669d9c84024d09f8a63b5eb78b6c2 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 29 Jan 2018 16:41:25 +0000
> Subject: [PATCH] change output
> 
> ---
>  gdb/inferior.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 38b7369275b..2986b510314 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -234,7 +234,8 @@ exit_inferior (int pid)
>    exit_inferior_1 (inf, 0);
> 
>    if (print_inferior_events)
> -    printf_unfiltered (_("[Inferior %d exited]\n"), pid);
> +    printf_unfiltered (_("[Inferior %d (%s) exited]\n"),
> +		       inf->num, target_pid_to_str (ptid_t (pid)));
>  }
> 
>  void
> @@ -264,7 +265,8 @@ detach_inferior (inferior *inf)
>    exit_inferior_1 (inf, 0);
> 
>    if (print_inferior_events)
> -    printf_unfiltered (_("[Inferior %d detached]\n"), pid);
> +    printf_unfiltered (_("[Inferior %d (%s) detached]\n"),
> +		       inf->num, target_pid_to_str (ptid_t (pid)));
>  }
> 
>  /* See inferior.h.  */

Agreed.  Though I don't think Sergio's original patch is needed if we 
instead remove the detach_inferior call in remote.c, so your patch 
shouldn't be based on his.

Simon

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Sergio Durigan Junior]

On Monday, January 29 2018, Simon Marchi wrote:

> On 2018-01-29 11:00, Pedro Alves wrote:
>> On 01/28/2018 04:50 PM, Simon Marchi wrote:
>>> On 2018-01-28 01:32, Sergio Durigan Junior wrote:
>>> This means (I just tried it) that it won't show the "[Inferior %d
>>> detached]\n" message in that case.  So what I would suggest is
>>> putting
>>>
>>>   if (print_inferior_events)
>>>     printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>>>
>>> in its own function, called by both versions of detach_inferior for
>>> now (bonus, it de-duplicates the printing of the message).  In the
>>> multi-target branch, remote_target::follow_fork (renamed from
>>> remote_follow_fork) can call this function in the case where we
>>> don't have an inferior object.
>>
>> But why would we want to print that?  We will have already printed
>>
>>   "Detaching after fork from child process PID."
>>
>> from the common code.  When native debugging, in this scenario,
>> we don't call detach_inferior either, right?  Can't see why
>> we'd want to call it for remote.
>
> It's true that it's a bit of a lie to say "[Inferior PID detached]" if
> there never actually was an inferior for that PID.  Since we never
> print "[Inferior PID detached]" on native in that case, I am fine with
> removing the call from remote.c.  Sergio, that would fix the crash you
> found I think?

I was also unsure about printing the message in this case, because
there's no real detach happening.  I'm fine with not printing it.  And
yes, removing the call to "detach_inferior" also fixes the problem.

I'll prepare a patch.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Pedro Alves]

On 01/29/2018 05:03 PM, Simon Marchi wrote:
> On 2018-01-29 11:58, Pedro Alves wrote:
>> On 01/29/2018 04:25 PM, Simon Marchi wrote:
>>
>>> It's true that it's a bit of a lie to say "[Inferior PID detached]" if there never actually was an inferior for that PID.  Since we never print "[Inferior PID detached]" on native in that case, I am fine with removing the call from remote.c.  Sergio, that would fix the crash you found I think?
>>
>> A tangent:
>>
>> We should probably change that message from:
>>
>>  [Inferior PID detached]
>>
>> to something like:
>>
>>  [Inferior ID (process PID) detached]
>>
>> I.e.:
>>  [Inferior 24822 detached]
>> vs:
>>  [Inferior 1 (process 24822) detached]
>>
>> In patch form, something like this:
>>

> Agreed.  Though I don't think Sergio's original patch is needed if we instead remove the detach_inferior call in remote.c, so your patch shouldn't be based on his.

Yeah, my patch was a tangent / orthogonal to his regression fix.
It was against master.  I was thinking more about the whole
effort to enable "set print inferior-events" on by default
that Sergio is working on in context of:
<https://sourceware.org/ml/gdb-patches/2018-01/msg00531.html>

Thanks,
Pedro Alves

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Pedro Alves]

On 01/29/2018 05:31 PM, Pedro Alves wrote:
> On 01/29/2018 05:03 PM, Simon Marchi wrote:
>> On 2018-01-29 11:58, Pedro Alves wrote:

>> Agreed.  Though I don't think Sergio's original patch is needed if we instead remove the detach_inferior call in remote.c, so your patch shouldn't be based on his.
> 
> Yeah, my patch was a tangent / orthogonal to his regression fix.
> It was against master.  I was thinking more about the whole
> effort to enable "set print inferior-events" on by default
> that Sergio is working on in context of:
> <https://sourceware.org/ml/gdb-patches/2018-01/msg00531.html>
> 

And now that I reread that url, I remember that I was actually
proposing to drop the print from within exit_inferior.  Otherwise,
we'd end up with (with both that url's patch and the patch I
just posted):

 ~~
 [Inferior 1 (process 2629) exited normally]
 [Inferior 1 (process 2629) exited]
 ~~

which is the sort of redundancy I was talking about.

Likely we'll end up doing the same to detach_inferior
too.  So I'll just forget this patch for now, as it'll
probably end up unnecessary.

Thanks,
Pedro Alves

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Sergio Durigan Junior]

On Monday, January 29 2018, I wrote:

> On Monday, January 29 2018, Simon Marchi wrote:
>
>> On 2018-01-29 11:00, Pedro Alves wrote:
>>> On 01/28/2018 04:50 PM, Simon Marchi wrote:
>>>> On 2018-01-28 01:32, Sergio Durigan Junior wrote:
>>>> This means (I just tried it) that it won't show the "[Inferior %d
>>>> detached]\n" message in that case.  So what I would suggest is
>>>> putting
>>>>
>>>>   if (print_inferior_events)
>>>>     printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>>>>
>>>> in its own function, called by both versions of detach_inferior for
>>>> now (bonus, it de-duplicates the printing of the message).  In the
>>>> multi-target branch, remote_target::follow_fork (renamed from
>>>> remote_follow_fork) can call this function in the case where we
>>>> don't have an inferior object.
>>>
>>> But why would we want to print that?  We will have already printed
>>>
>>>   "Detaching after fork from child process PID."
>>>
>>> from the common code.  When native debugging, in this scenario,
>>> we don't call detach_inferior either, right?  Can't see why
>>> we'd want to call it for remote.
>>
>> It's true that it's a bit of a lie to say "[Inferior PID detached]" if
>> there never actually was an inferior for that PID.  Since we never
>> print "[Inferior PID detached]" on native in that case, I am fine with
>> removing the call from remote.c.  Sergio, that would fix the crash you
>> found I think?
>
> I was also unsure about printing the message in this case, because
> there's no real detach happening.  I'm fine with not printing it.  And
> yes, removing the call to "detach_inferior" also fixes the problem.
>
> I'll prepare a patch.

Here's what I have.  WDYT?

I'll address Pedro's comment about changing the "[Inferior PID
detached]" output in another patch.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

From 4a37d08ca6c1aec7f47e2278b0fe78a0038eb9ee Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Mon, 29 Jan 2018 12:29:21 -0500
Subject: [PATCH] Don't call "detach_inferior" on "remote_follow_fork"

This patch fixes a regression that has been introduced by:

commit bc09b0c14fb713a9aec25e09b78499f3bc2441b5
Date:   Fri Jan 19 11:48:11 2018 -0500

    Make linux_nat_detach/thread_db_detach use the inferior parameter

Consider the following example program:

  #include <unistd.h>

  int
  main (int argc, char *argv[])
  {
    fork ();

    return 0;
  }

When running it under gdbserver:

  # ./gdb/gdbserver/gdbserver --multi --once :2345

And debugging it under GDB, we see a segmentation fault:

  # ./gdb/gdb -q -batch -ex 'set remote exec-file ./a.out' -ex 'tar extended-remote :2345' -ex r ./a.out
  Starting program:
  ...
  [Detaching after fork from child process 16102.]
  Segmentation fault (core dumped)

The problem happens on inferior.c:detach_inferior:

  void
  detach_inferior (inferior *inf)
  {
    /* Save the pid, since exit_inferior_1 will reset it.  */
    int pid = inf->pid;
              ^^^^^^^^^

    exit_inferior_1 (inf, 0);

    if (print_inferior_events)
      printf_unfiltered (_("[Inferior %d detached]\n"), pid);
  }

When this code is called from remote.c:remote_follow_fork, the PID is
valid but there is not 'inferior' associated with it, which means that
'inf == NULL'.

The proper fix here is to not call "detach_inferior" when doing remote
follow-fork, because we don't have an inferior to detach on the host
side.

This has been regtested using BuildBot and no regressions were found.

gdb/ChangeLog:
2018-01-29  Sergio Durigan Junior  <sergiodj@redhat.com>

	* remote.c (remote_follow_fork): Don't call "detach_inferior".
---
 gdb/remote.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 5ac84df0a0..74d18f7b17 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5206,7 +5206,6 @@ remote_follow_fork (struct target_ops *ops, int follow_child,
 	  child_pid = ptid_get_pid (child_ptid);
 
 	  remote_detach_pid (child_pid);
-	  detach_inferior (child_pid);
 	}
     }
   return 0;
-- 
2.14.3

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Pedro Alves]

On 01/29/2018 05:36 PM, Sergio Durigan Junior wrote:
> From 4a37d08ca6c1aec7f47e2278b0fe78a0038eb9ee Mon Sep 17 00:00:00 2001
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Mon, 29 Jan 2018 12:29:21 -0500
> Subject: [PATCH] Don't call "detach_inferior" on "remote_follow_fork"
> 
> This patch fixes a regression that has been introduced by:
> 
> commit bc09b0c14fb713a9aec25e09b78499f3bc2441b5
> Date:   Fri Jan 19 11:48:11 2018 -0500
> 
>     Make linux_nat_detach/thread_db_detach use the inferior parameter
> 
> Consider the following example program:
> 
>   #include <unistd.h>
> 
>   int
>   main (int argc, char *argv[])
>   {
>     fork ();
> 
>     return 0;
>   }
> 

Please also mention gdb.base/foll-fork.exp.

> When running it under gdbserver:
> 
>   # ./gdb/gdbserver/gdbserver --multi --once :2345
> 
> And debugging it under GDB, we see a segmentation fault:
> 
>   # ./gdb/gdb -q -batch -ex 'set remote exec-file ./a.out' -ex 'tar extended-remote :2345' -ex r ./a.out
>   Starting program:
>   ...
>   [Detaching after fork from child process 16102.]
>   Segmentation fault (core dumped)
> 
> The problem happens on inferior.c:detach_inferior:
> 
>   void
>   detach_inferior (inferior *inf)
>   {
>     /* Save the pid, since exit_inferior_1 will reset it.  */
>     int pid = inf->pid;
>               ^^^^^^^^^
> 
>     exit_inferior_1 (inf, 0);
> 
>     if (print_inferior_events)
>       printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>   }
> 
> When this code is called from remote.c:remote_follow_fork, the PID is
> valid but there is not 'inferior' associated with it, which means that
> 'inf == NULL'.

s/there is not/there is no/

> 
> The proper fix here is to not call "detach_inferior" when doing remote
> follow-fork, because we don't have an inferior to detach on the host
> side.

Add something like this here:

 Before bc09b0c1, that call was already a nop (exit_inferior_1 bails
 out early if you pass it a NULL inferior), except that it printed
 "Inferior PID detached" when "set print inferior-events" is on.
 Since native debugging doesn't call detach_inferior in this case,
 removing the call from remote aligns remote debugging output
 with native debugging output further.

and it's good to me.

Thanks,
Pedro Alves

Re: [Regression] Segfault on native-extended-gdbserver + fork

[Sergio Durigan Junior]

On Monday, January 29 2018, Pedro Alves wrote:

> On 01/29/2018 05:36 PM, Sergio Durigan Junior wrote:
>> From 4a37d08ca6c1aec7f47e2278b0fe78a0038eb9ee Mon Sep 17 00:00:00 2001
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Mon, 29 Jan 2018 12:29:21 -0500
>> Subject: [PATCH] Don't call "detach_inferior" on "remote_follow_fork"
>> 
>> This patch fixes a regression that has been introduced by:
>> 
>> commit bc09b0c14fb713a9aec25e09b78499f3bc2441b5
>> Date:   Fri Jan 19 11:48:11 2018 -0500
>> 
>>     Make linux_nat_detach/thread_db_detach use the inferior parameter
>> 
>> Consider the following example program:
>> 
>>   #include <unistd.h>
>> 
>>   int
>>   main (int argc, char *argv[])
>>   {
>>     fork ();
>> 
>>     return 0;
>>   }
>> 
>
> Please also mention gdb.base/foll-fork.exp.

Done.

>> When running it under gdbserver:
>> 
>>   # ./gdb/gdbserver/gdbserver --multi --once :2345
>> 
>> And debugging it under GDB, we see a segmentation fault:
>> 
>>   # ./gdb/gdb -q -batch -ex 'set remote exec-file ./a.out' -ex 'tar extended-remote :2345' -ex r ./a.out
>>   Starting program:
>>   ...
>>   [Detaching after fork from child process 16102.]
>>   Segmentation fault (core dumped)
>> 
>> The problem happens on inferior.c:detach_inferior:
>> 
>>   void
>>   detach_inferior (inferior *inf)
>>   {
>>     /* Save the pid, since exit_inferior_1 will reset it.  */
>>     int pid = inf->pid;
>>               ^^^^^^^^^
>> 
>>     exit_inferior_1 (inf, 0);
>> 
>>     if (print_inferior_events)
>>       printf_unfiltered (_("[Inferior %d detached]\n"), pid);
>>   }
>> 
>> When this code is called from remote.c:remote_follow_fork, the PID is
>> valid but there is not 'inferior' associated with it, which means that
>> 'inf == NULL'.
>
> s/there is not/there is no/

Fixed.

>> 
>> The proper fix here is to not call "detach_inferior" when doing remote
>> follow-fork, because we don't have an inferior to detach on the host
>> side.
>
> Add something like this here:
>
>  Before bc09b0c1, that call was already a nop (exit_inferior_1 bails
>  out early if you pass it a NULL inferior), except that it printed
>  "Inferior PID detached" when "set print inferior-events" is on.
>  Since native debugging doesn't call detach_inferior in this case,
>  removing the call from remote aligns remote debugging output
>  with native debugging output further.

Added.

> and it's good to me.

Pushed.

69ab5edb4d601611ba7b4d05e56689d4b60ca3b1

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/