[patch] Fix nesting of ui_out_redirect

[patch] Fix nesting of ui_out_redirect

[Jan Kratochvil]

Hi,

ui_out_redirect assumed only double level of redirections so far.

GDB started to use them nested, though.  The testcase crashes FSF GDB HEAD.

Made there also some small related safety fixups of the related calls.

SUPPRESS_UI_FILEP_DECL is very ugly there but I am not aware how to easily use
vec.h otherwise and vec.h was being pushed as the GDB data type in such cases.
I would use just xmalloc()ed single LIFO link list otherwise.

No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.


Thanks,
Jan


gdb/
2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (save_breakpoints): Use RETURN_MASK_ALL.
	* cli-out.c: Include vec.h.  Define SUPPRESS_UI_FILEP_DECL, ui_filep
	and use DEF_VEC_P for ui_filep.
	(cli_field_fmt, cli_spaces, cli_text, cli_message, cli_flush): New
	variable stream, initialize it, use it.
	(cli_redirect): New function comment.  Replace the stream and
	original_stream fields by the new streams field.  Remove the
	original_stream != NULL conditional, assert error on NULL instead.
	(out_field_fmt, field_separator): New variable stream, initialize it, use it.
	(cli_out_data_ctor): Assert non-NULL stream.  Replace the stream and
	original_stream fields by the new streams field.
	(cli_out_set_stream): Replace the stream field by the new streams
	field.
	* cli-out.h: Include vec.h.
	<! SUPPRESS_UI_FILEP_DECL>: Define ui_filep and use VEC_T.
	(struct cli_ui_out_data): Replace the stream and original_stream
	fields by the new streams field.
	* cli/cli-logging.c (set_logging_redirect): Call ui_out_redirect with
	NULL first.  Move the comment into the inner block.
	(handle_redirections): Call ui_out_redirect with output.
	* python/py-breakpoint.c (bppy_get_commands): Move ui_out_redirect
	calls outside of the TRY_CATCH block.

gdb/testsuite/
2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/ui-redirect.exp: New file.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11487,7 +11487,7 @@ save_breakpoints (char *filename, int from_tty,
 	fprintf_unfiltered (fp, "  commands\n");
 	
 	ui_out_redirect (uiout, fp);
-	TRY_CATCH (ex, RETURN_MASK_ERROR)
+	TRY_CATCH (ex, RETURN_MASK_ALL)
 	  {
 	    print_command_lines (uiout, tp->commands->commands, 2);
 	  }
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -22,6 +22,11 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "vec.h"
+#define SUPPRESS_UI_FILEP_DECL
+typedef struct ui_file *ui_filep;
+DEF_VEC_P (ui_filep);
+
 #include "ui-out.h"
 #include "cli-out.h"
 #include "gdb_string.h"
@@ -224,11 +229,13 @@ cli_field_fmt (struct ui_out *uiout, int fldno,
 	       va_list args)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;
 
   if (data->suppress_output)
     return;
 
-  vfprintf_filtered (data->stream, format, args);
+  stream = VEC_last (ui_filep, data->streams);
+  vfprintf_filtered (stream, format, args);
 
   if (align != ui_noalign)
     field_separator ();
@@ -238,20 +245,26 @@ static void
 cli_spaces (struct ui_out *uiout, int numspaces)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;
 
   if (data->suppress_output)
     return;
-  print_spaces_filtered (numspaces, data->stream);
+
+  stream = VEC_last (ui_filep, data->streams);
+  print_spaces_filtered (numspaces, stream);
 }
 
 static void
 cli_text (struct ui_out *uiout, const char *string)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;
 
   if (data->suppress_output)
     return;
-  fputs_filtered (string, data->stream);
+
+  stream = VEC_last (ui_filep, data->streams);
+  fputs_filtered (string, stream);
 }
 
 static void ATTRIBUTE_PRINTF (3, 0)
