Re: [patch] Fix nesting of ui_out_redirect

Pedro Alves
2010-09-03T13:04:35Z

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