Re: [PATCH v2 5/5] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy

Pedro Alves
2017-01-25T19:31:36Z

On 01/25/2017 05:52 PM, Pedro Alves wrote:
>
>> >
>>> >> + if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
>>> >> perror_with_name (_("set logging"));
>>> >> - cleanups = make_cleanup_ui_file_delete (output);
>>> >> +
>>> >> + output = log.get ();
>>> >>
>>> >> /* Redirects everything to gdb_stdout while this is running. */
>>> >> if (!logging_redirect)
>>> >> {
>>> >> no_redirect_file = output;
>>> >>
>>> >> - output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0);
>>> >> - if (output == NULL)
>>> >> - perror_with_name (_("set logging"));
>>> >> - make_cleanup_ui_file_delete (output);
>>> >> + output = new tee_file (gdb_stdout, 0, no_redirect_file, 0);
>> >
>> > Is there anything that replaces this cleanup? IOW, if the
>> > fprinf_unfiltered just below did throw an exception for some reason,
>> > would this tee_file leak?
> Looks like there isn't. I'll fix it. Bah, this is going to
> mess up a follow-up patch to push down the tee to the interpreter
> callback that I had prepared meanwhile. :-)
>

Here's the diff to the incoming v3. I think this addresses all
your comments. The only other change is that I realized that
ui_file should be abstract, so that people don't try to allocate
one to build a "/dev/null" stream. I've checked the AIX build
on gcc119. I'll send v3 shortly.

The "tee" patch I was mentioning will make
current_interp_set_logging take a ui_file_up and a bool
instead of two ui_file * pointers, so some of the ".get()s"
will end up disappearing (if that goes in).

From 4e444042b03c07f8387c872f9ded53bb81082a90 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 25 Jan 2017 18:52:31 +0000
Subject: [PATCH] v2-review

---
gdb/aix-thread.c | 3 +--
gdb/arm-tdep.c | 11 ++++-------
gdb/cli/cli-logging.c | 34 +++++++++++++++++-----------------
gdb/mi/mi-console.c | 6 ------
gdb/mi/mi-console.h | 2 --
gdb/serial.c | 2 +-
gdb/ui-file.h | 18 ++++++++++++++----
gdb/utils.h | 3 ---
8 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index cc14d9d..cf1a462 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -1749,7 +1749,6 @@ static char *
aix_thread_extra_thread_info (struct target_ops *self,
struct thread_info *thread)
{
- struct ui_file *buf;
int status;
pthdb_pthread_t pdtid;
pthdb_tid_t tid;
@@ -1797,7 +1796,7 @@ aix_thread_extra_thread_info (struct target_ops *self,

xfree (ret); /* Free old buffer. */

- ret = xstrdup (buf.string ());
+ ret = xstrdup (buf.c_str ());

return ret;
}
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index bc170b1..efce97b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9578,7 +9578,6 @@ _initialize_arm_tdep (void)
const char *setdesc;
const char *const *regnames;
int i;
- static std::string helptext;
char regdesc[1024], *rdptr = regdesc;
size_t rest = sizeof (regdesc);

@@ -9644,12 +9643,10 @@ _initialize_arm_tdep (void)
valid_disassembly_styles[num_disassembly_options] = NULL;

/* Create the help text. */
- string_file stb;
- stb.printf ("%s%s%s",
- _("The valid values are:\n"),
- regdesc,
- _("The default is \"std\"."));
- helptext = std::move (stb.string ());
+ std::string helptext = string_printf ("%s%s%s",
+ _("The valid values are:\n"),
+ regdesc,
+ _("The default is \"std\"."));

add_setshow_enum_cmd("disassembler", no_class,
valid_disassembly_styles, &disassembly_style,
diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index f9a845e..f165896 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -178,8 +178,8 @@ pop_output_files (void)
static void
handle_redirections (int from_tty)
{
- struct ui_file *output;
- struct ui_file *no_redirect_file = NULL;
+ ui_file_up output;
+ ui_file_up no_redirect_file;

if (saved_filename != NULL)
{
@@ -188,34 +188,30 @@ handle_redirections (int from_tty)
return;
}

- std::unique_ptr<stdio_file> log (new stdio_file ());
+ stdio_file_up log (new stdio_file ());
if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
perror_with_name (_("set logging"));

- output = log.get ();
-
/* Redirects everything to gdb_stdout while this is running. */
if (!logging_redirect)
{
- no_redirect_file = output;
+ no_redirect_file = std::move (log);
+ output.reset (new tee_file (gdb_stdout, 0, no_redirect_file.get (), 0));

- output = new tee_file (gdb_stdout, 0, no_redirect_file, 0);
if (from_tty)
fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
logging_filename);
- logging_no_redirect_file = no_redirect_file;
}
else
{
gdb_assert (logging_no_redirect_file == NULL);
+ output = std::move (log);

if (from_tty)
fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
logging_filename);
}

- log.release ();
-
saved_filename = xstrdup (logging_filename);
saved_output.out = gdb_stdout;
saved_output.err = gdb_stderr;
@@ -224,18 +220,22 @@ handle_redirections (int from_tty)
saved_output.targerr = gdb_stdtargerr;

/* Let the interpreter do anything it needs. */
- if (current_interp_set_logging (1, output, no_redirect_file) == 0)
+ if (current_interp_set_logging (1, output.get (),
+ no_redirect_file.get ()) == 0)
{
- gdb_stdout = output;
- gdb_stdlog = output;
- gdb_stderr = output;
- gdb_stdtarg = output;
- gdb_stdtargerr = output;
+ gdb_stdout = output.get ();
+ gdb_stdlog = output.get ();
+ gdb_stderr = output.get ();
+ gdb_stdtarg = output.get ();
+ gdb_stdtargerr = output.get ();
}

+ output.release ();
+ logging_no_redirect_file = no_redirect_file.release ();
+
/* Don't do the redirect for MI, it confuses MI's ui-out scheme. */
if (!current_uiout->is_mi_like_p ())
- current_uiout->redirect (output);
+ current_uiout->redirect (gdb_stdout);
}