@@ -262,8 +275,13 @@ cli_message (struct ui_out *uiout, int verbosity,
 
   if (data->suppress_output)
     return;
+
   if (ui_out_get_verblvl (uiout) >= verbosity)
-    vfprintf_unfiltered (data->stream, format, args);
+    {
+      struct ui_file *stream = VEC_last (ui_filep, data->streams);
+
+      vfprintf_unfiltered (stream, format, args);
+    }
 }
 
 static void
@@ -280,25 +298,24 @@ static void
 cli_flush (struct ui_out *uiout)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);
 
-  gdb_flush (data->stream);
+  gdb_flush (stream);
 }
 
+/* OUTSTREAM as non-NULL will push OUTSTREAM on the stack of output streams
+   and make it therefore active.  OUTSTREAM as NULL will pop the last pushed
+   output stream; it is an internal error if it does not exist.  */
+
 static int
 cli_redirect (struct ui_out *uiout, struct ui_file *outstream)
 {
   cli_out_data *data = ui_out_data (uiout);
 
   if (outstream != NULL)
-    {
-      data->original_stream = data->stream;
-      data->stream = outstream;
-    }
-  else if (data->original_stream != NULL)
-    {
-      data->stream = data->original_stream;
-      data->original_stream = NULL;
-    }
+    VEC_safe_push (ui_filep, data->streams, outstream);
+  else
+    VEC_pop (ui_filep, data->streams);
 
   return 0;
 }
@@ -315,10 +332,11 @@ out_field_fmt (struct ui_out *uiout, int fldno,
 	       const char *format,...)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);
   va_list args;
 
   va_start (args, format);
-  vfprintf_filtered (data->stream, format, args);
+  vfprintf_filtered (stream, format, args);
 
   va_end (args);
 }
@@ -329,8 +347,9 @@ static void
 field_separator (void)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);
 
-  fputc_filtered (' ', data->stream);
+  fputc_filtered (' ', stream);
 }
 
 /* This is the CLI ui-out implementation functions vector */
@@ -364,8 +383,11 @@ struct ui_out_impl cli_ui_out_impl =
 void
 cli_out_data_ctor (cli_out_data *self, struct ui_file *stream)
 {
-  self->stream = stream;
-  self->original_stream = NULL;
+  gdb_assert (stream != NULL);
+
+  self->streams = NULL;
+  VEC_safe_push (ui_filep, self->streams, stream);
+
   self->suppress_output = 0;
 }
 
@@ -385,8 +407,10 @@ struct ui_file *
 cli_out_set_stream (struct ui_out *uiout, struct ui_file *stream)
 {
   cli_out_data *data = ui_out_data (uiout);
-  struct ui_file *old = data->stream;
+  struct ui_file *old;
+  
+  old = VEC_pop (ui_filep, data->streams);
+  VEC_quick_push (ui_filep, data->streams, stream);
 
-  data->stream = stream;
   return old;
 }
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -22,14 +22,20 @@
 #define CLI_OUT_H
 
 #include "ui-out.h"
+#include "vec.h"
+
+/* Used for cli_ui_out_data->streams.  */
+#ifndef SUPPRESS_UI_FILEP_DECL
+typedef struct ui_file *ui_filep;
+VEC_T (ui_filep);
+#endif
 
 /* These are exported so that they can be extended by other `ui_out'
    implementations, like TUI's.  */
 
 struct cli_ui_out_data
   {
-    struct ui_file *stream;
-    struct ui_file *original_stream;
+    VEC (ui_filep) *streams;
     int suppress_output;
   };
 
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -118,9 +118,12 @@ set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
   gdb_stdtarg = output;
   logging_no_redirect_file = new_logging_no_redirect_file;
 
-  /* It should not happen, the redirection has been already setup.  */
-  if (ui_out_redirect (uiout, output) < 0)
-    warning (_("Current output protocol does not support redirection"));
+  if (ui_out_redirect (uiout, NULL) < 0
+      || ui_out_redirect (uiout, output) < 0)
+    {
+      /* It should not happen, the redirection has been already setup.  */
+      warning (_("Current output protocol does not support redirection"));
+    }
 
   if (logging_redirect != 0)
     do_cleanups (cleanups);
