[MI][patch] broken -target-detach

[MI][patch] broken -target-detach

[Marc Khouzam]

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 ]

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?

Thanks

Marc


You can see the error in the session below:

GNU gdb (GDB) 7.2.50.20100927-cvs
(gdb) interpreter-exec mi "-target-attach --thread-group i1 12283"
^done,frame={addr="0x00f2b422",func="__kernel_vsyscall",args=[]}
(gdb) 
(gdb)  interpreter-exec mi "-list-thread-groups"
^done,groups=[{id="i1",type="process",pid="12283",executable="/home/lmckhou/testing/a1",cores=["0"]}]
(gdb) 
(gdb) interpreter-exec mi "-target-detach a a"
^error,msg="Usage: -target-detach [thread-group]"
(gdb) 
(gdb) interpreter-exec mi "-target-detach i1"
^error,msg="Cannot parse thread group id 'i1'"
(gdb) interpreter-exec mi "-target-detach --thread-group i1"
^done

Patch:

### 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    27 Sep 2010 17:18:21 -0000
@@ -418,19 +418,36 @@
 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];
+      /* First see if we are dealing with a pid */
       int pid = strtol (argv[0], &end, 10);
 
       if (*end != '\0')
-       error (_("Cannot parse thread group id '%s'"), argv[0]);
+       {
+         int id;
+         struct inferior *inf;
+         /* We are not dealing with a pid.  Check for group id */
+         if (*(argv[0]) != 'i')
+           error (_("Invalid pid or thread-group id '%s'"), argv[0]);
+
+         id = strtoul (argv[0] + 1, &end, 0);
+         if (*end != '\0')
+           error (_("Invalid syntax of group id '%s'"), argv[0]);
+
+         inf = find_inferior_id (id);
+         if (!inf)
+           error (_("Non-existent thread-group id '%d'"), id);
+
+         pid = inf->pid;
+       }
 
       /* 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"));

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

[Marc Khouzam]

More on this below.

On September 27, 2010 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 ]
> 
> 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.

After giving it some thought, I believe this 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.

Using the --thread-group flag (alike -thread and --frame) tells the MI
command to focus on the thread-group specified and then execute the
command.  

-target-detach with no parameter means to detach from the process
currently in focus.  The --thread-group flags is meant exactly for such cases,
to avoid having to use a separate command to change the process in focus
and then issuing -target-detach.

On the other hand, it also makes sense to use -target-detach <thread-group>
to specify which thread-group to detach from, instead of the current one.

So, although "-target-detach --thread-group <gid>" and
"-target-detach <gid>" have the same effect, they both make sense
for a frontend.  Even "-target-detach --thread-group <gid> <gid2>",
makes sense, since a frontend may be focused on one process but may
want to detach from another.

Therefore the patch below seems like the right approach, which is to allow
-target-detach to work as it is documented:

-target-detach [pid | gid ]

I think this fix should go into the 7.2 branch as well.

What do you say?

Thanks

Marc


> You can see the error in the session below:
> 
> GNU gdb (GDB) 7.2.50.20100927-cvs
> (gdb) interpreter-exec mi "-target-attach --thread-group i1 12283"
> ^done,frame={addr="0x00f2b422",func="__kernel_vsyscall",args=[]}
> (gdb)
> (gdb)  interpreter-exec mi "-list-thread-groups"
> ^done,groups=[{id="i1",type="process",pid="12283",executable="/home/lmckhou/testing/a1",cores=["0"]}]
> (gdb)
> (gdb) interpreter-exec mi "-target-detach a a"
> ^error,msg="Usage: -target-detach [thread-group]"
> (gdb)
> (gdb) interpreter-exec mi "-target-detach i1"
> ^error,msg="Cannot parse thread group id 'i1'"
> (gdb) interpreter-exec mi "-target-detach --thread-group i1"
> ^done
> 
> Patch:


### 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    27 Sep 2010 17:18:21 -0000
@@ -418,19 +418,36 @@
 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];
+      /* First see if we are dealing with a pid */
       int pid = strtol (argv[0], &end, 10);

       if (*end != '\0')
