[MI non-stop 04/11] Implement --thread and --frame.

[MI non-stop 04/11] Implement --thread and --frame.

[Vladimir Prus]

This implements the --thread and --frame options to all MI command.
Please see http://article.gmane.org/gmane.comp.gdb.devel/23414/ for
background design for --thread. The --frame is ideologically same
as --thread.

The only non-MI change here is making find_thread_pid exported from
thread.c, which change seems obvious, so no approval is needed. Comments,
however, are much appreciated.

- Volodya

	* gdbthread.h (find_thread_id): Declare.
	* thread.c (find_thread_id): Make non-static.
	* mi/mi-main.c (mi_cmd_execute): Switch to the right
	thread and frame, if necessary.
	* mi/mi-parse.c (mi_parse): Handle --thread and --frame.
	* mi/mi-parse.h (struct mi_parse): New fields thread and frame.
---
 gdb/gdbthread.h   |    3 +++
 gdb/mi/mi-main.c  |   32 ++++++++++++++++++++++++++++++++
 gdb/mi/mi-parse.c |   34 ++++++++++++++++++++++++++++++++++
 gdb/mi/mi-parse.h |    2 ++
 gdb/thread.c      |    4 +---
 5 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index a061d86..878e44f 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -138,6 +138,9 @@ extern int valid_thread_id (int thread);
 /* Search function to lookup a thread by 'pid'.  */
 extern struct thread_info *find_thread_pid (ptid_t ptid);
 