@@ -212,7 +215,7 @@ handle_redirections (int from_tty)
   gdb_stdlog = output;
   gdb_stdtarg = output;
 
-  if (ui_out_redirect (uiout, gdb_stdout) < 0)
+  if (ui_out_redirect (uiout, output) < 0)
     warning (_("Current output protocol does not support redirection"));
 }
 
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -474,12 +474,12 @@ bppy_get_commands (PyObject *self, void *closure)
   string_file = mem_fileopen ();
   chain = make_cleanup_ui_file_delete (string_file);
 
+  ui_out_redirect (uiout, string_file);
   TRY_CATCH (except, RETURN_MASK_ALL)
     {
-      ui_out_redirect (uiout, string_file);
       print_command_lines (uiout, breakpoint_commands (bp), 0);
-      ui_out_redirect (uiout, NULL);
     }
+  ui_out_redirect (uiout, NULL);
   cmdstr = ui_file_xstrdup (string_file, &length);
   GDB_PY_HANDLE_EXCEPTION (except);
 
--- /dev/null
+++ b/gdb/testsuite/gdb.base/ui-redirect.exp
@@ -0,0 +1,41 @@
+# Copyright (C) 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if { [prepare_for_testing ui-redirect.exp ui-redirect start.c] } {
+    return -1
+}
+
+gdb_breakpoint main
+
+set test "commands"
+gdb_test_multiple $test $test {
+    -re "End with a line saying just \"end\"\\.\r\n>$" {
+	pass $test
+    }
+}
+
+set test "print 1"
+gdb_test_multiple $test $test {
+    -re "\r\n>$" {
+	pass $test
+    }
+}
+gdb_test_no_output "end"
+
+gdb_test_no_output "set logging file /dev/null"
+gdb_test "set logging on" "Copying output to /dev/null\\."
+gdb_test "save breakpoints /dev/null" "Saved to file '/dev/null'\\."
+gdb_test "set logging off" "Done logging to /dev/null\\."
+gdb_test "help" "List of classes of commands:.*"

Re: [patch] Fix nesting of ui_out_redirect

[Pedro Alves]

On Friday 03 September 2010 12:21:39, Jan Kratochvil wrote:
> Hi,
> 
> ui_out_redirect assumed only double level of redirections so far.
> 
> GDB started to use them nested, though.  The testcase crashes FSF GDB HEAD.
> 
> Made there also some small related safety fixups of the related calls.
> 
> SUPPRESS_UI_FILEP_DECL is very ugly there but I am not aware how to easily use
> vec.h otherwise and vec.h was being pushed as the GDB data type in such cases.

Just putting the

DEF_VEC_P (ui_filep);

in the cli-out.h header is the norm.

It looks easy to tweak vec.h to get rid of the typedef, and so be able to
forward declare VECs.  E.g.,:

---
 gdb/cli-out.c |    6 ++----
 gdb/cli-out.h |    4 +---
 gdb/vec.h     |   11 ++++++++---
 3 files changed, 11 insertions(+), 10 deletions(-)

Index: src/gdb/cli-out.c
===================================================================
--- src.orig/gdb/cli-out.c	2010-09-03 12:43:35.000000000 +0100
+++ src/gdb/cli-out.c	2010-09-03 13:42:01.000000000 +0100
@@ -23,15 +23,13 @@
 
 #include "defs.h"
 #include "vec.h"
-#define SUPPRESS_UI_FILEP_DECL
-typedef struct ui_file *ui_filep;
-DEF_VEC_P (ui_filep);
-
 #include "ui-out.h"
 #include "cli-out.h"
 #include "gdb_string.h"
 #include "gdb_assert.h"
 
+DEF_VEC_P (ui_filep);
+
 typedef struct cli_ui_out_data cli_out_data;
 
 
Index: src/gdb/cli-out.h
===================================================================
--- src.orig/gdb/cli-out.h	2010-09-03 12:43:35.000000000 +0100
+++ src/gdb/cli-out.h	2010-09-03 13:41:48.000000000 +0100
@@ -25,10 +25,8 @@
 #include "vec.h"
 
 /* Used for cli_ui_out_data->streams.  */