static void
diff --git a/gdb/mi/mi-console.c b/gdb/mi/mi-console.c
index 218ffbc..7c1c2ac 100644
--- a/gdb/mi/mi-console.c
+++ b/gdb/mi/mi-console.c
@@ -37,12 +37,6 @@ mi_console_file::mi_console_file (ui_file *raw, const char *prefix, char quote)
{}

void
-mi_console_file::puts (const char *buf)
-{
- this->write (buf, strlen (buf));
-}
-
-void
mi_console_file::write (const char *buf, long length_buf)
{
size_t prev_size = m_buffer.size ();
diff --git a/gdb/mi/mi-console.h b/gdb/mi/mi-console.h
index a7962a8..289013f 100644
--- a/gdb/mi/mi-console.h
+++ b/gdb/mi/mi-console.h
@@ -39,8 +39,6 @@ public:

void write (const char *buf, long length_buf) override;

- void puts (const char *) override;
-
private:
/* The wrapped raw output stream. */
ui_file *m_raw;
diff --git a/gdb/serial.c b/gdb/serial.c
index 831d2eb..b48b977 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -251,7 +251,7 @@ serial_open_ops_1 (const struct serial_ops *ops, const char *open_name)

if (serial_logfile != NULL)
{
- std::unique_ptr<stdio_file> file (new stdio_file ());
+ stdio_file_up file (new stdio_file ());

if (!file->open (serial_logfile, "w"))
perror_with_name (serial_logfile);
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 3972d50..1cb9693 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -21,13 +21,13 @@

#include <string>

-/* The ui_file base class. */
+/* The abstract ui_file base class. */

class ui_file
{
public:
ui_file ();
- virtual ~ui_file ();
+ virtual ~ui_file () = 0;

/* Public non-virtual API. */

@@ -117,8 +117,16 @@ public:
/* string_file-specific public API. */

/* Accesses the std::string containing the entire output collected
- so far. Returns a non-const reference so that it's easy to move
- the string contents out of the string_file. */
+ so far.
+
+ Returns a non-const reference so that it's easy to move the
+ string contents out of the string_file. E.g.:
+
+ string_file buf;
+ buf.printf (....);
+ buf.printf (....);
+ return std::move (buf.string ());
+ */
std::string &string () { return m_string; }

/* Provide a few convenience methods with the same API as the
@@ -187,6 +195,8 @@ private:
bool m_close_p;
};

+typedef std::unique_ptr<stdio_file> stdio_file_up;
+
/* Like stdio_file, but specifically for stderr.

This exists because there is no real line-buffering on Windows, see
diff --git a/gdb/utils.h b/gdb/utils.h
index aada0a4..f138702 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -66,9 +66,6 @@ char **gdb_buildargv (const char *);

extern struct cleanup *make_cleanup_freeargv (char **);

-struct ui_file;
-extern struct cleanup *make_cleanup_ui_file_delete (struct ui_file *);
-
struct ui_out;
extern struct cleanup *
make_cleanup_ui_out_redirect_pop (struct ui_out *uiout);
--
2.5.5