RE: [MI][patch] broken -target-detach

[Marc Khouzam <marc dot khouzam at ericsson dot com>]

> -----Original Message-----
> From: Pedro Alves [mailto:pedro@codesourcery.com] 
> Sent: Friday, October 15, 2010 7:47 PM
> To: gdb-patches@sourceware.org
> Cc: Marc Khouzam
> Subject: Re: [MI][patch] broken -target-detach
> 
> On Monday 27 September 2010 21:25:27, Marc Khouzam wrote:
> > Hi,
> > 
> > with GDB 7.2, the MI command -target-detach is not working 
> very well.
> > It still assumes that the thread-group id is a pid, instead of the
> > new thread-group id format which starts with an 'i'.
> > Also, the usage printout does not correspond to the documentation:
> > 
> > Usage: -target-detach [thread-group]
> > vs
> > -target-detach [ pid | gid ]
> 
> Yeah.  I think it used to correspond implicitly, since the thread
> group id in 7.0 and 7.1 was actually equal to the pid, IIRC.  With
> 7.2, the 1-1 correspondence disappeared, but this command appears
> to have been forgotten.
> 
> One would hope that frontends would stop using the PID form,
> cause you may want to detach from targets that don't have
> a PID concept at all, and even though GDB fakes a PID for
> you today in such cases, it's better to not assume that.
> 
> Unfortunately, 7.2 was released accepting the PID form only,
> so we may be better off continue accepting it...

If we put this fix in the 7.2 branch, could we get rid of the
PID form or is it too late?

> > I have a patch that fixes things to parse both a pid or a 
> thread-group.
> > I've added it at the bottom, but I'm not sure it is the 
> right approach.
> > With the new global MI flag --thread-group, I wonder if 
> -target-detach
> > should take a thread-group as a parameter anymore.
> > Note that although "-target-detach i1" does not work,
> > "-target-detach --thread-group i1" works.
> > I'm not sure what would happen if I did:
> > "-target-detach --thread-group i1 <pid or threadGroupId>"
> > I'm guessing the --thread-group flag would get ignored.
> > 
> > What should we do about this?
> 
> FWIW, codewise, your patch also looked good to me.
> (though IMO it'd be clearer to check for *argv[0] == 'i' even
> before trying to parse a number with strtol.)

I knew someone was going to call me on that :-).  Here is the new
patch which still accepts PID.  I have to apologize, I just don't
have the time to get a test case for it.  I hope the patch is useful
enough as it is.

If you feel we can get rid of PID, just let me know.

Good for the 7.2 branch too?

Thanks

Marc

2010-11-12  Marc Khouzam  <marc.khouzam@ericsson.com>

        * mi/mi-main.c (mi_cmd_target_detach): Accept new
        thread-group id format.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.181
diff -u -r1.181 mi-main.c
--- gdb/mi/mi-main.c    1 Sep 2010 19:03:54 -0000       1.181
+++ gdb/mi/mi-main.c    12 Nov 2010 15:56:16 -0000
@@ -418,19 +418,40 @@
 mi_cmd_target_detach (char *command, char **argv, int argc)
 {
   if (argc != 0 && argc != 1)
-    error ("Usage: -target-detach [thread-group]");
+    error ("Usage: -target-detach [pid | thread-group]");
 
   if (argc == 1)
     {
       struct thread_info *tp;
       char *end = argv[0];
-      int pid = strtol (argv[0], &end, 10);
+      int pid;
 
-      if (*end != '\0')
-       error (_("Cannot parse thread group id '%s'"), argv[0]);
+      /* First see if we are dealing with a thread-group id */
+      if (*(argv[0]) == 'i')
+       {
+         struct inferior *inf;
+         int id = strtoul (argv[0] + 1, &end, 0);
+
+         if (*end != '\0')
+           error (_("Invalid syntax of thread-group id '%s'"), argv[0]);
+
+         inf = find_inferior_id (id);
+         if (!inf)
+           error (_("Non-existent thread-group id '%d'"), id);
+
+         pid = inf->pid;
+       }
+      else
+       {
+         /* We must be dealing with a pid */
+         pid = strtol (argv[0], &end, 10);
+
+         if (*end != '\0')
+           error (_("Invalid identifier '%s'"), argv[0]);
+       }
 
       /* Pick any thread in the desired process.  Current
-        target_detach deteches from the parent of inferior_ptid.  */
+        target_detach detaches from the parent of inferior_ptid.  */
       tp = iterate_over_threads (find_thread_of_process, &pid);
       if (!tp)
        error (_("Thread group is empty"));