-#ifndef SUPPRESS_UI_FILEP_DECL
 typedef struct ui_file *ui_filep;
-VEC_T (ui_filep);
-#endif
+DEC_VEC (ui_filep);
 
 /* These are exported so that they can be extended by other `ui_out'
    implementations, like TUI's.  */
Index: src/gdb/vec.h
===================================================================
--- src.orig/gdb/vec.h	2010-06-16 12:36:45.000000000 +0100
+++ src/gdb/vec.h	2010-09-03 13:45:33.000000000 +0100
@@ -392,16 +392,21 @@ extern void *vec_o_reserve (void *, int,
 #define vec_assert(expr, op) \
   ((void)((expr) ? 0 : (gdb_assert_fail (op, file_, line_, ASSERT_FUNCTION), 0)))
 
-#define VEC(T) VEC_##T
+#define VEC_TAG(T) VEC_##T
 #define VEC_OP(T,OP) VEC_##T##_##OP
 
+#define DEC_VEC(T)							  \
+  struct VEC_TAG(T)
+
 #define VEC_T(T)							  \
-typedef struct VEC(T)							  \
+struct VEC_TAG(T)							  \
 {									  \
   unsigned num;								  \
   unsigned alloc;							  \
   T vec[1];								  \
-} VEC(T)
+}
+
+#define VEC(T) struct VEC_TAG(T)
 
 /* Vector of integer-like object.  */
 #define DEF_VEC_I(T)							  \



but I'm really not sure it's worth it to have.  Each module that
wants to use the VEC still needs to "DEF_VEC_P (ui_filep)"
(or similar), given that that defines the bunch of static inline
functions that actually manipulate the VEC.  We'd probably
want something like this in the headers:

DEC_VEC (ui_filep);
#define DEF_VEC_ui_filep \
DEF_VEC_P (ui_filep)

and then in the .c files that actually use the VEC, write

DEF_VEC_ui_filep;

somewhere at the top.  (replace ui_filep with your favorite
type name, and _P with _I or _O appropriately, of course.)


> --- a/gdb/cli/cli-logging.c
> +++ b/gdb/cli/cli-logging.c
> @@ -118,9 +118,12 @@ set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
>    gdb_stdtarg = output;
>    logging_no_redirect_file = new_logging_no_redirect_file;
>  
> -  /* It should not happen, the redirection has been already setup.  */
> -  if (ui_out_redirect (uiout, output) < 0)
> -    warning (_("Current output protocol does not support redirection"));
> +  if (ui_out_redirect (uiout, NULL) < 0
> +      || ui_out_redirect (uiout, output) < 0)
> +    {
> +      /* It should not happen, the redirection has been already setup.  */
> +      warning (_("Current output protocol does not support redirection"));
> +    }

I'm feeling dense, and this change isn't looking obvious to me.  Can you
explain it?

>  
>    if (logging_redirect != 0)
>      do_cleanups (cleanups);
> @@ -212,7 +215,7 @@ handle_redirections (int from_tty)
>    gdb_stdlog = output;
>    gdb_stdtarg = output;
>  
> -  if (ui_out_redirect (uiout, gdb_stdout) < 0)
> +  if (ui_out_redirect (uiout, output) < 0)
>      warning (_("Current output protocol does not support redirection"));
>  }
>  

Otherwise, it looks good to me.

Pedro Alves

Re: [patch] Fix nesting of ui_out_redirect

[Jan Kratochvil]

On Fri, 03 Sep 2010 15:04:22 +0200, Pedro Alves wrote:
> Just putting the
> 
> DEF_VEC_P (ui_filep);
> 
> in the cli-out.h header is the norm.

I did not expect it is possible (I expected some link conflicts).  I see it is
already being used in *.h files.


> It looks easy to tweak vec.h to get rid of the typedef, and so be able to
> forward declare VECs.  E.g.,:

OK but that is not a part of this patch and the typedef is the GDB norm now.


> Each module that
> wants to use the VEC still needs to "DEF_VEC_P (ui_filep)"

This is not needed in this case, the structure is not opaque only to be
"inheritable" (embeddable) by other structs.