+/* Find thread by GDB user-visible thread number.  */
+struct thread_info *find_thread_id (int num);
+
 /* Iterator function to call a user-provided callback function
    once for each known thread.  */
 typedef int (*thread_callback_func) (struct thread_info *, void *);
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 3c92980..61ef5a4 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1123,6 +1123,10 @@ static void
 mi_cmd_execute (struct mi_parse *parse)
 {
   struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+  char *thread_str;
+  char *frame_str;
+  int thread;
+  int i;
   free_all_values ();
 
   /* Switch to the frontend selected thread, before invoking the
@@ -1135,6 +1139,33 @@ mi_cmd_execute (struct mi_parse *parse)
      the selected thread.  */
   make_cleanup (do_restore_selected_thread_and_frame_cleanup, NULL);
 
+  if (parse->frame != -1 && !parse->thread == -1)
+    error ("Cannot specify --frame without --thread");
+  
+  if (parse->thread != -1)
+    {
+      struct thread_info *tp = find_thread_id (parse->thread);
+      if (!tp)
+	error (_("Invalid thread id: %d"), parse->thread);
+      
+      if (non_stop)
+	context_switch_to (tp->ptid);
+      else
+	switch_to_thread (tp->ptid);
+    }
+  
+  if (parse->frame != -1)
+    {
+      struct frame_info *fid;
+      int frame = parse->frame;
+      fid = find_relative_frame (get_current_frame (), &frame);
+      if (frame == 0)
+	/* find_relative_frame was successful */
+	select_frame (fid);
+      else
+	error (_("Invalid frame id: %s"), frame_str);
+    }
+  
   if (parse->cmd->argv_func != NULL)
     {
       if (target_can_async_p ()
@@ -1171,6 +1202,7 @@ mi_cmd_execute (struct mi_parse *parse)
 	      error_stream (stb);
 	    }
 	}
+
       current_token = xstrdup (parse->token);
       cleanup = make_cleanup (free_current_contents, &current_token);
       parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
index a2dc50d..835fb74 100644
--- a/gdb/mi/mi-parse.c
+++ b/gdb/mi/mi-parse.c
@@ -150,6 +150,8 @@ mi_parse (char *cmd)
   char *chp;
   struct mi_parse *parse = XMALLOC (struct mi_parse);
   memset (parse, 0, sizeof (*parse));
+  parse->thread = -1;
+  parse->frame = -1;
 
   /* Before starting, skip leading white space. */
   while (isspace (*cmd))
@@ -199,6 +201,38 @@ mi_parse (char *cmd)
   while (isspace (*chp))
     chp++;
 
+  /* Parse the --thread and --frame options, if present.  At present,
+     some important commands, like '-break-*' are implemented by forwarding
+     to the CLI layer directly.  We want to parse --thread and --frame
+     here, so as not to leave those option in the string that will be passed
+     to CLI.  */
+  for (;;)
+    {
+      char *start = chp;
+      if (strncmp (chp, "--thread", 8) == 0)
+	{
+	  if (parse->thread != -1)
+	    error ("Duplicate '--thread' option");
+	  chp += 8;
+	  parse->thread = strtol (chp, &chp, 10);
+	}
+      else if (strncmp (chp, "--frame", 7) == 0)
+	{
+	  if (parse->frame != -1)
+	    error ("Duplicate '--frame' option");
+	  chp += 7;
+	  parse->frame = strtol (chp, &chp, 10);
+	}
+      else
+	break;
+
+      if (*chp != '\0' && !isspace (*chp))
+	error ("Invalid value for the '%s' option",
+	       start[2] == 't' ? "--thread" : "--frame");
+      while (isspace (*chp))
+	chp++;
+    }
+
   /* For new argv commands, attempt to return the parsed argument
      list. */
   if (parse->cmd->argv_func != NULL)
diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
index 945c42d..aa262f5 100644
--- a/gdb/mi/mi-parse.h
+++ b/gdb/mi/mi-parse.h
@@ -46,6 +46,8 @@ struct mi_parse
     char *args;
     char **argv;
     int argc;
+    int thread;
+    int frame;
   };
 
 /* Attempts to parse CMD returning a ``struct mi_command''.  If CMD is
diff --git a/gdb/thread.c b/gdb/thread.c
index 4e172d8..be0d175 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -55,8 +55,6 @@ void _initialize_thread (void);
 static struct thread_info *thread_list = NULL;
 static int highest_thread_num;
 
-static struct thread_info *find_thread_id (int num);
-
 static void thread_command (char *tidstr, int from_tty);
 static void thread_apply_all_command (char *, int);
 static int thread_alive (struct thread_info *);
@@ -275,7 +273,7 @@ delete_thread (ptid_t ptid)
   free_thread (tp);
 }
 
-static struct thread_info *
+struct thread_info *
 find_thread_id (int num)
 {
   struct thread_info *tp;
-- 
1.5.3.5

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sat, 28 Jun 2008 20:44:14 +0400
> 
> 
> +  if (parse->frame != -1 && !parse->thread == -1)
> +    error ("Cannot specify --frame without --thread");

Why is this error message not in _(), while all the rest are?

Btw, do we at all want error messages issued by MI commands
translated?  MI commands are invoked by a program, so error messages
we generate should be understandable by a program, which probably
means they should not be translated.

> +      if (strncmp (chp, "--thread", 8) == 0)

Please, let's not use literal constants in this context, let's use
sizeof instead.

> +	  parse->frame = strtol (chp, &chp, 10);

Do we really want to disallow non-decimal numbers here?  What about
hex frame numbers?

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Vladimir Prus]

Eli Zaretskii wrote:

>> From: Vladimir Prus <vladimir@codesourcery.com>
>> Date: Sat, 28 Jun 2008 20:44:14 +0400
>> 
>> 
>> +  if (parse->frame != -1 && !parse->thread == -1)
>> +    error ("Cannot specify --frame without --thread");
> 
> Why is this error message not in _(), while all the rest are?
> 
> Btw, do we at all want error messages issued by MI commands
> translated?  

I think we don't, but I'm not sure.


> MI commands are invoked by a program, so error messages 
> we generate should be understandable by a program, which probably
> means they should not be translated.

It's a bit questionable. For example, the error you mention above
is clearly a bug in frontend. Presenting a translated version of
that message to the user is essentially pointless. On the other
hand, "Thread is running", or "Memory not accessible" messages
can be helpful for users. Do we need two error messages, maybe?

>> +      if (strncmp (chp, "--thread", 8) == 0)
> 
> Please, let's not use literal constants in this context, let's use
> sizeof instead.

sizeof? For all I know, sizeof("--thread") will be wrong here.

> 
>> +      parse->frame = strtol (chp, &chp, 10);
> 
> Do we really want to disallow non-decimal numbers here?  What about
> hex frame numbers?

Why would frontend want to specify frame level in hex?

- Volodya

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Eli Zaretskii]

> From:  Vladimir Prus <vladimir@codesourcery.com>
> Date:  Sat, 28 Jun 2008 21:34:57 +0400
> 
> Eli Zaretskii wrote:
> 
> >> From: Vladimir Prus <vladimir@codesourcery.com>
> >> Date: Sat, 28 Jun 2008 20:44:14 +0400
> >> 
> >> 
> >> +  if (parse->frame != -1 && !parse->thread == -1)
> >> +    error ("Cannot specify --frame without --thread");
> > 
> > Why is this error message not in _(), while all the rest are?
> > 
> > Btw, do we at all want error messages issued by MI commands
> > translated?  
> 
> I think we don't, but I'm not sure.
> 
> 
> > MI commands are invoked by a program, so error messages 
> > we generate should be understandable by a program, which probably
> > means they should not be translated.
> 
> It's a bit questionable. For example, the error you mention above
> is clearly a bug in frontend. Presenting a translated version of
> that message to the user is essentially pointless. On the other
> hand, "Thread is running", or "Memory not accessible" messages
> can be helpful for users. Do we need two error messages, maybe?

Maybe.  Do front ends show the error messages to the user, or do they
act on them themselves (or both)?

> >> +      if (strncmp (chp, "--thread", 8) == 0)
> > 
> > Please, let's not use literal constants in this context, let's use
> > sizeof instead.
> 
> sizeof? For all I know, sizeof("--thread") will be wrong here.

Why would it be wrong?

> >> +      parse->frame = strtol (chp, &chp, 10);
> > 
> > Do we really want to disallow non-decimal numbers here?  What about
> > hex frame numbers?
> 
> Why would frontend want to specify frame level in hex?

I dunno, does the MI spec mandate decimal here?  If it does, then my
comment is hereby withdrawn.

RE: [MI non-stop 04/11] Implement --thread and --frame.

[Marc Khouzam]

> > 
> > sizeof? For all I know, sizeof("--thread") will be wrong here.
> 
> Why would it be wrong?

sizeof("--thread") seems to count the \0, so you need to do subtract 1.
Another danger is that if someone decides to convert from sizeof("--thread")
to something like:
  char* str="--thread";
  int len=sizeof(str); 

you are going to the size of a char* (4) inste of the string.
I think strlen() is better.

Marc

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Vladimir Prus]

On Saturday 28 June 2008 22:16:06 Eli Zaretskii wrote:
> > From:  Vladimir Prus <vladimir@codesourcery.com>
> > Date:  Sat, 28 Jun 2008 21:34:57 +0400
> > 
> > Eli Zaretskii wrote:
> > 
> > >> From: Vladimir Prus <vladimir@codesourcery.com>
> > >> Date: Sat, 28 Jun 2008 20:44:14 +0400
> > >> 
> > >> 
> > >> +  if (parse->frame != -1 && !parse->thread == -1)
> > >> +    error ("Cannot specify --frame without --thread");
> > > 
> > > Why is this error message not in _(), while all the rest are?
> > > 
> > > Btw, do we at all want error messages issued by MI commands
> > > translated?  
> > 
> > I think we don't, but I'm not sure.
> > 
> > 
> > > MI commands are invoked by a program, so error messages 
> > > we generate should be understandable by a program, which probably
> > > means they should not be translated.
> > 
> > It's a bit questionable. For example, the error you mention above
> > is clearly a bug in frontend. Presenting a translated version of
> > that message to the user is essentially pointless. On the other
> > hand, "Thread is running", or "Memory not accessible" messages
> > can be helpful for users. Do we need two error messages, maybe?
> 
> Maybe.  Do front ends show the error messages to the user, or do they
> act on them themselves (or both)?

Both, I think. I'd expect that right now, showing the message to the user
is more common than handling it internally -- because it's fairly hard to
guess, from an error message, what is this. For example, KDevelop 3.5, when
operating with pre-6.8 gdb, will handle all errors from -break-insert command 
-- assuming we get an error because the location is in not-yet-loaded shared
library. But, this has the potential to hide real error -- like line number
that is out of range.

It seems like the only solution is to have some set of error codes, which
are never shown to the user, and then have the message string i18ned.

> > >> +      if (strncmp (chp, "--thread", 8) == 0)
> > > 
> > > Please, let's not use literal constants in this context, let's use
> > > sizeof instead.
> > 
> > sizeof? For all I know, sizeof("--thread") will be wrong here.
> 
> Why would it be wrong?

It will give you 9, here.

> > >> +      parse->frame = strtol (chp, &chp, 10);
> > > 
> > > Do we really want to disallow non-decimal numbers here?  What about
> > > hex frame numbers?
> > 
> > Why would frontend want to specify frame level in hex?
> 
> I dunno, does the MI spec mandate decimal here?  If it does, then my
> comment is hereby withdrawn.

The spec does not mandate decimal, or allow hex -- yet. Other commands that
take numbers are also silent, however the code appears to use decimal. For
example, -stack-list-args and friends use atoi -- which is 10-base.

- Volodya

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sun, 29 Jun 2008 10:05:14 +0400
> Cc: gdb-patches@sources.redhat.com
> 
> > > >> +      if (strncmp (chp, "--thread", 8) == 0)
> > > > 
> > > > Please, let's not use literal constants in this context, let's use
> > > > sizeof instead.
> > > 
> > > sizeof? For all I know, sizeof("--thread") will be wrong here.
> > 
> > Why would it be wrong?
> 
> It will give you 9, here.

Well, I hope you trust that I knew this.

> > > >> +      parse->frame = strtol (chp, &chp, 10);
> > > > 
> > > > Do we really want to disallow non-decimal numbers here?  What about
> > > > hex frame numbers?
> > > 
> > > Why would frontend want to specify frame level in hex?
> > 
> > I dunno, does the MI spec mandate decimal here?  If it does, then my
> > comment is hereby withdrawn.
> 
> The spec does not mandate decimal, or allow hex -- yet. Other commands that
> take numbers are also silent, however the code appears to use decimal. For
> example, -stack-list-args and friends use atoi -- which is 10-base.

Then perhaps we should document that it must be decimal.

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Vladimir Prus]

On Sunday 29 June 2008 21:49:52 Eli Zaretskii wrote:

> > > I dunno, does the MI spec mandate decimal here?  If it does, then my
> > > comment is hereby withdrawn.
> > 
> > The spec does not mandate decimal, or allow hex -- yet. Other commands that
> > take numbers are also silent, however the code appears to use decimal. For
> > example, -stack-list-args and friends use atoi -- which is 10-base.
> 
> Then perhaps we should document that it must be decimal.

Yes. Added to my documentation todo.

- Volodya

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Michael Snyder]

On Sat, 2008-06-28 at 21:16 +0300, Eli Zaretskii wrote:
> > From:  Vladimir Prus <vladimir@codesourcery.com>

> > > MI commands are invoked by a program, so error messages 
> > > we generate should be understandable by a program, which probably
> > > means they should not be translated.
> > 
> > It's a bit questionable. For example, the error you mention above
> > is clearly a bug in frontend. Presenting a translated version of
> > that message to the user is essentially pointless. On the other
> > hand, "Thread is running", or "Memory not accessible" messages
> > can be helpful for users. Do we need two error messages, maybe?
> 
> Maybe.  Do front ends show the error messages to the user, or do they
> act on them themselves (or both)?

I usually stay out of MI discussions, 'cause I'm not
well informed, but...

If the error messages could be made a part of the spec, 
then the front end could do its own translating.

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Daniel Jacobowitz]

On Mon, Jun 30, 2008 at 11:17:08AM -0700, Michael Snyder wrote:
> I usually stay out of MI discussions, 'cause I'm not
> well informed, but...
> 
> If the error messages could be made a part of the spec, 
> then the front end could do its own translating.

I don't think we can do this with textual error messages, since we use
them for the CLI - i.e. they are intended to be user-friendly.  We
could add new machine-readable errors, though.

-- 
Daniel Jacobowitz
CodeSourcery

RE: [MI non-stop 04/11] Implement --thread and --frame.

[Marc Khouzam]

Hi,

I don't know enough to thoroughly review so I'll give the minor comments
I have.
 
> +  if (parse->frame != -1 && !parse->thread == -1)

You probably didn't mean to have ! before parse->thread

> +    error ("Cannot specify --frame without --thread");
> +  
> +  if (parse->thread != -1)
> +    {
> +      struct thread_info *tp = find_thread_id (parse->thread);
> +      if (!tp)
> + error (_("Invalid thread id: %d"), parse->thread);
> +      
> +      if (non_stop)
> + context_switch_to (tp->ptid);
> +      else
> +   switch_to_thread (tp->ptid);
> +    }
> +  
> +  if (parse->frame != -1)
> +    {
> +      struct frame_info *fid;
> +      int frame = parse->frame;
> +      fid = find_relative_frame (get_current_frame (), &frame);
> +      if (frame == 0)
> +   /* find_relative_frame was successful */
> +   select_frame (fid);
> +      else
> +   error (_("Invalid frame id: %s"), frame_str);
> +    }
> +  
>    if (parse->cmd->argv_func != NULL)
>      {
>        if (target_can_async_p ()
> @@ -1171,6 +1202,7 @@ mi_cmd_execute (struct mi_parse *parse)
>           error_stream (stb);
>         }
>     }
> +
>        current_token = xstrdup (parse->token);
>        cleanup = make_cleanup (free_current_contents, &current_token);
>        parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
> diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
> index a2dc50d..835fb74 100644
> --- a/gdb/mi/mi-parse.c
> +++ b/gdb/mi/mi-parse.c
> @@ -150,6 +150,8 @@ mi_parse (char *cmd)
>    char *chp;
>    struct mi_parse *parse = XMALLOC (struct mi_parse);
>    memset (parse, 0, sizeof (*parse));
> +  parse->thread = -1;
> +  parse->frame = -1;
>  
>    /* Before starting, skip leading white space. */
>    while (isspace (*cmd))
> @@ -199,6 +201,38 @@ mi_parse (char *cmd)
>    while (isspace (*chp))
>      chp++;
>  
> +  /* Parse the --thread and --frame options, if present.  At present,
> +     some important commands, like '-break-*' are implemented by forwarding
> +     to the CLI layer directly.  We want to parse --thread and --frame
> +     here, so as not to leave those option in the string that will be passed
> +     to CLI.  */
> +  for (;;)
> +    {
> +      char *start = chp;
> +      if (strncmp (chp, "--thread", 8) == 0)

Although this works, if you later add a new param like --thread-list,
it will falsly match here.  Maybe you want to strncmp with "--thread "?
(with a space at the end) 
    
I'm not sure what the coding style is for GDB in this case, but I have
found that it is more maintainable not to hard-code the lengths, but to use
something like
   char* threadStr = "--thread ";
   int len = strlen(threadStr);

> +   {
> +     if (parse->thread != -1)
> +       error ("Duplicate '--thread' option");
> +     chp += 8;
> +     parse->thread = strtol (chp, &chp, 10);
> +   }
> +      else if (strncmp (chp, "--frame", 7) == 0)
> +   {
> +     if (parse->frame != -1)
> +       error ("Duplicate '--frame' option");
> +     chp += 7;
> +     parse->frame = strtol (chp, &chp, 10);
> +   }
> +      else
> +   break;

Are --frame and --thread the only options that can be found when we are
running this code? If yes, then its fine; if no, then you shouldn't break
on else but should skip the uninteresting option.

> +
> +      if (*chp != '\0' && !isspace (*chp))
> +   error ("Invalid value for the '%s' option",
> +          start[2] == 't' ? "--thread" : "--frame");
> +      while (isspace (*chp))
> +   chp++;
> +    }
> +
>    /* For new argv commands, attempt to return the parsed argument

[...]

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Eli Zaretskii]

> Date: Sat, 28 Jun 2008 13:56:21 -0400
> From: "Marc Khouzam" <marc.khouzam@ericsson.com>
> 
> I'm not sure what the coding style is for GDB in this case, but I have
> found that it is more maintainable not to hard-code the lengths, but to use
> something like
>    char* threadStr = "--thread ";
>    int len = strlen(threadStr);

I'd prefer

  char threadStr[] = "--thread ";
  size_t len = sizeof(threadStr) - 1;

which will avoid any runtime penalties.

RE: [MI non-stop 04/11] Implement --thread and --frame.

[Marc Khouzam]

> I'd prefer
> 
>  char threadStr[] = "--thread ";
>  size_t len = sizeof(threadStr) - 1;
>
> which will avoid any runtime penalties.

I like!

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Vladimir Prus]

On Saturday 28 June 2008 21:56:21 Marc Khouzam wrote:
> Hi,
> 
> I don't know enough to thoroughly review so I'll give the minor comments
> I have.
>  
> > +  if (parse->frame != -1 && !parse->thread == -1)
> 
> You probably didn't mean to have ! before parse->thread

Doh! You're right.

> 
> > +    error ("Cannot specify --frame without --thread");
> > +  
> > +  if (parse->thread != -1)
> > +    {
> > +      struct thread_info *tp = find_thread_id (parse->thread);
> > +      if (!tp)
> > + error (_("Invalid thread id: %d"), parse->thread);
> > +      
> > +      if (non_stop)
> > + context_switch_to (tp->ptid);
> > +      else
> > +   switch_to_thread (tp->ptid);
> > +    }
> > +  
> > +  if (parse->frame != -1)
> > +    {
> > +      struct frame_info *fid;
> > +      int frame = parse->frame;
> > +      fid = find_relative_frame (get_current_frame (), &frame);
> > +      if (frame == 0)
> > +   /* find_relative_frame was successful */
> > +   select_frame (fid);
> > +      else
> > +   error (_("Invalid frame id: %s"), frame_str);
> > +    }
> > +  
> >    if (parse->cmd->argv_func != NULL)
> >      {
> >        if (target_can_async_p ()
> > @@ -1171,6 +1202,7 @@ mi_cmd_execute (struct mi_parse *parse)
> >           error_stream (stb);
> >         }
> >     }
> > +
> >        current_token = xstrdup (parse->token);
> >        cleanup = make_cleanup (free_current_contents, &current_token);
> >        parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
> > diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
> > index a2dc50d..835fb74 100644
> > --- a/gdb/mi/mi-parse.c
> > +++ b/gdb/mi/mi-parse.c
> > @@ -150,6 +150,8 @@ mi_parse (char *cmd)
> >    char *chp;
> >    struct mi_parse *parse = XMALLOC (struct mi_parse);
> >    memset (parse, 0, sizeof (*parse));
> > +  parse->thread = -1;
> > +  parse->frame = -1;
> >  
> >    /* Before starting, skip leading white space. */
> >    while (isspace (*cmd))
> > @@ -199,6 +201,38 @@ mi_parse (char *cmd)
> >    while (isspace (*chp))
> >      chp++;
> >  
> > +  /* Parse the --thread and --frame options, if present.  At present,
> > +     some important commands, like '-break-*' are implemented by forwarding
> > +     to the CLI layer directly.  We want to parse --thread and --frame
> > +     here, so as not to leave those option in the string that will be passed
> > +     to CLI.  */
> > +  for (;;)
> > +    {
> > +      char *start = chp;
> > +      if (strncmp (chp, "--thread", 8) == 0)
> 
> Although this works, if you later add a new param like --thread-list,
> it will falsly match here.  Maybe you want to strncmp with "--thread "?
> (with a space at the end) 

Yes, I think this will make the code more robust.
    
> I'm not sure what the coding style is for GDB in this case, but I have
> found that it is more maintainable not to hard-code the lengths, but to use
> something like
>    char* threadStr = "--thread ";
>    int len = strlen(threadStr);

This is nice bikeshed question :-) Personally, I find that what I have is
perfectly maintainable, due to GNU Emacs having Esc-= shortcut -- which
counts the number of characters in a region.

