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

[Vladimir Prus <vladimir at codesourcery dot com>]

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