This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFC/A] gdb/680: ui_out_reset?
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 <keiths@redhat.com>
>
> * 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;
> }
>