I tried to put there void * satisfying the sizeof but that does not work for
various reasons of its usage from vec.h.


> > +  if (ui_out_redirect (uiout, NULL) < 0
> > +      || ui_out_redirect (uiout, output) < 0)
> > +    {
> > +      /* It should not happen, the redirection has been already setup.  */
> > +      warning (_("Current output protocol does not support redirection"));
> > +    }
> 
> I'm feeling dense, and this change isn't looking obvious to me.  Can you
> explain it?

Updated the comment, is it OK?


Thanks,
Jan


gdb/
2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (save_breakpoints): Use RETURN_MASK_ALL.
	* cli-out.c: Include vec.h.
	(cli_field_fmt, cli_spaces, cli_text, cli_message, cli_flush): New
	variable stream, initialize it, use it.
	(cli_redirect): New function comment.  Replace the stream and
	original_stream fields by the new streams field.  Remove the
	original_stream != NULL conditional, assert error on NULL instead.
	(out_field_fmt, field_separator): New variable stream, initialize it, use it.
	(cli_out_data_ctor): Assert non-NULL stream.  Replace the stream and
	original_stream fields by the new streams field.
	(cli_out_set_stream): Replace the stream field by the new streams
	field.
	* cli-out.h: Include vec.h.
	(ui_filep): New typedef, call DEF_VEC_P for it.
	(struct cli_ui_out_data): Replace the stream and original_stream
	fields by the new streams field.
	* cli/cli-logging.c (set_logging_redirect): Call ui_out_redirect with
	NULL first.  Extend the comment.
	(handle_redirections): Call ui_out_redirect with output.
	* python/py-breakpoint.c (bppy_get_commands): Move ui_out_redirect
	calls outside of the TRY_CATCH block.

gdb/testsuite/
2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/ui-redirect.exp: New file.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11487,7 +11487,7 @@ save_breakpoints (char *filename, int from_tty,
 	fprintf_unfiltered (fp, "  commands\n");
 	
 	ui_out_redirect (uiout, fp);
-	TRY_CATCH (ex, RETURN_MASK_ERROR)
+	TRY_CATCH (ex, RETURN_MASK_ALL)
 	  {
 	    print_command_lines (uiout, tp->commands->commands, 2);
 	  }
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -26,6 +26,7 @@
 #include "cli-out.h"
 #include "gdb_string.h"
 #include "gdb_assert.h"
+#include "vec.h"
 
 typedef struct cli_ui_out_data cli_out_data;
 
@@ -224,11 +225,13 @@ cli_field_fmt (struct ui_out *uiout, int fldno,
 	       va_list args)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;
 
   if (data->suppress_output)
     return;
 
-  vfprintf_filtered (data->stream, format, args);
+  stream = VEC_last (ui_filep, data->streams);
+  vfprintf_filtered (stream, format, args);
 
   if (align != ui_noalign)
     field_separator ();
@@ -238,20 +241,26 @@ static void
 cli_spaces (struct ui_out *uiout, int numspaces)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;
 
   if (data->suppress_output)
     return;
-  print_spaces_filtered (numspaces, data->stream);
+
+  stream = VEC_last (ui_filep, data->streams);
+  print_spaces_filtered (numspaces, stream);
 }
 
 static void
 cli_text (struct ui_out *uiout, const char *string)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;
 
   if (data->suppress_output)
     return;
-  fputs_filtered (string, data->stream);
+
+  stream = VEC_last (ui_filep, data->streams);
+  fputs_filtered (string, stream);
 }
 
 static void ATTRIBUTE_PRINTF (3, 0)
@@ -262,8 +271,13 @@ cli_message (struct ui_out *uiout, int verbosity,
 
   if (data->suppress_output)
     return;
+
   if (ui_out_get_verblvl (uiout) >= verbosity)
-    vfprintf_unfiltered (data->stream, format, args);
+    {
+      struct ui_file *stream = VEC_last (ui_filep, data->streams);
+
+      vfprintf_unfiltered (stream, format, args);
+    }
 }
 
 static void
