From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17641 invoked by alias); 28 Jun 2008 18:00:56 -0000 Received: (qmail 17423 invoked by uid 22791); 28 Jun 2008 18:00:51 -0000 X-Spam-Check-By: sourceware.org Received: from imr1.ericy.com (HELO imr1.ericy.com) (198.24.6.9) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 28 Jun 2008 18:00:29 +0000 Received: from eusrcmw751.eamcs.ericsson.se (eusrcmw751.exu.ericsson.se [138.85.77.51]) by imr1.ericy.com (8.13.1/8.13.1) with ESMTP id m5SHuKfk027119; Sat, 28 Jun 2008 12:56:20 -0500 Received: from ecamlmw720.eamcs.ericsson.se ([142.133.1.72]) by eusrcmw751.eamcs.ericsson.se with Microsoft SMTPSVC(6.0.3790.1830); Sat, 28 Jun 2008 12:56:53 -0500 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [MI non-stop 04/11] Implement --thread and --frame. Date: Sat, 28 Jun 2008 18:13:00 -0000 Message-ID: <6D19CA8D71C89C43A057926FE0D4ADAA04E1BD85@ecamlmw720.eamcs.ericsson.se> References: <200806282044.14246.vladimir@codesourcery.com> From: "Marc Khouzam" To: "Vladimir Prus" , X-IsSubscribed: yes 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/msg00557.txt.bz2 Hi, I don't know enough to thoroughly review so I'll give the minor comments I have. =20 > + if (parse->frame !=3D -1 && !parse->thread =3D=3D -1) You probably didn't mean to have ! before parse->thread > + error ("Cannot specify --frame without --thread"); > +=20=20 > + if (parse->thread !=3D -1) > + { > + struct thread_info *tp =3D find_thread_id (parse->thread); > + if (!tp) > + error (_("Invalid thread id: %d"), parse->thread); > +=20=20=20=20=20=20 > + if (non_stop) > + context_switch_to (tp->ptid); > + else > + switch_to_thread (tp->ptid); > + } > +=20=20 > + if (parse->frame !=3D -1) > + { > + struct frame_info *fid; > + int frame =3D parse->frame; > + fid =3D find_relative_frame (get_current_frame (), &frame); > + if (frame =3D=3D 0) > + /* find_relative_frame was successful */ > + select_frame (fid); > + else > + error (_("Invalid frame id: %s"), frame_str); > + } > +=20=20 > if (parse->cmd->argv_func !=3D NULL) > { > if (target_can_async_p () > @@ -1171,6 +1202,7 @@ mi_cmd_execute (struct mi_parse *parse) > error_stream (stb); > } > } > + > current_token =3D xstrdup (parse->token); > cleanup =3D 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 =3D XMALLOC (struct mi_parse); > memset (parse, 0, sizeof (*parse)); > + parse->thread =3D -1; > + parse->frame =3D -1; >=20=20 > /* Before starting, skip leading white space. */ > while (isspace (*cmd)) > @@ -199,6 +201,38 @@ mi_parse (char *cmd) > while (isspace (*chp)) > chp++; >=20=20 > + /* Parse the --thread and --frame options, if present. At present, > + some important commands, like '-break-*' are implemented by forward= ing > + 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 pa= ssed > + to CLI. */ > + for (;;) > + { > + char *start =3D chp; > + if (strncmp (chp, "--thread", 8) =3D=3D 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)=20 =20=20=20=20 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 =3D "--thread "; int len =3D strlen(threadStr); > + { > + if (parse->thread !=3D -1) > + error ("Duplicate '--thread' option"); > + chp +=3D 8; > + parse->thread =3D strtol (chp, &chp, 10); > + } > + else if (strncmp (chp, "--frame", 7) =3D=3D 0) > + { > + if (parse->frame !=3D -1) > + error ("Duplicate '--frame' option"); > + chp +=3D 7; > + parse->frame =3D 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 !=3D '\0' && !isspace (*chp)) > + error ("Invalid value for the '%s' option", > + start[2] =3D=3D 't' ? "--thread" : "--frame"); > + while (isspace (*chp)) > + chp++; > + } > + > /* For new argv commands, attempt to return the parsed argument [...]