From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13939 invoked by alias); 7 Dec 2010 21:39:07 -0000 Received: (qmail 13608 invoked by uid 22791); 7 Dec 2010 21:39:01 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,TW_CP,TW_RG X-Spam-Check-By: sourceware.org Received: from imr3.ericy.com (HELO imr3.ericy.com) (198.24.6.13) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 07 Dec 2010 21:38:39 +0000 Received: from eusaamw0712.eamcs.ericsson.se ([147.117.20.181]) by imr3.ericy.com (8.13.8/8.13.8) with ESMTP id oB7LcRFX030124 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL); Tue, 7 Dec 2010 15:38:27 -0600 Received: from EUSAACMS0703.eamcs.ericsson.se ([169.254.1.63]) by eusaamw0712.eamcs.ericsson.se ([147.117.20.181]) with mapi; Tue, 7 Dec 2010 16:38:27 -0500 From: Marc Khouzam To: "'Tom Tromey'" CC: "'gdb-patches@sourceware.org'" Date: Tue, 07 Dec 2010 21:39:00 -0000 Subject: RE: [MI] Segfault using 'interpreter-exec mi' Message-ID: References: In-Reply-To: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: 2010-12/txt/msg00077.txt.bz2 =20 > -----Original Message----- > From: Tom Tromey [mailto:tromey@redhat.com]=20 > Sent: Tuesday, December 07, 2010 3:33 PM > To: Marc Khouzam > Cc: 'gdb-patches@sourceware.org' > Subject: Re: [MI] Segfault using 'interpreter-exec mi' >=20 > >>>>> "Marc" =3D=3D Marc Khouzam writes: >=20 > Marc> I agree with you and I started coding such a patch. Things > Marc> got a bit more complex than I expected though. Mostly because > Marc> the error output of mi_parse() has to use some of the data that=20 > Marc> was actually parsed in mi_parse(), which lead me to have to=20 > Marc> play around with return values while handling exceptions, or > Marc> having to free some memory _after_ calling error(), which=20 > Marc> I didn't know how to do. >=20 > Ok -- I wrote it myself and now I actually understand. mi_parse > extracts the token from the MI command line, and you need=20 > this token to > report the error. But, if you write the patch the obvious way, the > token will be freed too soon. >=20 > Here is the patch I came up with. Let me know what you think. Thanks! I was stuck trying to use catch_exception(), which was not working because I needed the return value of mi_parse(), but I see that using TRY_CATCH was the way to go. > I am running it through the regression tester. >=20 > Marc> Could we have the brute force fix now, and leave the improvement > Marc> for later? >=20 > As a general rule I think it is perfectly ok for one patch to go on a > branch and a different one to go in mainline. >=20 > In this particular case, I think the appended is probably safe enough > for 7.2 as well, but I'd appreciate a sanity check on that. I haven't actually run it yet, I'll do that tonight, but I have a single question about the patch, please see below. Thanks a lot for taking the time for this! Marc >=20 > Tom >=20 > 2010-12-07 Tom Tromey >=20 > * mi/mi-parse.h (mi_parse): Update. > * mi/mi-parse.c (mi_parse_cleanup): New function. > (mi_parse): Add 'token' argument. Throw exception on error. > * mi/mi-main.c (mi_print_exception): New function. > (mi_execute_command): Use mi_print_exception. Catch exceptions > from mi_parse. >=20 > 2010-12-07 Tom Tromey >=20 > * gdb.base/interp.exp: Add regression test. >=20 > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index 3343c03..48e907f 100644 > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -1865,11 +1865,26 @@ captured_mi_execute_command (struct=20 > ui_out *uiout, void *data) > return; > } >=20=20 > +/* Print a gdb exception to the MI output stream. */ > + > +static void > +mi_print_exception (const char *token, struct gdb_exception=20 > exception) > +{ > + fputs_unfiltered (token, raw_stdout); > + fputs_unfiltered ("^error,msg=3D\"", raw_stdout); > + if (exception.message =3D=3D NULL) > + fputs_unfiltered ("unknown error", raw_stdout); > + else > + fputstr_unfiltered (exception.message, '"', raw_stdout); > + fputs_unfiltered ("\"\n", raw_stdout); > +} >=20=20 > void > mi_execute_command (char *cmd, int from_tty) > { > - struct mi_parse *command; > + char *token; > + struct mi_parse *command =3D NULL; > + volatile struct gdb_exception exception; >=20=20 > /* This is to handle EOF (^D). We just quit gdb. */ > /* FIXME: we should call some API function here. */ > @@ -1878,13 +1893,22 @@ mi_execute_command (char *cmd, int from_tty) >=20=20 > target_log_command (cmd); >=20=20 > - command =3D mi_parse (cmd); > - > - if (command !=3D NULL) > + TRY_CATCH (exception, RETURN_MASK_ALL) > + { > + command =3D mi_parse (cmd, &token); > + } > + if (exception.reason < 0) > + { > + mi_print_exception (token, exception); > + xfree (token); Here, do we need an call to mi_out_rewind (uiout)? I don't know what it does, but I noticed it was being called below after mi_print_exception() > + } > + else > { > struct gdb_exception result; > ptid_t previous_ptid =3D inferior_ptid; >=20=20 > + command->token =3D token; > + > if (do_timings) > { > command->cmd_start =3D (struct mi_timestamp *) > @@ -1898,13 +1922,7 @@ mi_execute_command (char *cmd, int from_tty) > { > /* The command execution failed and error() was called > somewhere. */ > - fputs_unfiltered (command->token, raw_stdout); > - fputs_unfiltered ("^error,msg=3D\"", raw_stdout); > - if (result.message =3D=3D NULL) > - fputs_unfiltered ("unknown error", raw_stdout); > - else > - fputstr_unfiltered (result.message, '"', raw_stdout); > - fputs_unfiltered ("\"\n", raw_stdout); > + mi_print_exception (command->token, result); > mi_out_rewind (uiout); > } >=20=20 > diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c > index 4541b66..774d368 100644 > --- a/gdb/mi/mi-parse.c > +++ b/gdb/mi/mi-parse.c > @@ -223,12 +223,20 @@ mi_parse_free (struct mi_parse *parse) > xfree (parse); > } >=20=20 > +/* A cleanup that calls mi_parse_free. */ > + > +static void > +mi_parse_cleanup (void *arg) > +{ > + mi_parse_free (arg); > +} >=20=20 > struct mi_parse * > -mi_parse (char *cmd) > +mi_parse (char *cmd, char **token) > { > char *chp; > struct mi_parse *parse =3D XMALLOC (struct mi_parse); > + struct cleanup *cleanup; >=20=20 > memset (parse, 0, sizeof (*parse)); > parse->all =3D 0; > @@ -236,6 +244,8 @@ mi_parse (char *cmd) > parse->thread =3D -1; > parse->frame =3D -1; >=20=20 > + cleanup =3D make_cleanup (mi_parse_cleanup, parse); > + > /* Before starting, skip leading white space. */ > while (isspace (*cmd)) > cmd++; > @@ -243,9 +253,9 @@ mi_parse (char *cmd) > /* Find/skip any token and then extract it. */ > for (chp =3D cmd; *chp >=3D '0' && *chp <=3D '9'; chp++) > ; > - parse->token =3D xmalloc ((chp - cmd + 1) * sizeof (char *)); > - memcpy (parse->token, cmd, (chp - cmd)); > - parse->token[chp - cmd] =3D '\0'; > + *token =3D xmalloc ((chp - cmd + 1) * sizeof (char *)); > + memcpy (*token, cmd, (chp - cmd)); > + (*token)[chp - cmd] =3D '\0'; >=20=20 > /* This wasn't a real MI command. Return it as a CLI_COMMAND. */ > if (*chp !=3D '-') > @@ -254,6 +264,9 @@ mi_parse (char *cmd) > chp++; > parse->command =3D xstrdup (chp); > parse->op =3D CLI_COMMAND; > + > + discard_cleanups (cleanup); > + > return parse; > } >=20=20 > @@ -271,15 +284,7 @@ mi_parse (char *cmd) > /* Find the command in the MI table. */ > parse->cmd =3D mi_lookup (parse->command); > if (parse->cmd =3D=3D NULL) > - { > - /* FIXME: This should be a function call. */ > - fprintf_unfiltered > - (raw_stdout, > - "%s^error,msg=3D\"Undefined MI command: %s\"\n", > - parse->token, parse->command); > - mi_parse_free (parse); > - return NULL; > - } > + error (_("Undefined MI command: %s"), parse->command); >=20=20 > /* Skip white space following the command. */ > while (isspace (*chp)) > @@ -349,15 +354,7 @@ mi_parse (char *cmd) > { > mi_parse_argv (chp, parse); > if (parse->argv =3D=3D NULL) > - { > - /* FIXME: This should be a function call. */ > - fprintf_unfiltered > - (raw_stdout, > - "%s^error,msg=3D\"Problem parsing arguments: %s %s\"\n", > - parse->token, parse->command, chp); > - mi_parse_free (parse); > - return NULL; > - } > + error (_("Problem parsing arguments: %s %s"),=20 > parse->command, chp); > } >=20=20 > /* FIXME: DELETE THIS */ > @@ -366,6 +363,8 @@ mi_parse (char *cmd) > if (parse->cmd->cli.cmd !=3D NULL) > parse->args =3D xstrdup (chp); >=20=20 > + discard_cleanups (cleanup); > + > /* Fully parsed. */ > parse->op =3D MI_COMMAND; > return parse; > diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h > index 3c6cd9a..6d0f53c 100644 > --- a/gdb/mi/mi-parse.h > +++ b/gdb/mi/mi-parse.h > @@ -52,13 +52,15 @@ struct mi_parse > int frame; > }; >=20=20 > -/* Attempts to parse CMD returning a ``struct mi_command''.=20=20 > If CMD is > - invalid, an error mesage is reported (MI format) and NULL is > - returned. For a CLI_COMMAND, COMMAND, TOKEN and OP are=20 > initialized. > - For an MI_COMMAND COMMAND, TOKEN, ARGS and OP are > - initialized. Un-initialized fields are zero. */ > +/* Attempts to parse CMD returning a ``struct mi_parse''. If CMD is > + invalid, an exception is thrown. For an MI_COMMAND COMMAND, ARGS > + and OP are initialized. Un-initialized fields are zero.=20=20 > *TOKEN is > + set to the token, even if an exception is thrown. It is allocated > + with xmalloc; it must either be freed with xfree, or assigned to > + the TOKEN field of the resultant mi_parse object, to be freed by > + mi_parse_free. */ >=20=20 > -extern struct mi_parse *mi_parse (char *cmd); > +extern struct mi_parse *mi_parse (char *cmd, char **token); >=20=20 > /* Free a command returned by mi_parse_command. */ >=20=20 > diff --git a/gdb/testsuite/gdb.base/interp.exp=20 > b/gdb/testsuite/gdb.base/interp.exp > index ece2552..a923f1a 100644 > --- a/gdb/testsuite/gdb.base/interp.exp > +++ b/gdb/testsuite/gdb.base/interp.exp > @@ -33,4 +33,15 @@ gdb_test_multiple $cmd $cmd { > } > gdb_test "interpreter-exec console \"show version\"" "GNU gdb .*" >=20=20 > +# Regression test for crash when an exception occurs in mi_parse. > +gdb_test_multiple "interpreter-exec mi \"-break-insert=20 > --thread a\"" \ > + "regression test for mi_parse crash" { > + -re ".error,msg=3D.Invalid value for the '--thread'=20 > option.\r\n$gdb_prompt " { > + pass "$cmd" > + gdb_expect 1 { > + -re "\r\n$gdb_prompt $" { } > + } > + } > + } > + > gdb_exit >=20