Re: [RFC/A] gdb/680: ui_out_reset?

[Keith Seitz <keiths@redhat.com>]

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  <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;
  }