@@ -280,25 +294,24 @@ static void
 cli_flush (struct ui_out *uiout)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);
 
-  gdb_flush (data->stream);
+  gdb_flush (stream);
 }
 
+/* OUTSTREAM as non-NULL will push OUTSTREAM on the stack of output streams
+   and make it therefore active.  OUTSTREAM as NULL will pop the last pushed
+   output stream; it is an internal error if it does not exist.  */
+
 static int
 cli_redirect (struct ui_out *uiout, struct ui_file *outstream)
 {
   cli_out_data *data = ui_out_data (uiout);
 
   if (outstream != NULL)
-    {
-      data->original_stream = data->stream;
-      data->stream = outstream;
-    }
-  else if (data->original_stream != NULL)
-    {
-      data->stream = data->original_stream;
-      data->original_stream = NULL;
-    }
+    VEC_safe_push (ui_filep, data->streams, outstream);
+  else
+    VEC_pop (ui_filep, data->streams);
 
   return 0;
 }
@@ -315,10 +328,11 @@ out_field_fmt (struct ui_out *uiout, int fldno,
 	       const char *format,...)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);
   va_list args;
 
   va_start (args, format);
-  vfprintf_filtered (data->stream, format, args);
+  vfprintf_filtered (stream, format, args);
 
   va_end (args);
 }
@@ -329,8 +343,9 @@ static void
 field_separator (void)
 {
   cli_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);
 
-  fputc_filtered (' ', data->stream);
+  fputc_filtered (' ', stream);
 }
 
 /* This is the CLI ui-out implementation functions vector */
@@ -364,8 +379,11 @@ struct ui_out_impl cli_ui_out_impl =
 void
 cli_out_data_ctor (cli_out_data *self, struct ui_file *stream)
 {
-  self->stream = stream;
-  self->original_stream = NULL;
+  gdb_assert (stream != NULL);
+
+  self->streams = NULL;
+  VEC_safe_push (ui_filep, self->streams, stream);
+
   self->suppress_output = 0;
 }
 
@@ -385,8 +403,10 @@ struct ui_file *
 cli_out_set_stream (struct ui_out *uiout, struct ui_file *stream)
 {
   cli_out_data *data = ui_out_data (uiout);
-  struct ui_file *old = data->stream;
+  struct ui_file *old;
+  
+  old = VEC_pop (ui_filep, data->streams);
+  VEC_quick_push (ui_filep, data->streams, stream);
 
-  data->stream = stream;
   return old;
 }
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -22,14 +22,19 @@
 #define CLI_OUT_H
 
 #include "ui-out.h"
+#include "vec.h"
+
+/* Used for cli_ui_out_data->streams.  */
+
+typedef struct ui_file *ui_filep;
+DEF_VEC_P (ui_filep);
 
 /* These are exported so that they can be extended by other `ui_out'
    implementations, like TUI's.  */
 
 struct cli_ui_out_data
   {
-    struct ui_file *stream;
-    struct ui_file *original_stream;
+    VEC (ui_filep) *streams;
     int suppress_output;
   };
 
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -118,8 +118,13 @@ set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
   gdb_stdtarg = output;
   logging_no_redirect_file = new_logging_no_redirect_file;
 
-  /* It should not happen, the redirection has been already setup.  */
-  if (ui_out_redirect (uiout, output) < 0)
+  /* There is a former output pushed on the ui_out_redirect stack.  We want to
+     replace it by OUTPUT so we must pop the former value first.  We should
+     either do both the pop and push or to do neither of it.  At least do not
+     try to push OUTPUT if the pop already failed.  */
+
+  if (ui_out_redirect (uiout, NULL) < 0
+      || ui_out_redirect (uiout, output) < 0)
     warning (_("Current output protocol does not support redirection"));
 
   if (logging_redirect != 0)
@@ -212,7 +217,7 @@ handle_redirections (int from_tty)
   gdb_stdlog = output;
   gdb_stdtarg = output;
 
