From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11439 invoked by alias); 29 Jun 2008 06:03:47 -0000 Received: (qmail 11389 invoked by uid 22791); 29 Jun 2008 06:03:46 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 29 Jun 2008 06:03:29 +0000 Received: (qmail 18497 invoked from network); 29 Jun 2008 06:03:27 -0000 Received: from unknown (HELO wind.local) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Jun 2008 06:03:27 -0000 From: Vladimir Prus To: "Marc Khouzam" Subject: Re: [MI non-stop 04/11] Implement --thread and --frame. Date: Sun, 29 Jun 2008 06:08:00 -0000 User-Agent: KMail/1.9.9 Cc: gdb-patches@sources.redhat.com References: <200806282044.14246.vladimir@codesourcery.com> <6D19CA8D71C89C43A057926FE0D4ADAA04E1BD85@ecamlmw720.eamcs.ericsson.se> In-Reply-To: <6D19CA8D71C89C43A057926FE0D4ADAA04E1BD85@ecamlmw720.eamcs.ericsson.se> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200806291003.28226.vladimir@codesourcery.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-06/txt/msg00574.txt.bz2 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, ¤t_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