From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17507 invoked by alias); 11 Sep 2002 19:12:53 -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 17499 invoked from network); 11 Sep 2002 19:12:52 -0000 Received: from unknown (HELO valrhona.uglyboxes.com) (64.1.192.220) by sources.redhat.com with SMTP; 11 Sep 2002 19:12:52 -0000 Received: from localhost.localdomain (IDENT:uNjfirJXRwVY9omL26Nwz0i/Gv8d3ju3@localhost.localdomain [127.0.0.1]) by valrhona.uglyboxes.com (8.11.6/8.11.6) with ESMTP id g8BJFSK04635 for ; Wed, 11 Sep 2002 12:15:36 -0700 Date: Wed, 11 Sep 2002 12:12:00 -0000 From: Keith Seitz X-X-Sender: keiths@valrhona.uglyboxes.com To: gdb-patches@sources.redhat.com Subject: Re: [RFC/A] gdb/680: ui_out_reset? In-Reply-To: <1020911002927.ZM18394@localhost.localdomain> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2002-09/txt/msg00186.txt.bz2 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. How's this patch look? It doesn't cause an regressions and fixes gdb/680. 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; }