From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19488 invoked by alias); 11 Sep 2002 21:29:24 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 19481 invoked from network); 11 Sep 2002 21:29:23 -0000 Received: from unknown (HELO localhost.redhat.com) (66.30.197.194) by sources.redhat.com with SMTP; 11 Sep 2002 21:29:23 -0000 Received: by localhost.redhat.com (Postfix, from userid 469) id B992F106CC; Wed, 11 Sep 2002 17:27:31 -0400 (EDT) From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15743.46402.624640.843168@localhost.redhat.com> Date: Wed, 11 Sep 2002 14:29:00 -0000 To: Keith Seitz Cc: gdb-patches@sources.redhat.com Subject: Re: [RFC/A] gdb/680: ui_out_reset? In-Reply-To: References: <1020911002927.ZM18394@localhost.localdomain> X-SW-Source: 2002-09/txt/msg00191.txt.bz2 Keith Seitz writes: > On Tue, 10 Sep 2002, Kevin Buettner wrote: > > > Hmm. I guess it prevents you from writing code which goes too deep. > > But it doesn't really help you to check to see if you've correctly > > popped each level. At the moment, if you don't have enough pops, > > you'll eventually wind up seeing an internal error. (Which is a good > > thing, I think.) > > > > It also occurs to me that making sure that you've done the pops in the > > right places is important when the caller (of a function which > > returns MI_CMD_ERROR) decides to do something other than return to > > the top level. I.e, maybe it decides that it can somehow continue and > > produces more output. > > Wow, really good points. I hadn't thought of this last one, but I still > wish there was something a little less error-prone than remembering to pop > levels off of the uiout. Although we're lucky that this has only happened > in one isolated area of MI, it seems to me that this sort of thing could > creep in elsewhere, because it's really not very obvious or logical, I > think. > > I'll assume that it is a goal to make programming MI as much like > programming gdb as possible. Then it follows that what we should be doing > is using cleanups. > > In this case, the register commands (and probably a lot of other commands, > too) are just plain programmed wrong. They should be using > make_cleanup_ui_out_list/tuple_begin_end instead of > ui_out_list/tuple_begin and ui_out_list/tuple_end. > the cleanup functions were introduced after a lot of commands had been already implemented, so yes, I would think that other ones will have this problem. > How's this patch look? It doesn't cause an regressions and fixes gdb/680. > there is a testcase for this, right? I think it looks good. Elena > Keith > > ChangeLog > 2002-09-11 Keith Seitz > > * mi-main.c (mi_cmd_data_list_register_names): Use cleanups > for the uiout list. Do the cleanups when returning an error. > (mi_cmd_data_list_changed_registers): Ditto. > (mi_cmd_data_list_register_values): Use cleanups for the uiout list > and tuples. Do the cleanups when returning errors. > > Patch > Index: mi/mi-main.c > =================================================================== > RCS file: /cvs/src/src/gdb/mi/mi-main.c,v > retrieving revision 1.30 > diff -p -r1.30 mi-main.c > *** mi/mi-main.c 20 May 2002 18:09:57 -0000 1.30 > --- mi/mi-main.c 11 Sep 2002 19:02:49 -0000 > *************** mi_cmd_data_list_register_names (char *c > *** 275,280 **** > --- 275,281 ---- > { > int regnum, numregs; > int i; > + struct cleanup *cleanup; > > /* Note that the test for a valid register must include checking the > REGISTER_NAME because NUM_REGS may be allocated for the union of > *************** mi_cmd_data_list_register_names (char *c > *** 284,290 **** > > numregs = NUM_REGS + NUM_PSEUDO_REGS; > > ! ui_out_list_begin (uiout, "register-names"); > > if (argc == 0) /* No args, just do all the regs */ > { > --- 285,291 ---- > > numregs = NUM_REGS + NUM_PSEUDO_REGS; > > ! cleanup = make_cleanup_ui_out_list_begin_end (uiout, "register-names"); > > if (argc == 0) /* No args, just do all the regs */ > { > *************** mi_cmd_data_list_register_names (char *c > *** 306,311 **** > --- 307,313 ---- > regnum = atoi (argv[i]); > if (regnum < 0 || regnum >= numregs) > { > + do_cleanups (cleanup); > xasprintf (&mi_error_message, "bad register number"); > return MI_CMD_ERROR; > } > *************** mi_cmd_data_list_register_names (char *c > *** 315,321 **** > else > ui_out_field_string (uiout, NULL, REGISTER_NAME (regnum)); > } > ! ui_out_list_end (uiout); > return MI_CMD_DONE; > } > > --- 317,323 ---- > else > ui_out_field_string (uiout, NULL, REGISTER_NAME (regnum)); > } > ! do_cleanups (cleanup); > return MI_CMD_DONE; > } > > *************** mi_cmd_data_list_changed_registers (char > *** 324,329 **** > --- 326,332 ---- > { > int regnum, numregs, changed; > int i; > + struct cleanup *cleanup; > > /* Note that the test for a valid register must include checking the > REGISTER_NAME because NUM_REGS may be allocated for the union of > *************** mi_cmd_data_list_changed_registers (char > *** 333,339 **** > > numregs = NUM_REGS; > > ! ui_out_list_begin (uiout, "changed-registers"); > > if (argc == 0) /* No args, just do all the regs */ > { > --- 336,342 ---- > > numregs = NUM_REGS; > > ! cleanup = make_cleanup_ui_out_list_begin_end (uiout, "changed-registers"); > > if (argc == 0) /* No args, just do all the regs */ > { > *************** mi_cmd_data_list_changed_registers (char > *** 347,352 **** > --- 350,356 ---- > changed = register_changed_p (regnum); > if (changed < 0) > { > + do_cleanups (cleanup); > xasprintf (&mi_error_message, > "mi_cmd_data_list_changed_registers: Unable to read register contents."); > return MI_CMD_ERROR; > *************** mi_cmd_data_list_changed_registers (char > *** 369,374 **** > --- 373,379 ---- > changed = register_changed_p (regnum); > if (changed < 0) > { > + do_cleanups (cleanup); > xasprintf (&mi_error_message, > "mi_cmd_data_list_register_change: Unable to read register contents."); > return MI_CMD_ERROR; > *************** mi_cmd_data_list_changed_registers (char > *** 378,388 **** > } > else > { > xasprintf (&mi_error_message, "bad register number"); > return MI_CMD_ERROR; > } > } > ! ui_out_list_end (uiout); > return MI_CMD_DONE; > } > > --- 383,394 ---- > } > else > { > + do_cleanups (cleanup); > xasprintf (&mi_error_message, "bad register number"); > return MI_CMD_ERROR; > } > } > ! do_cleanups (cleanup); > return MI_CMD_DONE; > } > > *************** mi_cmd_data_list_register_values (char * > *** 418,423 **** > --- 424,430 ---- > { > int regnum, numregs, format, result; > int i; > + struct cleanup *list_cleanup, *tuple_cleanup; > > /* Note that the test for a valid register must include checking the > REGISTER_NAME because NUM_REGS may be allocated for the union of > *************** mi_cmd_data_list_register_values (char * > *** 443,449 **** > return MI_CMD_ERROR; > } > > ! ui_out_list_begin (uiout, "register-values"); > > if (argc == 1) /* No args, beside the format: do all the regs */ > { > --- 450,456 ---- > return MI_CMD_ERROR; > } > > ! list_cleanup = make_cleanup_ui_out_list_begin_end (uiout, "register-values"); > > if (argc == 1) /* No args, beside the format: do all the regs */ > { > *************** mi_cmd_data_list_register_values (char * > *** 454,465 **** > if (REGISTER_NAME (regnum) == NULL > || *(REGISTER_NAME (regnum)) == '\0') > continue; > ! ui_out_tuple_begin (uiout, NULL); > ui_out_field_int (uiout, "number", regnum); > result = get_register (regnum, format); > if (result == -1) > ! return MI_CMD_ERROR; > ! ui_out_tuple_end (uiout); > } > } > > --- 461,475 ---- > if (REGISTER_NAME (regnum) == NULL > || *(REGISTER_NAME (regnum)) == '\0') > continue; > ! tuple_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL); > ui_out_field_int (uiout, "number", regnum); > result = get_register (regnum, format); > if (result == -1) > ! { > ! do_cleanups (list_cleanup); > ! return MI_CMD_ERROR; > ! } > ! do_cleanups (tuple_cleanup); > } > } > > *************** mi_cmd_data_list_register_values (char * > *** 473,492 **** > && REGISTER_NAME (regnum) != NULL > && *REGISTER_NAME (regnum) != '\000') > { > ! ui_out_tuple_begin (uiout, NULL); > ui_out_field_int (uiout, "number", regnum); > result = get_register (regnum, format); > if (result == -1) > ! return MI_CMD_ERROR; > ! ui_out_tuple_end (uiout); > } > else > { > xasprintf (&mi_error_message, "bad register number"); > return MI_CMD_ERROR; > } > } > ! ui_out_list_end (uiout); > return MI_CMD_DONE; > } > > --- 483,506 ---- > && REGISTER_NAME (regnum) != NULL > && *REGISTER_NAME (regnum) != '\000') > { > ! tuple_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL); > ui_out_field_int (uiout, "number", regnum); > result = get_register (regnum, format); > if (result == -1) > ! { > ! do_cleanups (list_cleanup); > ! return MI_CMD_ERROR; > ! } > ! do_cleanups (tuple_cleanup); > } > else > { > + do_cleanups (list_cleanup); > xasprintf (&mi_error_message, "bad register number"); > return MI_CMD_ERROR; > } > } > ! do_cleanups (list_cleanup); > return MI_CMD_DONE; > } >