> > +   {
> > +     if (parse->thread != -1)
> > +       error ("Duplicate '--thread' option");
> > +     chp += 8;
> > +     parse->thread = strtol (chp, &chp, 10);
> > +   }
> > +      else if (strncmp (chp, "--frame", 7) == 0)
> > +   {
> > +     if (parse->frame != -1)
> > +       error ("Duplicate '--frame' option");
> > +     chp += 7;
> > +     parse->frame = strtol (chp, &chp, 10);
> > +   }
> > +      else
> > +   break;
> 
> Are --frame and --thread the only options that can be found when we are
> running this code? If yes, then its fine; if no, then you shouldn't break
> on else but should skip the uninteresting option.

I intend for those option to be required to be the first.  This is yet another
issue arising from too many ways to handle MI commands -- for some commands
we parse the command per MI rule, get argc/argv and then can extract options
just fine. Some other commands are passed to CLI, verbatim. But we want --thread
to work for those commands, too. It's a bit risky to grab --thread in the
middle of command, so I look for --thread and --frame at the beginning of the
command, and pass the remainder to CLI.

- Volodya

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sun, 29 Jun 2008 10:03:27 +0400
> Cc: gdb-patches@sources.redhat.com
> 
> >    char* threadStr = "--thread ";
> >    int len = strlen(threadStr);
> 
> This is nice bikeshed question :-) Personally, I find that what I have is
> perfectly maintainable, due to GNU Emacs having Esc-= shortcut -- which
> counts the number of characters in a region.