-  if (ui_out_redirect (uiout, gdb_stdout) < 0)
+  if (ui_out_redirect (uiout, output) < 0)
     warning (_("Current output protocol does not support redirection"));
 }
 
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -474,12 +474,12 @@ bppy_get_commands (PyObject *self, void *closure)
   string_file = mem_fileopen ();
   chain = make_cleanup_ui_file_delete (string_file);
 
+  ui_out_redirect (uiout, string_file);
   TRY_CATCH (except, RETURN_MASK_ALL)
     {
-      ui_out_redirect (uiout, string_file);
       print_command_lines (uiout, breakpoint_commands (bp), 0);
-      ui_out_redirect (uiout, NULL);
     }
+  ui_out_redirect (uiout, NULL);
   cmdstr = ui_file_xstrdup (string_file, &length);
   GDB_PY_HANDLE_EXCEPTION (except);
 
--- /dev/null
+++ b/gdb/testsuite/gdb.base/ui-redirect.exp
@@ -0,0 +1,41 @@
+# Copyright (C) 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if { [prepare_for_testing ui-redirect.exp ui-redirect start.c] } {
+    return -1
+}
+
+gdb_breakpoint main
+
+set test "commands"
+gdb_test_multiple $test $test {
+    -re "End with a line saying just \"end\"\\.\r\n>$" {
+	pass $test
+    }
+}
+
+set test "print 1"
+gdb_test_multiple $test $test {
+    -re "\r\n>$" {
+	pass $test
+    }
+}
+gdb_test_no_output "end"
+
+gdb_test_no_output "set logging file /dev/null"
+gdb_test "set logging on" "Copying output to /dev/null\\."
+gdb_test "save breakpoints /dev/null" "Saved to file '/dev/null'\\."
+gdb_test "set logging off" "Done logging to /dev/null\\."
+gdb_test "help" "List of classes of commands:.*"

Re: [patch] Fix nesting of ui_out_redirect

[Pedro Alves]

On Friday 03 September 2010 16:23:32, Jan Kratochvil wrote:

> gdb/
> 2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* breakpoint.c (save_breakpoints): Use RETURN_MASK_ALL.
> 	* cli-out.c: Include vec.h.
> 	(cli_field_fmt, cli_spaces, cli_text, cli_message, cli_flush): New
> 	variable stream, initialize it, use it.
> 	(cli_redirect): New function comment.  Replace the stream and
> 	original_stream fields by the new streams field.  Remove the
> 	original_stream != NULL conditional, assert error on NULL instead.
> 	(out_field_fmt, field_separator): New variable stream, initialize it, use it.
> 	(cli_out_data_ctor): Assert non-NULL stream.  Replace the stream and
> 	original_stream fields by the new streams field.
> 	(cli_out_set_stream): Replace the stream field by the new streams
> 	field.
> 	* cli-out.h: Include vec.h.
> 	(ui_filep): New typedef, call DEF_VEC_P for it.
> 	(struct cli_ui_out_data): Replace the stream and original_stream
> 	fields by the new streams field.
> 	* cli/cli-logging.c (set_logging_redirect): Call ui_out_redirect with
> 	NULL first.  Extend the comment.
> 	(handle_redirections): Call ui_out_redirect with output.
> 	* python/py-breakpoint.c (bppy_get_commands): Move ui_out_redirect
> 	calls outside of the TRY_CATCH block.
> 
> gdb/testsuite/
> 2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/ui-redirect.exp: New file.

Okay.

-- 
Pedro Alves

Re: [patch] Fix nesting of ui_out_redirect

[Jan Kratochvil]