-       error (_("Cannot parse thread group id '%s'"), argv[0]);
+       {
+         int id;
+         struct inferior *inf;
+         /* We are not dealing with a pid.  Check for group id */
+         if (*(argv[0]) != 'i')
+           error (_("Invalid pid or thread-group id '%s'"), argv[0]);
+
+         id = strtoul (argv[0] + 1, &end, 0);
+         if (*end != '\0')
+           error (_("Invalid syntax of group id '%s'"), argv[0]);
+
+         inf = find_inferior_id (id);
+         if (!inf)
+           error (_("Non-existent thread-group id '%d'"), id);
+
+         pid = inf->pid;
+       }

       /* 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"));

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

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

I'm sorry about the delay.

I recommend pinging your patches weekly.

Marc> Using the --thread-group flag (alike -thread and --frame) tells the MI
Marc> command to focus on the thread-group specified and then execute the
Marc> command.  

It seems reasonable to me.

Marc> +         /* We are not dealing with a pid.  Check for group id */
Marc> +         if (*(argv[0]) != 'i')
Marc> +           error (_("Invalid pid or thread-group id '%s'"), argv[0]);
Marc> +
Marc> +         id = strtoul (argv[0] + 1, &end, 0);
Marc> +         if (*end != '\0')
Marc> +           error (_("Invalid syntax of group id '%s'"), argv[0]);

This code is duplicated in various spots in MI.  Seems ugly -- but not
your problem.


I think this patch looks good.  It needs a ChangeLog entry.  A test case
would be nice to have.

I think it would be better for Volodya to review it, since he is the MI
maintainer.  If he doesn't review it within a week or so, please ping it
and I will approve it.

thanks,
Tom

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

[Pedro Alves]

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...

> 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.)

-- 
Pedro Alves

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

[Marc Khouzam]

> -----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"));

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

[Pedro Alves]

On Friday 12 November 2010 16:01:20, Marc Khouzam wrote:

> > 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?

IMO, it's too late for 7.2.  Having 7.2 and 7.2.1 handle this
command's argument incompatibly would cause pain for frontends
already passing it an argument, and we can avoid causing that pain.
So IMO, since it's quite easy to do so, we should continue handling it.
We can consider mentioning in the manual that the PID form
is deprecated, and that it may be removed in a later release.
Then we can consider removing it in the future if it ever causes
trouble.  (I'm not suggesting you do that.)

> I knew someone was going to call me on that :-).  Here is the new
> patch which still accepts PID.  

Sorry about that.  :-)

> 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.

It sure is, thanks for persevering.

> Good for the 7.2 branch too?

IMO, yes.

You've just given me a new chance to nit on formatting, though! :-)

> +      /* First see if we are dealing with a thread-group id */

Period and double space at end of sentence, like:

 /* First see if we are dealing with a thread-group id.  */


> +      if (*(argv[0]) == 'i')

     if (*argv[0] == 'i')

Okay with those changes.

-- 
Pedro Alves

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

[Marc Khouzam]

> -----Original Message-----
> From: Pedro Alves [mailto:pedro@codesourcery.com] 
> Sent: Friday, November 12, 2010 11:44 AM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'; 'Tom Tromey'
> Subject: Re: [MI][patch] broken -target-detach
> 
> On Friday 12 November 2010 16:01:20, Marc Khouzam wrote:
> 
> > > 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?
> 
> IMO, it's too late for 7.2.  Having 7.2 and 7.2.1 handle this
> command's argument incompatibly would cause pain for frontends
> already passing it an argument, and we can avoid causing that pain.
> So IMO, since it's quite easy to do so, we should continue 
> handling it.
> We can consider mentioning in the manual that the PID form
> is deprecated, and that it may be removed in a later release.
> Then we can consider removing it in the future if it ever causes
> trouble.  (I'm not suggesting you do that.)
> 
> > I knew someone was going to call me on that :-).  Here is the new
> > patch which still accepts PID.  
> 
> Sorry about that.  :-)
> 
> > 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.
> 
> It sure is, thanks for persevering.
> 
> > Good for the 7.2 branch too?
> 
> IMO, yes.
> 
> You've just given me a new chance to nit on formatting, though! :-)
> 
> > +      /* First see if we are dealing with a thread-group id */
> 
> Period and double space at end of sentence, like:
> 
>  /* First see if we are dealing with a thread-group id.  */
> 
> 
> > +      if (*(argv[0]) == 'i')
> 
>      if (*argv[0] == 'i')
> 
> Okay with those changes.

Great!
I committed the following to HEAD and 7_2:

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.178.2.2
diff -u -r1.178.2.2 mi-main.c
--- gdb/mi/mi-main.c    1 Sep 2010 19:16:00 -0000       1.178.2.2
+++ gdb/mi/mi-main.c    12 Nov 2010 19:00:48 -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"));