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

[Pedro Alves <palves@redhat.com>]

On 12/31/2017 05:50 AM, Simon Marchi wrote:
> 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.
> 

Eh.  I knew about "detach SIG", from touching the detach-related
code many times before.  Had no idea that it was broken.

I think the main use cases are/were:

 - To be able to detach without passing the just-intercepted signal
   to the program.  I.e., "detach 0" was to "detach", like "signal 0"
   is to "continue".

 - To be able to attach to a program (maybe job-stopped), debug it
   a bit, and then detach it, leaving it job-stopped.  I.e., queue
   a SIGSTOP when you detach, with "detach SIGTOP".

I think you can do the former today with either:
  "handle SIGFOO nopass" + "detach"
  "queue-signal 0" + "detach"

and the latter with either:
  "queue-signal SIGSTOP" + "detach"
  "shell kill -SIGSTOP $PID" + "detach"

The "shell kill" variant is a bit more cumbersome, of
course, and only works with native debugging.

We're probably missing testcases for the above scenarios.
IWBN to add them, of course.

The patch looks fine to me.  One nit:

>  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 (0);

I think the 'signo' parameter of do_detach is now always
0, so it could be removed.  I understand that you might
want to avoid breaking the port (too much), but it looks
very easy to remove.

>  
>    inferior_ptid = null_ptid;
>    detach_inferior (pid);

Thanks,
Pedro Alves