From: Pedro Alves <pedro@codesourcery.com>
To: gdb-patches@sourceware.org
Cc: Jan Kratochvil <jan.kratochvil@redhat.com>, pebolle@tiscali.nl
Subject: Re: [patch] Fix nesting of ui_out_redirect
Date: Fri, 03 Sep 2010 15:36:00 -0000 [thread overview]
Message-ID: <201009031404.23364.pedro@codesourcery.com> (raw)
In-Reply-To: <20100903112139.GA20855@host1.dyn.jankratochvil.net>
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
next prev parent reply other threads:[~2010-09-03 13:04 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-03 15:33 Jan Kratochvil
2010-09-03 15:36 ` Pedro Alves [this message]
2010-09-03 15:40 ` Jan Kratochvil
2010-09-03 15:40 ` Pedro Alves
2010-09-03 18:01 ` Jan Kratochvil
2010-09-03 18:17 ` Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201009031404.23364.pedro@codesourcery.com \
--to=pedro@codesourcery.com \
--cc=gdb-patches@sourceware.org \
--cc=jan.kratochvil@redhat.com \
--cc=pebolle@tiscali.nl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).