I think the variant with threadStr[] and sizeof - 1 is more
maintainable, especially since not everyone uses Emacs.  Moreover, a
literal constant makes code a bit harder to read, since I need to
count characters after you, to be sure I understand exactly what your
code does.

Granted, it's a minor nit, but then so are our indentation and
whitespace rules.

(Btw, in stock Emacs "ESC =" counts _lines_ in region, not characters.
You probably have some customization that does this.)

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> I think the variant with threadStr[] and sizeof - 1 is more
Eli> maintainable, especially since not everyone uses Emacs.  Moreover, a
Eli> literal constant makes code a bit harder to read, since I need to
Eli> count characters after you, to be sure I understand exactly what your
Eli> code does.

libcpp uses:

#define DSC(str) (const unsigned char *)str, sizeof str - 1

Sometimes this is a bit obscure but it has the nice quality that you
can use the string constant in the place where it is needed, and you
only have to write it once.  (The 'unsigned' is because libcpp uses
unsigned chars everywhere -- gdb wouldn't need this.)

Eli> (Btw, in stock Emacs "ESC =" counts _lines_ in region, not characters.
Eli> You probably have some customization that does this.)

FWIW count-lines-region displays the number of characters as well as
the number of lines.

Tom

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Eli Zaretskii]

> Cc: Vladimir Prus <vladimir@codesourcery.com>, marc.khouzam@ericsson.com,         gdb-patches@sources.redhat.com
> From: Tom Tromey <tromey@redhat.com>
> Date: Sun, 29 Jun 2008 13:41:39 -0600
> 
> Eli> (Btw, in stock Emacs "ESC =" counts _lines_ in region, not characters.
> Eli> You probably have some customization that does this.)
> 
> FWIW count-lines-region displays the number of characters as well as
> the number of lines.

