[RFC/A] gdb/680: ui_out_reset?

[RFC/A] gdb/680: ui_out_reset?

[Keith Seitz]

Hi,

Following up to gdb/680 where the MI register commands cause assertion 
failures with errors...

There are several ways to approach this. One is to make sure that 
everytime ui_out_{list,tuple}_begin is called, there is a 
ui_out_{list,tuple}_end. This leads to code like (from 
mi_data_list_register_values):

  ui_out_list_begin (uiout, "register-values");

  if (argc == 1)		/* No args, beside the format: do all the regs */
    {
      for (regnum = 0;
	   regnum < numregs;
	   regnum++)
	{
	  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)
	    {
	      ui_out_tuple_end (uiout);
	      ui_out_list_end (uiout);
	      return MI_CMD_ERROR;
	    }
	  ui_out_tuple_end (uiout);
	}
    }
    /* snip */

I'd rather just propose a new ui-out function, ui_out_reset, which can be 
called to "cleanup" the uiout in case of errors. In this case, 
mi_out_rewind could call ui_out_reset and reset the "levels" field.

Of course, I think that Kevin also proposed something in the longjmp case 
which is needed in these functions, afaict, but that's another bug/patch.

What do people think? Explicitly call ui_out_{list,tuple}_end before 
returning an error code or add ui_out_reset? Perhaps another solution I've 
overlooked?

Keith

ChangeLog
2002-09-05  Keith Seitz  <keiths@redhat.com>

        * ui-out.c (ui_out_reset): New function.
        (ui_out_new): Use ui_out_reset.
        * ui-out.h (ui_out_reset): Add declaration.

mi/ChangeLog
2002-09-05  Keith Seitz  <keiths@redhat.com>

        * mi-out.c (mi_out_rewind): Call ui_out_reset to reset
        the uiout in case of errors.

Patch
Index: ui-out.c
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.c,v
retrieving revision 1.23
diff -p -r1.23 ui-out.c
*** ui-out.c	27 Jul 2002 01:54:15 -0000	1.23
--- ui-out.c	5 Sep 2002 15:02:46 -0000
*************** ui_out_data (struct ui_out *uiout)
*** 1119,1124 ****
--- 1119,1136 ----
    return uiout->data;
  }
  
+ void
+ ui_out_reset (struct ui_out *uiout)
+ {
+   uiout->table.flag = 0;
+   uiout->table.body_flag = 0;
+   uiout->level = 0;
+   memset (uiout->levels, 0, sizeof (uiout->levels));
+   uiout->table.header_first = NULL;
+   uiout->table.header_last = NULL;
+   uiout->table.header_next = NULL;
+ }
+ 
  /* initalize private members at startup */
  
  struct ui_out *
*************** ui_out_new (struct ui_out_impl *impl,
*** 1130,1142 ****
    uiout->data = data;
    uiout->impl = impl;
    uiout->flags = flags;
!   uiout->table.flag = 0;
!   uiout->table.body_flag = 0;
!   uiout->level = 0;
!   memset (uiout->levels, 0, sizeof (uiout->levels));
!   uiout->table.header_first = NULL;
!   uiout->table.header_last = NULL;
!   uiout->table.header_next = NULL;
    return uiout;
  }
  
--- 1142,1148 ----
    uiout->data = data;
    uiout->impl = impl;
    uiout->flags = flags;
!   ui_out_reset (uiout);
    return uiout;
  }
  
Index: ui-out.h
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.h,v
retrieving revision 1.15
diff -p -r1.15 ui-out.h
*** ui-out.h	6 Jul 2001 03:53:11 -0000	1.15
--- ui-out.h	5 Sep 2002 15:02:46 -0000
*************** extern struct ui_out *ui_out_new (struct
*** 272,275 ****
--- 272,278 ----
  				  struct ui_out_data *data,
  				  int flags);
  
+ /* Reset the ui_out object. This is useful when dealing with errors. */
+ extern void ui_out_reset (struct ui_out *uiout);
+ 
  #endif /* UI_OUT_H */
