[RFA] Implement 'detach pid'.

[RFA] Implement 'detach pid'.

[Vladimir Prus]

This patch makes CLI 'detach' and MI '-target-detach' accept the PID
of the process to detach from.

Is the infcmd.c change OK?

- Volodya

From 8c0569ee17d85cf591426a0b9900d8cad0e02389 Mon Sep 17 00:00:00 2001

	* infcmd.c (_initialize_infcmd): Make 'detach' accept
	parameter.
	* mi/mi-cmds.c (mi_cmds): Make '-target-detach' pass parameter
	to 'detach'.
	* remote.c (remote_detach_1): Interpret the passed
	parameter as a pid to detach from.
---
 gdb/infcmd.c     |    2 +-
 gdb/mi/mi-cmds.c |    2 +-
 gdb/remote.c     |   13 +++++++++++--
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b3af31f..76b48ae 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2556,7 +2556,7 @@ to specify the program, and to load its symbol table."));
 Detach a process or file previously attached.\n\
 If a process, it is no longer traced, and it continues its execution.  If\n\
 you were debugging a file, the file is closed and gdb no longer accesses it."),
-		  &detachlist, "detach ", 0, &cmdlist);
+		  &detachlist, "detach ", 1, &cmdlist);
 
   add_com ("disconnect", class_run, disconnect_command, _("\
 Disconnect from a target.\n\
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index d38de35..708dcf8 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -121,7 +121,7 @@ struct mi_cmd mi_cmds[] =
   { "symbol-type", { NULL, 0 }, NULL },
   { "target-attach", { "attach", 1 }, NULL },
   { "target-compare-sections", { NULL, 0 }, NULL },
-  { "target-detach", { "detach", 0 }, 0 },
+  { "target-detach", { "detach", 1 }, 0 },
   { "target-disconnect", { "disconnect", 0 }, 0 },
   { "target-download", { "load", 1 }, NULL},
   { "target-exec-status", { NULL, 0 }, NULL },
diff --git a/gdb/remote.c b/gdb/remote.c
index 5cb36b8..b1adfdb 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3261,11 +3261,20 @@ remote_open_1 (char *name, int from_tty, struct target_ops *target, int extended
 static void
 remote_detach_1 (char *args, int from_tty, int extended)
 {
-  int pid = ptid_get_pid (inferior_ptid);
+  int pid;
   struct remote_state *rs = get_remote_state ();
 
   if (args)
-    error (_("Argument given to \"detach\" when remotely debugging."));
+    {      
+      char *end = args;
+      pid = strtol (args, &end, 10);
+      if (*end != '\0')
+	error (_("Cannot parse process id '%s'"), args);
+      if (!in_inferior_list (pid))
+	error (_("Invalid process id %d"), pid);
+    }
+  else
+    pid = ptid_get_pid (inferior_ptid);
 
   if (!target_has_execution)
     error (_("No process to detach from."));
-- 
1.5.3.5

Re: [RFA] Implement 'detach pid'.

[Pedro Alves]

On Wednesday 12 November 2008 20:39:02, Vladimir Prus wrote:
> 
> This patch makes CLI 'detach' and MI '-target-detach' accept the PID
> of the process to detach from.
> 

I see several issues with this patch:

 - The target is not the right layer to do this.  Before you reach here,
   you've already done things to the current inferior.  E.g., target.c:target_detach
   will call remove_breakpoints before reaching remote_detach_1.  This means that,
   if you have e.g., selected inferior 1, and do detach 3, you'll remove breakpoints
   from inferior 1, and detach process 3.  Breakpoints are an example.  Other example
   is that before you reach the process stratum, you can pass by the thread_stratum,
   which again would do anything to the wrong detachee, since it's usually the process_statum
   that does the final real detach.  That's bad.

   You should do the needed parsing at a higher level, switch to a thread of the process
   you want and then call the target method.  In the long run, ideally inferior_ptid
   shouldn't be so pervasive, but that's in a really far long run yet.  This also means
   that the syntax becomes uniform across targets.

 - Other targets are already using the args to mean something different, although
   at the cli level that broke (I'm guessing) with the introduction of the
   checkpoint/multifork support, which added subcommands to "detach".
   Several targets (e.g. inf-ptrace.c, procfs.c, inf-ttrace, nto-procfs.c) use it
   to detach with a signal.

 - "detach 42000" will make it through even in remote target (not extended remote).
   That's the pid used when we don't know about any (from magic_null_ptid).

   inferior ids have internal ids != pids (shown with info inferiors", just like threads
   have the internal thread id, and a ptid.  I wonder if at the cli level (don't
   know about MI), we shouldn't use those instead of the real PID (or have switches
   to specify what the number represents.  So, hippotetically "detach 1" would
   detach from "inferior 1", which could be pid 4234.  I'm not so sure the cli
   needs this extension (as opposed to switching to the inferior and using plain "detach").

> Is the infcmd.c change OK?
> 
> - Volodya
> 
> From 8c0569ee17d85cf591426a0b9900d8cad0e02389 Mon Sep 17 00:00:00 2001
> 
> 	* infcmd.c (_initialize_infcmd): Make 'detach' accept
> 	parameter.
> 	* mi/mi-cmds.c (mi_cmds): Make '-target-detach' pass parameter
> 	to 'detach'.
> 	* remote.c (remote_detach_1): Interpret the passed
> 	parameter as a pid to detach from.
> ---
>  gdb/infcmd.c     |    2 +-
>  gdb/mi/mi-cmds.c |    2 +-
>  gdb/remote.c     |   13 +++++++++++--
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index b3af31f..76b48ae 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2556,7 +2556,7 @@ to specify the program, and to load its symbol table."));
>  Detach a process or file previously attached.\n\
>  If a process, it is no longer traced, and it continues its execution.  If\n\
>  you were debugging a file, the file is closed and gdb no longer accesses it."),
> -		  &detachlist, "detach ", 0, &cmdlist);
> +		  &detachlist, "detach ", 1, &cmdlist);
>  
>    add_com ("disconnect", class_run, disconnect_command, _("\
>  Disconnect from a target.\n\
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index d38de35..708dcf8 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -121,7 +121,7 @@ struct mi_cmd mi_cmds[] =
>    { "symbol-type", { NULL, 0 }, NULL },
>    { "target-attach", { "attach", 1 }, NULL },
>    { "target-compare-sections", { NULL, 0 }, NULL },
> -  { "target-detach", { "detach", 0 }, 0 },
> +  { "target-detach", { "detach", 1 }, 0 },
>    { "target-disconnect", { "disconnect", 0 }, 0 },
>    { "target-download", { "load", 1 }, NULL},
>    { "target-exec-status", { NULL, 0 }, NULL },
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5cb36b8..b1adfdb 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3261,11 +3261,20 @@ remote_open_1 (char *name, int from_tty, struct target_ops *target, int extended
>  static void
>  remote_detach_1 (char *args, int from_tty, int extended)
>  {
> -  int pid = ptid_get_pid (inferior_ptid);
> +  int pid;
>    struct remote_state *rs = get_remote_state ();
>  
>    if (args)
> -    error (_("Argument given to \"detach\" when remotely debugging."));
> +    {      
> +      char *end = args;
> +      pid = strtol (args, &end, 10);
> +      if (*end != '\0')
> +	error (_("Cannot parse process id '%s'"), args);
> +      if (!in_inferior_list (pid))
> +	error (_("Invalid process id %d"), pid);
> +    }
> +  else
> +    pid = ptid_get_pid (inferior_ptid);
>  
>    if (!target_has_execution)
>      error (_("No process to detach from."));



-- 
Pedro Alves

Re: [RFA] Implement 'detach pid'.

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On Thursday 13 November 2008 02:19:57 Pedro Alves wrote:

> On Wednesday 12 November 2008 20:39:02, Vladimir Prus wrote:
> > This patch makes CLI 'detach' and MI '-target-detach' accept the PID
> > of the process to detach from.
>
> I see several issues with this patch:
>
>  - The target is not the right layer to do this.  Before you reach here,
>    you've already done things to the current inferior.  E.g.,
> target.c:target_detach will call remove_breakpoints before reaching
> remote_detach_1.  This means that, if you have e.g., selected inferior 1,
> and do detach 3, you'll remove breakpoints from inferior 1, and detach
> process 3.  Breakpoints are an example.  Other example is that before you
> reach the process stratum, you can pass by the thread_stratum, which again
> would do anything to the wrong detachee, since it's usually the
> process_statum that does the final real detach.  That's bad.

How about the attached -- where mostly everything is done on MI side?

- Volodya


[-- Attachment #2: detach.diff --]
[-- Type: text/x-patch, Size: 4512 bytes --]

commit da4c2b7d1163dfd36c492f9f793c04f9bdb36d2b
Author: vladimir <vladimir@e7755896-6108-0410-9592-8049d3e74e28>
Date:   Thu Jul 31 13:23:59 2008 +0000

    Implement '-target-detach pid'.
    
    	* gdbthread.h (thread_callback_func): Clarify comment.
    	* infcmd.c (detach_command): Make nonstatic.
    	* inferior.h (detach_command): Declare.
    	* mi/mi-cmds.c (mi_cmds): Don't route -target-detach via CLI.
    	* mi/mi-cmds.h (mi_cmd_target_detach): Declare.
    	* /mi/mi-main.c (find_thread_of_process, mi_cmd_target_detach): New.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index cac20f7..1a65aa8 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -229,7 +229,9 @@ struct thread_info *find_thread_id (int num);
 void thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid);
 
 /* Iterator function to call a user-provided callback function
-   once for each known thread.  */
+   once for each known thread.  Returns 0 to continue iteration,
+   and 1 to stop -- which causes iterate_over_threads to return
+   the current thread_info.  */
 typedef int (*thread_callback_func) (struct thread_info *, void *);
 extern struct thread_info *iterate_over_threads (thread_callback_func, void *);
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b3af31f..810b3b7 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -86,8 +86,6 @@ static void unset_command (char *, int);
 
 static void float_info (char *, int);
 
-static void detach_command (char *, int);
-
 static void disconnect_command (char *, int);
 
 static void unset_environment_command (char *, int);
@@ -2344,7 +2342,7 @@ attach_command (char *args, int from_tty)
  * started via the normal ptrace (PTRACE_TRACEME).
  */
 
-static void
+void
 detach_command (char *args, int from_tty)
 {
   dont_repeat ();		/* Not for the faint of heart.  */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 1be6cc5..f004d44 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -269,6 +269,8 @@ extern void interrupt_target_command (char *args, int from_tty);
 
 extern void interrupt_target_1 (int all_threads);
 
+extern void detach_command (char *, int);
+
 /* Address at which inferior stopped.  */
 
 extern CORE_ADDR stop_pc;
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index d38de35..51c720e 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -121,7 +121,7 @@ struct mi_cmd mi_cmds[] =
   { "symbol-type", { NULL, 0 }, NULL },
   { "target-attach", { "attach", 1 }, NULL },
   { "target-compare-sections", { NULL, 0 }, NULL },
-  { "target-detach", { "detach", 0 }, 0 },
+  { "target-detach", { NULL, 0 }, mi_cmd_target_detach },
   { "target-disconnect", { "disconnect", 0 }, 0 },
   { "target-download", { "load", 1 }, NULL},
   { "target-exec-status", { NULL, 0 }, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index a9bb1e0..a399b9e 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -75,6 +75,7 @@ extern mi_cmd_argv_ftype mi_cmd_stack_list_frames;
 extern mi_cmd_argv_ftype mi_cmd_stack_list_locals;
 extern mi_cmd_argv_ftype mi_cmd_stack_select_frame;
 extern mi_cmd_argv_ftype mi_cmd_symbol_list_lines;
+extern mi_cmd_argv_ftype mi_cmd_target_detach;
 extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 43ec0b4..36c0223 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -199,6 +199,42 @@ mi_cmd_exec_interrupt (char *command, char **argv, int argc)
     error ("Usage: -exec-interrupt [--all]");
 }
 
+static int
+find_thread_of_process (struct thread_info *ti, void *p)
+{
+  int pid = *(int *)p;
+  if (PIDGET (ti->ptid) == pid && !is_exited (ti->ptid))
+    return 1;
+
+  return 0;
+}
+
+void
+mi_cmd_target_detach (char *command, char **argv, int argc)
+{
+  if (argc != 0 && argc != 1)
+    error ("Usage: -target-detach [thread-group]");
+
+  if (argc == 1)
+    {
+      struct thread_info *tp;
+      char *end = argv[0];
+      int pid = strtol (argv[0], &end, 10);
+      if (*end != '\0')
+	error (_("Cannot parse thread group id '%s'"), argv[0]);
+
+      /* Pick any thread in the desired process.  Current
+	 target_detach deteches from the parent of inferior_ptid.  */
+      tp = iterate_over_threads (find_thread_of_process, &pid);
+      if (!tp)
+	error (_("Thread group is empty"));
+
+      switch_to_thread (tp->ptid);
+    }
+
+  detach_command (NULL, 0);
+}
+
 void
 mi_cmd_thread_select (char *command, char **argv, int argc)
 {

Re: [RFA] Implement 'detach pid'.

[Pedro Alves]

On Thursday 13 November 2008 21:58:49, Vladimir Prus wrote:

>  /* Iterator function to call a user-provided callback function
> -   once for each known thread.  */
> +   once for each known thread.  Returns 0 to continue iteration,
> +   and 1 to stop -- which causes iterate_over_threads to return
> +   the current thread_info.  */

To be honest, this sounds a bit confusing to me: the iterator function,
which is what was being described in the first sentence isn't the
same subject of the second sentence, "returns 0 ..." is now talking
about the callback.

>  typedef int (*thread_callback_func) (struct thread_info *, void *);
>  extern struct thread_info *iterate_over_threads (thread_callback_func, void *);

Notice that a few gdbthread.h exported functions are also
documented in thread.c.  In this case:

/*
 * Thread iterator function.
 *
 * Calls a callback function once for each thread, so long as
 * the callback function returns false.  If the callback function
 * returns true, the iteration will end and the current thread
 * will be returned.  This can be useful for implementing a 
 * search for a thread with arbitrary attributes, or for applying
 * some operation to every thread.
 *
 */

If we're moving the descriptions around, please, let's do
that as a separate patch.

> +void
> +mi_cmd_target_detach (char *command, char **argv, int argc)
> +{
> +  if (argc != 0 && argc != 1)
> +    error ("Usage: -target-detach [thread-group]");
> +
> +  if (argc == 1)
> +    {
> +      struct thread_info *tp;
> +      char *end = argv[0];
> +      int pid = strtol (argv[0], &end, 10);
> +      if (*end != '\0')
> +       error (_("Cannot parse thread group id '%s'"), argv[0]);
> +
> +      /* Pick any thread in the desired process.  Current
> +        target_detach deteches from the parent of inferior_ptid.  */
                            ^ detaches.

> +      tp = iterate_over_threads (find_thread_of_process, &pid);
> +      if (!tp)
> +       error (_("Thread group is empty"));

Yep, something like that.

I take that it's OK then to not revert back to the
previous thread?

Thanks!

-- 
Pedro Alves

Re: [RFA] Implement 'detach pid'.

[Vladimir Prus]

On Friday 14 November 2008 01:49:17 Pedro Alves wrote:
> On Thursday 13 November 2008 21:58:49, Vladimir Prus wrote:
> >  /* Iterator function to call a user-provided callback function
> > -   once for each known thread.  */
> > +   once for each known thread.  Returns 0 to continue iteration,
> > +   and 1 to stop -- which causes iterate_over_threads to return
> > +   the current thread_info.  */
>
> To be honest, this sounds a bit confusing to me: the iterator function,
> which is what was being described in the first sentence isn't the
> same subject of the second sentence, "returns 0 ..." is now talking
> about the callback.
>
> >  typedef int (*thread_callback_func) (struct thread_info *, void *);
> >  extern struct thread_info *iterate_over_threads (thread_callback_func,
> > void *);
>
> Notice that a few gdbthread.h exported functions are also
> documented in thread.c.  In this case:
>
> /*
>  * Thread iterator function.
>  *
>  * Calls a callback function once for each thread, so long as
>  * the callback function returns false.  If the callback function
>  * returns true, the iteration will end and the current thread
>  * will be returned.  This can be useful for implementing a
>  * search for a thread with arbitrary attributes, or for applying
>  * some operation to every thread.
>  *
>  */
>
> If we're moving the descriptions around, please, let's do
> that as a separate patch.

Ok, I'll drop this part. I can never get accustomed to the practice to
documenting interface inside implementation files :-)

> > +void
> > +mi_cmd_target_detach (char *command, char **argv, int argc)
> > +{
> > +  if (argc != 0 && argc != 1)
> > +    error ("Usage: -target-detach [thread-group]");
> > +
> > +  if (argc == 1)
> > +    {
> > +      struct thread_info *tp;
> > +      char *end = argv[0];
> > +      int pid = strtol (argv[0], &end, 10);
> > +      if (*end != '\0')
> > +       error (_("Cannot parse thread group id '%s'"), argv[0]);
> > +
> > +      /* Pick any thread in the desired process.  Current
> > +        target_detach deteches from the parent of inferior_ptid.  */
>
>                             ^ detaches.
>
> > +      tp = iterate_over_threads (find_thread_of_process, &pid);
> > +      if (!tp)
> > +       error (_("Thread group is empty"));
>
> Yep, something like that.
>
> I take that it's OK then to not revert back to the
> previous thread?

Yes, --thread work the same way.

- Volodya