Well, yes, but I thought Vladimir was saying that it actually inserts
the number into the source code for him.  I guess I was reading too
much into what he actually wrote, sorry.

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Michael Snyder]

On Sun, 2008-06-29 at 13:41 -0600, Tom Tromey wrote:
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> I think the variant with threadStr[] and sizeof - 1 is more
> Eli> maintainable, especially since not everyone uses Emacs.  Moreover, a
> Eli> literal constant makes code a bit harder to read, since I need to
> Eli> count characters after you, to be sure I understand exactly what your
> Eli> code does.
> 
> libcpp uses:
> 
> #define DSC(str) (const unsigned char *)str, sizeof str - 1
> 
> Sometimes this is a bit obscure but it has the nice quality that you
> can use the string constant in the place where it is needed, and you
> only have to write it once.  (The 'unsigned' is because libcpp uses
> unsigned chars everywhere -- gdb wouldn't need this.)

Boy... IMHO, I find that *very* obscure.
I wouldn't like reading code that used that, 
if I had no idea what it was or where it was defined.

Granted it's also clever...

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Tom Tromey]

>>>>> "Michael" == Michael Snyder <msnyder@specifix.com> writes:

>> #define DSC(str) (const unsigned char *)str, sizeof str - 1

Michael> Boy... IMHO, I find that *very* obscure.