On Fri, 03 Sep 2010 17:33:23 +0200, Pedro Alves wrote:
> On Friday 03 September 2010 16:23:32, Jan Kratochvil wrote:
> 
> > gdb/
> > 2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* breakpoint.c (save_breakpoints): Use RETURN_MASK_ALL.
> > 	* cli-out.c: Include vec.h.
> > 	(cli_field_fmt, cli_spaces, cli_text, cli_message, cli_flush): New
> > 	variable stream, initialize it, use it.
> > 	(cli_redirect): New function comment.  Replace the stream and
> > 	original_stream fields by the new streams field.  Remove the
> > 	original_stream != NULL conditional, assert error on NULL instead.
> > 	(out_field_fmt, field_separator): New variable stream, initialize it, use it.
> > 	(cli_out_data_ctor): Assert non-NULL stream.  Replace the stream and
> > 	original_stream fields by the new streams field.
> > 	(cli_out_set_stream): Replace the stream field by the new streams
> > 	field.
> > 	* cli-out.h: Include vec.h.
> > 	(ui_filep): New typedef, call DEF_VEC_P for it.
> > 	(struct cli_ui_out_data): Replace the stream and original_stream
> > 	fields by the new streams field.
> > 	* cli/cli-logging.c (set_logging_redirect): Call ui_out_redirect with
> > 	NULL first.  Extend the comment.
> > 	(handle_redirections): Call ui_out_redirect with output.
> > 	* python/py-breakpoint.c (bppy_get_commands): Move ui_out_redirect
> > 	calls outside of the TRY_CATCH block.
> > 
> > gdb/testsuite/
> > 2010-09-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* gdb.base/ui-redirect.exp: New file.
> 
> Okay.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-09/msg00031.html


Thanks,
Jan

Re: [patch] Fix nesting of ui_out_redirect

[Pedro Alves]

On Friday 03 September 2010 16:23:32, Jan Kratochvil wrote:
> > It looks easy to tweak vec.h to get rid of the typedef, and so be able to
> > forward declare VECs.  E.g.,:
> 
> OK but that is not a part of this patch and the typedef is the GDB norm now.

Excuse me for coming back to this, but I'd like to leave a note for the
archives, as I'm not sure there wasn't some confusion what typedef I
was talking about (and I have a feeling I'll point at this thread
some time in the future):

> Index: src/gdb/vec.h
> ===================================================================
> --- src.orig/gdb/vec.h  2010-06-16 12:36:45.000000000 +0100
> +++ src/gdb/vec.h       2010-09-03 13:45:33.000000000 +0100
> @@ -392,16 +392,21 @@ extern void *vec_o_reserve (void *, int,
>  #define vec_assert(expr, op) \
>    ((void)((expr) ? 0 : (gdb_assert_fail (op, file_, line_, ASSERT_FUNCTION), 0)))
>  
> -#define VEC(T) VEC_##T
> +#define VEC_TAG(T) VEC_##T
>  #define VEC_OP(T,OP) VEC_##T##_##OP
>  
> +#define DEC_VEC(T)                                                       \
> +  struct VEC_TAG(T)
> +
>  #define VEC_T(T)                                                         \
> -typedef struct VEC(T)                                                    \
> +struct VEC_TAG(T)                                                        \
>  {                                                                        \
>    unsigned num;                                                                  \
>    unsigned alloc;                                                        \
>    T vec[1];                                                              \
> -} VEC(T)
> +}

It's this ^^^typedef I was talking about.  It's an internal detail
to vec.h.  That typedef what prevents forward declaring "VEC(T);" easily.
Getting rid of it allows writing the DEC_VEC (DEClare VECtor) macro as
above.  I was not talking about the ui_filep (the T) typedef.  No client
code would be affected by that change.  Only code that wanted to
forward the declare the VEC would now be able to do so.

> +
> +#define VEC(T) struct VEC_TAG(T)
>  
>  /* Vector of integer-like object.  */
>  #define DEF_VEC_I(T)                                                     \
> (END) 
> 

Pedro also wrote:

> but I'm really not sure it's worth it to have.  Each module that
> wants to use the VEC still needs to "DEF_VEC_P (ui_filep)"
> (or similar), given that that defines the bunch of static inline
> functions that actually manipulate the VEC.  We'd probably
> want something like this in the headers:

> DEC_VEC (ui_filep);
> #define DEF_VEC_ui_filep \
> DEF_VEC_P (ui_filep)

> and then in the .c files that actually use the VEC, write

> DEF_VEC_ui_filep;

> somewhere at the top.  (replace ui_filep with your favorite
> type name, and _P with _I or _O appropriately, of course.)

-- 
Pedro Alves