Re: [RFA] Implement 'detach pid'.

[Pedro Alves <pedro at codesourcery dot com>]

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