Haha, you should see the rest of libcpp ;)

Michael> I wouldn't like reading code that used that, 
Michael> if I had no idea what it was or where it was defined.
Michael> Granted it's also clever...

Yeah.  IME it isn't so bad once you've looked it up a couple of times.
I don't even know what "DSC" stands for though... bah.

Tom

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Daniel Jacobowitz]

On Mon, Jun 30, 2008 at 12:35:03PM -0600, Tom Tromey wrote:
> >>>>> "Michael" == Michael Snyder <msnyder@specifix.com> writes:
> 
> >> #define DSC(str) (const unsigned char *)str, sizeof str - 1
> 
> Michael> Boy... IMHO, I find that *very* obscure.
> 
> Haha, you should see the rest of libcpp ;)
> 
> Michael> I wouldn't like reading code that used that, 
> Michael> if I had no idea what it was or where it was defined.
> Michael> Granted it's also clever...
> 
> Yeah.  IME it isn't so bad once you've looked it up a couple of times.
> I don't even know what "DSC" stands for though... bah.

Maybe "descriptor"?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Daniel Jacobowitz]

On Sat, Jun 28, 2008 at 08:44:14PM +0400, Vladimir Prus wrote:
> 
> This implements the --thread and --frame options to all MI command.
> Please see http://article.gmane.org/gmane.comp.gdb.devel/23414/ for
> background design for --thread. The --frame is ideologically same
> as --thread.
> 
> The only non-MI change here is making find_thread_pid exported from
> thread.c, which change seems obvious, so no approval is needed. Comments,
> however, are much appreciated.