Index: mi/mi-out.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-out.c,v
retrieving revision 1.23
diff -p -r1.23 mi-out.c
*** mi/mi-out.c	19 Mar 2002 02:51:08 -0000	1.23
--- mi/mi-out.c	5 Sep 2002 15:02:46 -0000
*************** mi_out_rewind (struct ui_out *uiout)
*** 409,414 ****
--- 409,417 ----
  {
    struct ui_out_data *data = ui_out_data (uiout);
    ui_file_rewind (data->buffer);
+ 
+   /* Reset the uiout in case there was an error. */
+   ui_out_reset (uiout);
  }
  
  /* dump the buffer onto the specified stream */

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

[Kevin Buettner]

On Sep 10,  4:44pm, Keith Seitz wrote:

> Following up to gdb/680 where the MI register commands cause assertion 
> failures with errors...
> 
> There are several ways to approach this. One is to make sure that 
> everytime ui_out_{list,tuple}_begin is called, there is a 
> ui_out_{list,tuple}_end. This leads to code like (from 
> mi_data_list_register_values):
> 
>   ui_out_list_begin (uiout, "register-values");
> 
>   if (argc == 1)		/* No args, beside the format: do all the regs */
>     {
>       for (regnum = 0;
> 	   regnum < numregs;
> 	   regnum++)
> 	{
> 	  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)
> 	    {
> 	      ui_out_tuple_end (uiout);
> 	      ui_out_list_end (uiout);
> 	      return MI_CMD_ERROR;
> 	    }
> 	  ui_out_tuple_end (uiout);
> 	}
>     }
>     /* snip */
> 
> I'd rather just propose a new ui-out function, ui_out_reset, which can be 
> called to "cleanup" the uiout in case of errors. In this case, 
> mi_out_rewind could call ui_out_reset and reset the "levels" field.
> 
> Of course, I think that Kevin also proposed something in the longjmp case 
> which is needed in these functions, afaict, but that's another bug/patch.
> 
> What do people think? Explicitly call ui_out_{list,tuple}_end before 
> returning an error code or add ui_out_reset? Perhaps another solution I've 
> overlooked?

Well, ui_out_reset() certainly seems easier, but I wonder if it's too
sloppy?  I.e, if we're going to do this, I'm wonder what the point of
checking the value of the ``level'' field (in push_level() and
pop_level()) in the first place is?

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.

So...  I guess I'm favoring explicit calls to ui_out_{list,tuple}_end().

Kevin

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

[Kevin Buettner]

On Sep 10,  5:29pm, Kevin Buettner wrote:

> So...  I guess I'm favoring explicit calls to ui_out_{list,tuple}_end().

Which comes as something of a surprise to me since I was thinking that
a reset mechanism might be a more palatable alternative to my own ui_out
proposals...

Kevin

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

[Keith Seitz]

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

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

[Kevin Buettner]

On Sep 11, 12:15pm, Keith Seitz wrote:

> 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.

Looks good to me.

(Since this is also the approach that I took, I'll commit my patches
too.)

Kevin

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

[Elena Zannoni]

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

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

[Keith Seitz]

On Wed, 11 Sep 2002, Elena Zannoni wrote:

>  > How's this patch look? It doesn't cause an regressions and fixes gdb/680. 
>  > 
> 
> there is a testcase for this, right?

Yes, gdb/testsuite/gdb.mi/gdb680.exp.

So okay to check this in?

Keith

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

[Elena Zannoni]

Keith Seitz writes:
 > On Wed, 11 Sep 2002, Elena Zannoni wrote:
 > 
 > >  > How's this patch look? It doesn't cause an regressions and fixes gdb/680. 
 > >  > 
 > > 
 > > there is a testcase for this, right?
 > 
 > Yes, gdb/testsuite/gdb.mi/gdb680.exp.
 > 
 > So okay to check this in?
 > 
 > Keith
 > 

sure.

Elena

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

[Keith Seitz]

On Wed, 11 Sep 2002, Elena Zannoni wrote:

>  > So okay to check this in?
>  > 
> sure.

Thanks for looking at this. I've committed it.

Keith