Re: [patch] Fix nesting of ui_out_redirect

[Pedro Alves <pedro at codesourcery dot com>]

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