There were a lot of comments on this patch, so I'll only skim
it... might want to post an updated version.

> +      if (strncmp (chp, "--thread", 8) == 0)
> +	{
> +	  if (parse->thread != -1)
> +	    error ("Duplicate '--thread' option");
> +	  chp += 8;
> +	  parse->thread = strtol (chp, &chp, 10);
> +	}

Someone may have mentioned this, but I think you should check for
"--thread " and "--frame " with trailing space.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Ulrich Weigand]

Vladimir Prus wrote:

>  mi_cmd_execute (struct mi_parse *parse)
>  {
>    struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
> +  char *thread_str;
> +  char *frame_str;
> +  int thread;
> +  int i;

[snip]

> +	error (_("Invalid frame id: %s"), frame_str);

This variable is used uninitialized, which breaks the -Werror
build for me.

(Also, it seems the other local variables introduced by this
patch are never used ...)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Vladimir Prus]

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

On Sunday 13 July 2008 00:14:42 Ulrich Weigand wrote:
> Vladimir Prus wrote:
> 
> >  mi_cmd_execute (struct mi_parse *parse)
> >  {
> >    struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
> > +  char *thread_str;
> > +  char *frame_str;
> > +  int thread;
> > +  int i;
> 
> [snip]
> 
> > +	error (_("Invalid frame id: %s"), frame_str);
> 
> This variable is used uninitialized, which breaks the -Werror
> build for me.
> 
> (Also, it seems the other local variables introduced by this
> patch are never used ...)

Apologies. I've checked in the attached to fix this.

- Volodya


[-- Attachment #2: commit.diff --]
[-- Type: text/x-diff, Size: 1340 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.9550
diff -u -p -r1.9550 ChangeLog
--- gdb/ChangeLog	12 Jul 2008 17:10:59 -0000	1.9550
+++ gdb/ChangeLog	13 Jul 2008 03:51:22 -0000
@@ -1,3 +1,8 @@
+2008-07-13  Vladimir Prus  <vladimir@codesourcery.com>
+
+	* mi/mi-main.c (mi_cmd_execute): Remove unused variable.
+	Fix printing of frame, when frame is wrong.
+	
 2008-07-12  Vladimir Prus  <vladimir@codesourcery.com>
 
 	Implement -exec-continue/-exec-interrupt --all.
Index: gdb/mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.126
diff -u -p -r1.126 mi-main.c
--- gdb/mi/mi-main.c	12 Jul 2008 17:10:59 -0000	1.126
+++ gdb/mi/mi-main.c	13 Jul 2008 03:51:22 -0000
@@ -1068,9 +1068,6 @@ static void
 mi_cmd_execute (struct mi_parse *parse)
 {
   struct cleanup *cleanup;
-  char *thread_str;
-  char *frame_str;
-  int thread;
   int i;
   free_all_values ();
 
@@ -1101,7 +1098,7 @@ mi_cmd_execute (struct mi_parse *parse)
 	/* find_relative_frame was successful */
 	select_frame (fid);
       else
-	error (_("Invalid frame id: %s"), frame_str);
+	error (_("Invalid frame id: %d"), frame);
     }
   
   if (parse->cmd->argv_func != NULL)

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Ulrich Weigand]

Vladimir Prus wrote:
> On Sunday 13 July 2008 00:14:42 Ulrich Weigand wrote:
> > This variable is used uninitialized, which breaks the -Werror
> > build for me.
> > 
> > (Also, it seems the other local variables introduced by this
> > patch are never used ...)
> 
> Apologies. I've checked in the attached to fix this.

Thanks!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [MI non-stop 04/11] Implement --thread and --frame.

[Vladimir Prus]

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

Vladimir Prus wrote:

> 
> This implements the --thread and --frame options to all MI command.
> Please see http://article.gmane.org/gmane.comp.gdb.devel/23414/ for
> background design for --thread. The --frame is ideologically same
> as --thread.
> 
> The only non-MI change here is making find_thread_pid exported from
> thread.c, which change seems obvious, so no approval is needed. Comments,
> however, are much appreciated.

I've checked in the below. It checks for "--thread " as opposed to "--thread"
as suggested by Marc and uses sizeof.

- Volodya



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: thread.diff --]
[-- Type: text/x-diff; name="thread.diff", Size: 5404 bytes --]

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 89072a6..d8b3e0b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2008-07-12  Vladimir Prus  <vladimir@codesourcery.com>
 
+	Implement --thread and --frame.
+	* gdbthread.h (find_thread_id): Declare.
+        * thread.c (find_thread_id): Make non-static.
+        * mi/mi-main.c (mi_cmd_execute): Switch to the right
+        thread and frame, if necessary.
+        * mi/mi-parse.c (mi_parse): Handle --thread and --frame.
+        * mi/mi-parse.h (strcut mi_parse): New fields thread and frame.
+
+2008-07-12  Vladimir Prus  <vladimir@codesourcery.com>
+
 	* infrun.c (resume): Discard cleanups on early exit path.
 
 2008-07-12  Vladimir Prus  <vladimir@codesourcery.com>
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 3f1d217..f283865 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -148,6 +148,9 @@ extern int valid_thread_id (int thread);
 /* Search function to lookup a thread by 'pid'.  */
 extern struct thread_info *find_thread_pid (ptid_t ptid);
 
+/* Find thread by GDB user-visible thread number.  */
+struct thread_info *find_thread_id (int num);
+
 /* Iterator function to call a user-provided callback function
    once for each known thread.  */
 typedef int (*thread_callback_func) (struct thread_info *, void *);
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index a82d2f0..f829a93 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1052,11 +1052,42 @@ static void
 mi_cmd_execute (struct mi_parse *parse)
 {
   struct cleanup *cleanup;
+  char *thread_str;
+  char *frame_str;
+  int thread;
+  int i;
   free_all_values ();
 
   current_token = xstrdup (parse->token);
   cleanup = make_cleanup (free_current_contents, &current_token);
 
+  if (parse->frame != -1 && parse->thread == -1)
+    error (_("Cannot specify --frame without --thread"));
+  
+  if (parse->thread != -1)
+    {
+      struct thread_info *tp = find_thread_id (parse->thread);
+      if (!tp)
+	error (_("Invalid thread id: %d"), parse->thread);
+      
+      if (non_stop)
+	context_switch_to (tp->ptid);
+      else
+	switch_to_thread (tp->ptid);
+    }
+  
+  if (parse->frame != -1)
+    {
+      struct frame_info *fid;
+      int frame = parse->frame;
+      fid = find_relative_frame (get_current_frame (), &frame);
+      if (frame == 0)
+	/* find_relative_frame was successful */
+	select_frame (fid);
+      else
+	error (_("Invalid frame id: %s"), frame_str);
+    }
+  
   if (parse->cmd->argv_func != NULL)
     {
       if (target_can_async_p ()
@@ -1093,6 +1124,7 @@ mi_cmd_execute (struct mi_parse *parse)
 	      error_stream (stb);
 	    }
 	}
+
       parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
     }
   else if (parse->cmd->cli.cmd != 0)
diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
index a2dc50d..73e04aa 100644
--- a/gdb/mi/mi-parse.c
+++ b/gdb/mi/mi-parse.c
@@ -150,6 +150,8 @@ mi_parse (char *cmd)
   char *chp;
   struct mi_parse *parse = XMALLOC (struct mi_parse);
   memset (parse, 0, sizeof (*parse));
+  parse->thread = -1;
+  parse->frame = -1;
 
   /* Before starting, skip leading white space. */
   while (isspace (*cmd))
@@ -199,6 +201,40 @@ mi_parse (char *cmd)
   while (isspace (*chp))
     chp++;
 
+  /* Parse the --thread and --frame options, if present.  At present,
+     some important commands, like '-break-*' are implemented by forwarding
+     to the CLI layer directly.  We want to parse --thread and --frame
+     here, so as not to leave those option in the string that will be passed
+     to CLI.  */
+  for (;;)
+    {
+      char *start = chp;
+      size_t ts = sizeof ("--thread ") - 1;
+      size_t fs = sizeof ("--frame ") - 1;
+      if (strncmp (chp, "--thread ", ts) == 0)
+	{
+	  if (parse->thread != -1)
+	    error ("Duplicate '--thread' option");
+	  chp += ts;
+	  parse->thread = strtol (chp, &chp, 10);
+	}
+      else if (strncmp (chp, "--frame ", fs) == 0)
+	{
+	  if (parse->frame != -1)
+	    error ("Duplicate '--frame' option");
+	  chp += fs;
+	  parse->frame = strtol (chp, &chp, 10);
+	}
+      else
+	break;
+
+      if (*chp != '\0' && !isspace (*chp))
+	error ("Invalid value for the '%s' option",
+	       start[2] == 't' ? "--thread" : "--frame");
+      while (isspace (*chp))
+	chp++;
+    }
+
   /* For new argv commands, attempt to return the parsed argument
      list. */
   if (parse->cmd->argv_func != NULL)
diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
index 945c42d..aa262f5 100644
--- a/gdb/mi/mi-parse.h
+++ b/gdb/mi/mi-parse.h
@@ -46,6 +46,8 @@ struct mi_parse
     char *args;
     char **argv;
     int argc;
+    int thread;
+    int frame;
   };
 
 /* Attempts to parse CMD returning a ``struct mi_command''.  If CMD is
diff --git a/gdb/thread.c b/gdb/thread.c
index bb821cc..02ef6f1 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -54,8 +54,6 @@ void _initialize_thread (void);
 static struct thread_info *thread_list = NULL;
 static int highest_thread_num;
 
-static struct thread_info *find_thread_id (int num);
-
 static void thread_command (char *tidstr, int from_tty);
 static void thread_apply_all_command (char *, int);
 static int thread_alive (struct thread_info *);
@@ -289,7 +287,7 @@ delete_thread_silent (ptid_t ptid)
   delete_thread_1 (ptid, 1 /* silent */);
 }
 
-static struct thread_info *
+struct thread_info *
 find_thread_id (int num)
 {
   struct thread_info *tp;