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

[Elena Zannoni <ezannoni@redhat.com>]

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