[PATCH] Move "tee" building down to interpreter::set_logging_proc

[PATCH] Move "tee" building down to interpreter::set_logging_proc

[Pedro Alves]

This patch gets rid of this hack in mi_set_logging:

      /* The tee created already is based on gdb_stdout, which for MI
	 is a console and so we end up in an infinite loop of console
	 writing to ui_file writing to console etc.  So discard the
	 existing tee (it hasn't been used yet, and MI won't ever use
	 it), and create one based on raw_stdout instead.  */

By pushing down responsibility for the tee creation to the
interpreter.  I.e., pushing the CLI bits out of handle_redirections
down to the CLI interpreter's set_logging_proc method.

This fixes a few leaks that I spotted, and then confirmed with
"valgrind --leak-check=full":

[...]
  ==21429== 56 (32 direct, 24 indirect) bytes in 1 blocks are definitely lost in loss record 30,243 of 34,980
  ==21429==    at 0x4C29216: operator new(unsigned long) (vg_replace_malloc.c:334)
  ==21429==    by 0x62D9A9: mi_set_logging(interp*, int, ui_file*, ui_file*) (mi-interp.c:1395)
  ==21429==    by 0x810B8A: current_interp_set_logging(int, ui_file*, ui_file*) (interps.c:360)
  ==21429==    by 0x61C537: handle_redirections(int) (cli-logging.c:162)
  ==21429==    by 0x61C6EC: set_logging_on(char*, int) (cli-logging.c:190)
  ==21429==    by 0x6163BE: do_cfunc(cmd_list_element*, char*, int) (cli-decode.c:105)
  ==21429==    by 0x6193C1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1913)
  ==21429==    by 0x8DB790: execute_command(char*, int) (top.c:674)
  ==21429==    by 0x632AE6: mi_execute_cli_command(char const*, int, char const*) (mi-main.c:2343)
  ==21429==    by 0x6329BA: mi_cmd_execute(mi_parse*) (mi-main.c:2306)
  ==21429==    by 0x631E19: captured_mi_execute_command(ui_out*, mi_parse*) (mi-main.c:1998)
  ==21429==    by 0x632389: mi_execute_command(char const*, int) (mi-main.c:2163)
  ==21429==
[...]
  ==26635== 24 bytes in 1 blocks are definitely lost in loss record 20,740 of 34,995
  ==26635==    at 0x4C29216: operator new(unsigned long) (vg_replace_malloc.c:334)
  ==26635==    by 0x61C355: handle_redirections(int) (cli-logging.c:131)
  ==26635==    by 0x61C6EC: set_logging_on(char*, int) (cli-logging.c:190)
  ==26635==    by 0x6163BE: do_cfunc(cmd_list_element*, char*, int) (cli-decode.c:105)
  ==26635==    by 0x6193C1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1913)
  ==26635==    by 0x8DB7BC: execute_command(char*, int) (top.c:674)
  ==26635==    by 0x7B9132: command_handler(char*) (event-top.c:590)
  ==26635==    by 0x7B94F7: command_line_handler(char*) (event-top.c:780)
  ==26635==    by 0x7B8ABB: gdb_rl_callback_handler(char*) (event-top.c:213)
  ==26635==    by 0x933CE9: rl_callback_read_char (callback.c:220)
  ==26635==    by 0x7B89ED: gdb_rl_callback_read_char_wrapper_noexcept() (event-top.c:175)
  ==26635==    by 0x7B8A49: gdb_rl_callback_read_char_wrapper(void*) (event-top.c:192)

One is fixed by transfering ownership of the log file to the tee.  In
pseudo-patch, since the code was moved at the same time:

 -     out = new tee_file (curr_output, false, logfile.get (), false);
 +     out = new tee_file (curr_output, false, logfile.get (), true);

The other is this bit in mi_set_logging:

    else
      {
 +      delete mi->raw_stdout;

I tried to split the leak fixes to a smaller preparatory patch, but
that was difficult exactly because of the tee hack in
handle_redirections -> mi_set_logging.

gdb/ChangeLog:
2017-01-24  Pedro Alves  <palves@redhat.com>

	* cli/cli-interp.c (struct saved_output_files, saved_output):
	Moved from cli/cli-logging.c.
	(cli_set_logging): New function.
	(cli_interp_procs): Install cli_set_logging.
	* cli/cli-interp.h (make_logging_output, cli_set_logging):
	Declare.
	* cli/cli-logging.c (struct saved_output_files, saved_output):
	Moved to cli/cli-interp.c.
	(pop_output_files): Don't save outputs here.
	(make_logging_output): New function.
	(handle_redirections): Don't build tee nor save previous outputs
	here.
	* interps.c (current_interp_set_logging): Change prototype.
	Assume there's always a set_logging_proc method installed.
	* interps.h (interp_set_logging_ftype): Change prototype.
	(current_interp_set_logging): Change prototype and adjust comment.
	* mi/mi-interp.c (mi_set_logging): Change protototype.  Adjust to
	use make_logging_output.
	* tui/tui-interp.c (tui_interp_procs): Install cli_set_logging.
---
 gdb/cli/cli-interp.c  | 59 +++++++++++++++++++++++++++++++-
 gdb/cli/cli-interp.h  | 16 +++++++++
 gdb/cli/cli-logging.c | 93 +++++++++++++++++----------------------------------
 gdb/interps.c         | 13 +++----
 gdb/interps.h         | 16 ++++-----
 gdb/mi/mi-interp.c    | 30 ++++++-----------
 gdb/tui/tui-interp.c  |  2 +-
 7 files changed, 129 insertions(+), 100 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 8802a5a..e0327f6 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -373,6 +373,63 @@ cli_ui_out (struct interp *self)
   return cli->cli_uiout;
 }
 
+/* These hold the pushed copies of the gdb output files.
+   If NULL then nothing has yet been pushed.  */
+struct saved_output_files
+{
+  ui_file *out;
+  ui_file *err;
+  ui_file *log;
+  ui_file *targ;
+  ui_file *targerr;
+};
+static saved_output_files saved_output;
+
+/* See cli-interp.h.  */
+
+void
+cli_set_logging (struct interp *interp,
+		 ui_file_up logfile, bool logging_redirect)
+{
+  if (logfile != NULL)
+    {
+      saved_output.out = gdb_stdout;
+      saved_output.err = gdb_stderr;
+      saved_output.log = gdb_stdlog;
+      saved_output.targ = gdb_stdtarg;
+      saved_output.targerr = gdb_stdtargerr;
+
+      /* A raw pointer since ownership is transferred to
+	 gdb_stdout.  */
+      ui_file *output = make_logging_output (gdb_stdout,
+					     std::move (logfile),
+					     logging_redirect);
+      gdb_stdout = output;
+      gdb_stdlog = output;
+      gdb_stderr = output;
+      gdb_stdtarg = output;
+      gdb_stdtargerr = output;
+    }
+  else
+    {
+      /* Only delete one of the files -- they are all set to the same
+	 value.  */
+      delete gdb_stdout;
+
+      gdb_stdout = saved_output.out;
+      gdb_stderr = saved_output.err;
+      gdb_stdlog = saved_output.log;
+      gdb_stdtarg = saved_output.targ;
+      gdb_stdtargerr = saved_output.targerr;
+
+      saved_output.out = NULL;
+      saved_output.err = NULL;
+      saved_output.log = NULL;
+      saved_output.targ = NULL;
+      saved_output.targerr = NULL;
+    }
+}
+
 /* The CLI interpreter's vtable.  */
 
 static const struct interp_procs cli_interp_procs = {
@@ -381,7 +438,7 @@ static const struct interp_procs cli_interp_procs = {
   cli_interpreter_suspend,	/* suspend_proc */
   cli_interpreter_exec,		/* exec_proc */
   cli_ui_out,			/* ui_out_proc */
-  NULL,                       	/* set_logging_proc */
+  cli_set_logging,		/* set_logging_proc */
   cli_interpreter_pre_command_loop, /* pre_command_loop_proc */
   cli_interpreter_supports_command_editing, /* supports_command_editing_proc */
 };
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index ef86372..abae3d6 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -20,6 +20,22 @@
 
 struct interp;
 
+/* Make the output ui_file to use when logging is enabled.
+   CURR_OUTPUT is the current stream where output is currently being
+   sent to.  LOGFILE is the already-open log file.  LOGGING_REDIRECT
+   is true if the output is to be the logfile, and false if the output
+   stream is to be a tee, with the log file as one of the outputs.
+   Ownership of the log file is transferred to the returned output
+   file.  The returned output file is an owning pointer.  */
+extern ui_file *make_logging_output (ui_file *curr_output,
+				     ui_file_up logfile,
+				     bool logging_redirect);
+
+/* The CLI interpreter's set_logging_proc method.  Exported so other
+   interpreters can reuse it.  */
+extern void cli_set_logging (struct interp *interp,
+			     ui_file_up logfile, bool logging_redirect);
+
 extern int cli_interpreter_supports_command_editing (struct interp *interp);
 
 extern void cli_interpreter_pre_command_loop (struct interp *self);
diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index edb8313..2d8b86b 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -22,17 +22,6 @@
 #include "ui-out.h"
 #include "interps.h"
 
-/* These hold the pushed copies of the gdb output files.
-   If NULL then nothing has yet been pushed.  */
-struct saved_output_files
-{
-  struct ui_file *out;
-  struct ui_file *err;
-  struct ui_file *log;
-  struct ui_file *targ;
-  struct ui_file *targerr;
-};
-static struct saved_output_files saved_output;
 static char *saved_filename;
 
 static char *logging_filename;
@@ -90,37 +79,36 @@ show_logging_redirect (struct ui_file *file, int from_tty,
 static void
 pop_output_files (void)
 {
-  if (current_interp_set_logging (0, NULL, NULL) == 0)
-    {
-      /* Only delete one of the files -- they are all set to the same
-	 value.  */
-      delete gdb_stdout;
-
-      gdb_stdout = saved_output.out;
-      gdb_stderr = saved_output.err;
-      gdb_stdlog = saved_output.log;
-      gdb_stdtarg = saved_output.targ;
-      gdb_stdtargerr = saved_output.targerr;
-    }
-
-  saved_output.out = NULL;
-  saved_output.err = NULL;
-  saved_output.log = NULL;
-  saved_output.targ = NULL;
-  saved_output.targerr = NULL;
+  current_interp_set_logging (NULL, false);
 
   /* Stay consistent with handle_redirections.  */
   if (!current_uiout->is_mi_like_p ())
     current_uiout->redirect (NULL);
 }
 
+/* See cli-interp.h.  */
+
+ui_file *
+make_logging_output (ui_file *curr_output, ui_file_up logfile,
+		     bool logging_redirect)
+{
+  ui_file *out;
+
+  if (logging_redirect)
+    return logfile.release ();
+  else
+    {
+      /* Note that the "tee" takes ownership of the log file.  */
+      out = new tee_file (curr_output, false, logfile.get (), true);
+      logfile.release ();
+      return out;
+    }
+}
+
 /* This is a helper for the `set logging' command.  */
 static void
 handle_redirections (int from_tty)
 {
-  ui_file_up output;
-  ui_file_up no_redirect_file;
-
   if (saved_filename != NULL)
     {
       fprintf_unfiltered (gdb_stdout, "Already logging to %s.\n",
@@ -133,46 +121,27 @@ handle_redirections (int from_tty)
     perror_with_name (_("set logging"));
 
   /* Redirects everything to gdb_stdout while this is running.  */
-  if (!logging_redirect)
+  if (from_tty)
     {
-      no_redirect_file = std::move (log);
-      output.reset (new tee_file (gdb_stdout, 0, no_redirect_file.get (), 0));
-
-      if (from_tty)
+      if (!logging_redirect)
 	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
 			    logging_filename);
-    }
-  else
-    {
-      output = std::move (log);
-
-      if (from_tty)
+      else
 	fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
 			    logging_filename);
     }
 
   saved_filename = xstrdup (logging_filename);
-  saved_output.out = gdb_stdout;
-  saved_output.err = gdb_stderr;
-  saved_output.log = gdb_stdlog;
-  saved_output.targ = gdb_stdtarg;
-  saved_output.targerr = gdb_stdtargerr;
 
   /* Let the interpreter do anything it needs.  */
-  if (current_interp_set_logging (1, output.get (),
-				  no_redirect_file.get ()) == 0)
-    {
-      gdb_stdout = output.get ();
-      gdb_stdlog = output.get ();
-      gdb_stderr = output.get ();
-      gdb_stdtarg = output.get ();
-      gdb_stdtargerr = output.get ();
-    }
-
-  output.release ();
-  no_redirect_file.release ();
-
-  /* Don't do the redirect for MI, it confuses MI's ui-out scheme.  */
+  current_interp_set_logging (std::move (log), logging_redirect);
+
+  /* Redirect the current ui-out object's output to the log.  Use
+     gdb_stdout, not log, since the interpreter may have created a tee
+     that wraps the log.  Don't do the redirect for MI, it confuses
+     MI's ui-out scheme.  Note that we may get here with MI as current
+     interpreter, but with the current ui_out as a CLI ui_out, with
+     '-interpreter-exec console "set logging on"'.  */
   if (!current_uiout->is_mi_like_p ())
     current_uiout->redirect (gdb_stdout);
 }
diff --git a/gdb/interps.c b/gdb/interps.c
index a2b7ffe..06e7ccf 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -346,18 +346,15 @@ interp_ui_out (struct interp *interp)
   return interp->procs->ui_out_proc (interp);
 }
 
-int
-current_interp_set_logging (int start_log, struct ui_file *out,
-			    struct ui_file *logfile)
+void
+current_interp_set_logging (ui_file_up logfile,
+			    bool logging_redirect)
 {
   struct ui_interp_info *ui_interp = get_current_interp_info ();
   struct interp *interp = ui_interp->current_interpreter;
 
-  if (interp == NULL
-      || interp->procs->set_logging_proc == NULL)
-    return 0;
-
-  return interp->procs->set_logging_proc (interp, start_log, out, logfile);
+  return interp->procs->set_logging_proc (interp, std::move (logfile),
+					  logging_redirect);
 }
 
 /* Temporarily overrides the current interpreter.  */
diff --git a/gdb/interps.h b/gdb/interps.h
index 5ec2b05..af8c27f 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -49,9 +49,9 @@ typedef struct gdb_exception (interp_exec_ftype) (void *data,
 typedef void (interp_pre_command_loop_ftype) (struct interp *self);
 typedef struct ui_out *(interp_ui_out_ftype) (struct interp *self);
 
-typedef int (interp_set_logging_ftype) (struct interp *self, int start_log,
-					struct ui_file *out,
-					struct ui_file *logfile);
+typedef void (interp_set_logging_ftype) (struct interp *self,
+					 ui_file_up logfile,
+					 bool logging_redirect);
 
 typedef int (interp_supports_command_editing_ftype) (struct interp *self);
 
@@ -109,13 +109,13 @@ extern int current_interp_named_p (const char *name);
 
 /* Call this function to give the current interpreter an opportunity
    to do any special handling of streams when logging is enabled or
-   disabled.  START_LOG is 1 when logging is starting, 0 when it ends,
-   and OUT is the stream for the log file; it will be NULL when
-   logging is ending.  LOGFILE is non-NULL if the output streams
+   disabled.  START_LOG is true when logging is starting, false when
+   it ends.  LOGFILE is the stream for the log file; it's NULL when
+   logging is ending.  LOGGING_REDIRECT is false if the output streams
    are to be tees, with the log file as one of the outputs.  */
 
-extern int current_interp_set_logging (int start_log, struct ui_file *out,
-				       struct ui_file *logfile);
+extern void current_interp_set_logging (ui_file_up logfile,
+					bool logging_redirect);
 
 /* Returns opaque data associated with the top-level interpreter.  */
 extern void *top_level_interpreter_data (void);
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index f167a53..aa76989 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -1373,33 +1373,25 @@ mi_ui_out (struct interp *interp)
 /* Do MI-specific logging actions; save raw_stdout, and change all
    the consoles to use the supplied ui-file(s).  */
 
-static int
-mi_set_logging (struct interp *interp, int start_log,
-		struct ui_file *out, struct ui_file *logfile)
+static void
+mi_set_logging (struct interp *interp,
+	        ui_file_up logfile, bool logging_redirect)
 {
   struct mi_interp *mi = (struct mi_interp *) interp_data (interp);
 
-  if (!mi)
-    return 0;
+  gdb_assert (mi != NULL);
 
-  if (start_log)
+  if (logfile != NULL)
     {
-      /* The tee created already is based on gdb_stdout, which for MI
-	 is a console and so we end up in an infinite loop of console
-	 writing to ui_file writing to console etc.  So discard the
-	 existing tee (it hasn't been used yet, and MI won't ever use
-	 it), and create one based on raw_stdout instead.  */
-      if (logfile)
-	{
-	  delete out;
-	  out = new tee_file (mi->raw_stdout, false, logfile, false);
-	}
-
       mi->saved_raw_stdout = mi->raw_stdout;
-      mi->raw_stdout = out;
+      mi->raw_stdout = make_logging_output (mi->raw_stdout,
+					    std::move (logfile),
+					    logging_redirect);
+
     }
   else
     {
+      delete mi->raw_stdout;
       mi->raw_stdout = mi->saved_raw_stdout;
       mi->saved_raw_stdout = NULL;
     }
@@ -1409,8 +1401,6 @@ mi_set_logging (struct interp *interp, int start_log,
   mi->log->set_raw (mi->raw_stdout);
   mi->targ->set_raw (mi->raw_stdout);
   mi->event_channel->set_raw (mi->raw_stdout);
-
-  return 1;
 }
 
 /* The MI interpreter's vtable.  */
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 7893904..e2c0605 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -302,7 +302,7 @@ static const struct interp_procs tui_interp_procs = {
   tui_suspend,
   tui_exec,
   tui_ui_out,
-  NULL,
+  cli_set_logging,
   cli_interpreter_pre_command_loop,
   cli_interpreter_supports_command_editing,
 };
-- 
2.5.5

Re: [PATCH] Move "tee" building down to interpreter::set_logging_proc

[Simon Marchi]

On 2017-02-02 09:28, Pedro Alves wrote:
> @@ -109,13 +109,13 @@ extern int current_interp_named_p (const char 
> *name);
> 
>  /* Call this function to give the current interpreter an opportunity
>     to do any special handling of streams when logging is enabled or
> -   disabled.  START_LOG is 1 when logging is starting, 0 when it ends,
> -   and OUT is the stream for the log file; it will be NULL when
> -   logging is ending.  LOGFILE is non-NULL if the output streams
> +   disabled.  START_LOG is true when logging is starting, false when

START_LOG is not there anymore.  From what I understand, it's replaced 
with LOGFILE being null or not?

Otherwise, LGTM.

Re: [PATCH] Move "tee" building down to interpreter::set_logging_proc

[Pedro Alves]

On 02/02/2017 03:17 PM, Simon Marchi wrote:
> On 2017-02-02 09:28, Pedro Alves wrote:
>> @@ -109,13 +109,13 @@ extern int current_interp_named_p (const char
>> *name);
>>
>>  /* Call this function to give the current interpreter an opportunity
>>     to do any special handling of streams when logging is enabled or
>> -   disabled.  START_LOG is 1 when logging is starting, 0 when it ends,
>> -   and OUT is the stream for the log file; it will be NULL when
>> -   logging is ending.  LOGFILE is non-NULL if the output streams
>> +   disabled.  START_LOG is true when logging is starting, false when
> 
> START_LOG is not there anymore.  From what I understand, it's replaced
> with LOGFILE being null or not?

You're right.  How about this:

-- i/gdb/interps.h
+++ w/gdb/interps.h
@@ -109,10 +109,10 @@ extern int current_interp_named_p (const char *name);

 /* Call this function to give the current interpreter an opportunity
    to do any special handling of streams when logging is enabled or
-   disabled.  START_LOG is true when logging is starting, false when
-   it ends.  LOGFILE is the stream for the log file; it's NULL when
-   logging is ending.  LOGGING_REDIRECT is false if the output streams
-   are to be tees, with the log file as one of the outputs.  */
+   disabled.  LOGFILE is the stream for the log file when logging is
+   starting and is NULL when logging is ending.  LOGGING_REDIRECT is
+   false if the output streams are to be tees, with the log file as
+   one of the outputs.  */

 extern void current_interp_set_logging (ui_file_up logfile,
                                        bool logging_redirect);


OK?

> Otherwise, LGTM.

Thanks!

-- 
Pedro Alves

Re: [PATCH] Move "tee" building down to interpreter::set_logging_proc

[Simon Marchi]

On 2017-02-02 12:39, Pedro Alves wrote:
> On 02/02/2017 03:17 PM, Simon Marchi wrote:
>> On 2017-02-02 09:28, Pedro Alves wrote:
>>> @@ -109,13 +109,13 @@ extern int current_interp_named_p (const char
>>> *name);
>>> 
>>>  /* Call this function to give the current interpreter an opportunity
>>>     to do any special handling of streams when logging is enabled or
>>> -   disabled.  START_LOG is 1 when logging is starting, 0 when it 
>>> ends,
>>> -   and OUT is the stream for the log file; it will be NULL when
>>> -   logging is ending.  LOGFILE is non-NULL if the output streams
>>> +   disabled.  START_LOG is true when logging is starting, false when
>> 
>> START_LOG is not there anymore.  From what I understand, it's replaced
>> with LOGFILE being null or not?
> 
> You're right.  How about this:
> 
> -- i/gdb/interps.h
> +++ w/gdb/interps.h
> @@ -109,10 +109,10 @@ extern int current_interp_named_p (const char 
> *name);
> 
>  /* Call this function to give the current interpreter an opportunity
>     to do any special handling of streams when logging is enabled or
> -   disabled.  START_LOG is true when logging is starting, false when
> -   it ends.  LOGFILE is the stream for the log file; it's NULL when
> -   logging is ending.  LOGGING_REDIRECT is false if the output streams
> -   are to be tees, with the log file as one of the outputs.  */
> +   disabled.  LOGFILE is the stream for the log file when logging is
> +   starting and is NULL when logging is ending.  LOGGING_REDIRECT is
> +   false if the output streams are to be tees, with the log file as
> +   one of the outputs.  */
> 
>  extern void current_interp_set_logging (ui_file_up logfile,
>                                         bool logging_redirect);
> 
> 
> OK?

Yeah sounds good.

Though the pre-existing sentence "...if the output streams are to be 
tees" is not that clear to me, I'm not sure I would understand if I 
didn't already know what the function does.  Why does it talk about 
multiple output streams that have to be tees, isn't there only one tee?  
Or is it meant to be a past tense verb, in which case it should be 
something like "...are to be tee-ed"?  I just find the formulation 
awkward.

Re: [PATCH] Move "tee" building down to interpreter::set_logging_proc

[Pedro Alves]

On 02/02/2017 05:44 PM, Simon Marchi wrote:

> Yeah sounds good.
> 
> Though the pre-existing sentence "...if the output streams are to be
> tees" is not that clear to me, I'm not sure I would understand if I
> didn't already know what the function does.  Why does it talk about
> multiple output streams that have to be tees, isn't there only one tee? 
> Or is it meant to be a past tense verb, in which case it should be
> something like "...are to be tee-ed"?  I just find the formulation awkward.

I think it's saying that all of "gdb_stdout, gdb_stderr, gdb_stdlog", etc.
should all end up as tees.  They happens to end up as the same tee
object, but it's true that they're all tees.

How about this?

diff --git i/gdb/cli/cli-interp.h w/gdb/cli/cli-interp.h
index abae3d6..accecfa 100644
--- i/gdb/cli/cli-interp.h
+++ w/gdb/cli/cli-interp.h
@@ -21,12 +21,14 @@
 struct interp;
 
 /* Make the output ui_file to use when logging is enabled.
-   CURR_OUTPUT is the current stream where output is currently being
-   sent to.  LOGFILE is the already-open log file.  LOGGING_REDIRECT
-   is true if the output is to be the logfile, and false if the output
-   stream is to be a tee, with the log file as one of the outputs.
-   Ownership of the log file is transferred to the returned output
-   file.  The returned output file is an owning pointer.  */
+   CURR_OUTPUT is the stream where output is currently being sent to
+   (e.g., gdb_stdout for the CLI, raw output stream for the MI).
+   LOGFILE is log file already opened by the caller.  LOGGING_REDIRECT
+   is the value of the "set logging redirect" setting.  If true, the
+   resulting output is the logfile.  If false, the output stream is a
+   tee, with the log file as one of the outputs.  Ownership of LOGFILE
+   is transferred to the returned output file, which is an owning
+   pointer.  */
 extern ui_file *make_logging_output (ui_file *curr_output,
 				     ui_file_up logfile,
 				     bool logging_redirect);
diff --git i/gdb/interps.h w/gdb/interps.h
index 7672393..ef2ceeb 100644
--- i/gdb/interps.h
+++ w/gdb/interps.h
@@ -111,9 +111,11 @@ extern int current_interp_named_p (const char *name);
    to do any special handling of streams when logging is enabled or
    disabled.  LOGFILE is the stream for the log file when logging is
    starting and is NULL when logging is ending.  LOGGING_REDIRECT is
-   false if the output streams are to be tees, with the log file as
-   one of the outputs.  */
-
+   the value of the "set logging redirect" setting.  If true, the
+   interpreter should configure the output streams to send output only
+   to the logfile.  If false, the interpreter should configure the
+   output streams to send output to both the current output stream
+   (i.e., the terminal) and the log file.  */
 extern void current_interp_set_logging (ui_file_up logfile,
 					bool logging_redirect);
 

Re: [PATCH] Move "tee" building down to interpreter::set_logging_proc

[Simon Marchi]

On 2017-02-02 13:04, Pedro Alves wrote:
> On 02/02/2017 05:44 PM, Simon Marchi wrote:
> 
>> Yeah sounds good.
>> 
>> Though the pre-existing sentence "...if the output streams are to be
>> tees" is not that clear to me, I'm not sure I would understand if I
>> didn't already know what the function does.  Why does it talk about
>> multiple output streams that have to be tees, isn't there only one 
>> tee?
>> Or is it meant to be a past tense verb, in which case it should be
>> something like "...are to be tee-ed"?  I just find the formulation 
>> awkward.
> 
> I think it's saying that all of "gdb_stdout, gdb_stderr, gdb_stdlog", 
> etc.
> should all end up as tees.  They happens to end up as the same tee
> object, but it's true that they're all tees.
> 
> How about this?
> 
> diff --git i/gdb/cli/cli-interp.h w/gdb/cli/cli-interp.h
> index abae3d6..accecfa 100644
> --- i/gdb/cli/cli-interp.h
> +++ w/gdb/cli/cli-interp.h
> @@ -21,12 +21,14 @@
>  struct interp;
> 
>  /* Make the output ui_file to use when logging is enabled.
> -   CURR_OUTPUT is the current stream where output is currently being
> -   sent to.  LOGFILE is the already-open log file.  LOGGING_REDIRECT
> -   is true if the output is to be the logfile, and false if the output
> -   stream is to be a tee, with the log file as one of the outputs.
> -   Ownership of the log file is transferred to the returned output
> -   file.  The returned output file is an owning pointer.  */
> +   CURR_OUTPUT is the stream where output is currently being sent to
> +   (e.g., gdb_stdout for the CLI, raw output stream for the MI).
> +   LOGFILE is log file already opened by the caller.  LOGGING_REDIRECT
> +   is the value of the "set logging redirect" setting.  If true, the
> +   resulting output is the logfile.  If false, the output stream is a
> +   tee, with the log file as one of the outputs.  Ownership of LOGFILE
> +   is transferred to the returned output file, which is an owning
> +   pointer.  */
>  extern ui_file *make_logging_output (ui_file *curr_output,
>  				     ui_file_up logfile,
>  				     bool logging_redirect);
> diff --git i/gdb/interps.h w/gdb/interps.h
> index 7672393..ef2ceeb 100644
> --- i/gdb/interps.h
> +++ w/gdb/interps.h
> @@ -111,9 +111,11 @@ extern int current_interp_named_p (const char 
> *name);
>     to do any special handling of streams when logging is enabled or
>     disabled.  LOGFILE is the stream for the log file when logging is
>     starting and is NULL when logging is ending.  LOGGING_REDIRECT is
> -   false if the output streams are to be tees, with the log file as
> -   one of the outputs.  */
> -
> +   the value of the "set logging redirect" setting.  If true, the
> +   interpreter should configure the output streams to send output only
> +   to the logfile.  If false, the interpreter should configure the
> +   output streams to send output to both the current output stream
> +   (i.e., the terminal) and the log file.  */
>  extern void current_interp_set_logging (ui_file_up logfile,
>  					bool logging_redirect);

LGTM!

Re: [PATCH] Move "tee" building down to interpreter::set_logging_proc

[Pedro Alves]

On 02/02/2017 08:42 PM, Simon Marchi wrote:
> On 2017-02-02 13:04, Pedro Alves wrote:
>> How about this?
> LGTM!

Many thanks for the review!  I've pushed it in now, as below.
(Added a missing "the".)

From 616268b639780e0819b51053c794037bcde3de16 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 2 Feb 2017 22:00:43 +0000
Subject: [PATCH] Move "tee" building down to interpreter::set_logging_proc

This patch gets rid of this hack in mi_set_logging:

      /* The tee created already is based on gdb_stdout, which for MI
	 is a console and so we end up in an infinite loop of console
	 writing to ui_file writing to console etc.  So discard the
	 existing tee (it hasn't been used yet, and MI won't ever use
	 it), and create one based on raw_stdout instead.  */

By pushing down responsibility for the tee creation to the
interpreter.  I.e., pushing the CLI bits out of handle_redirections
down to the CLI interpreter's set_logging_proc method.

This fixes a few leaks that I spotted, and then confirmed with
"valgrind --leak-check=full":

[...]
  ==21429== 56 (32 direct, 24 indirect) bytes in 1 blocks are definitely lost in loss record 30,243 of 34,980
  ==21429==    at 0x4C29216: operator new(unsigned long) (vg_replace_malloc.c:334)
  ==21429==    by 0x62D9A9: mi_set_logging(interp*, int, ui_file*, ui_file*) (mi-interp.c:1395)
  ==21429==    by 0x810B8A: current_interp_set_logging(int, ui_file*, ui_file*) (interps.c:360)
  ==21429==    by 0x61C537: handle_redirections(int) (cli-logging.c:162)
  ==21429==    by 0x61C6EC: set_logging_on(char*, int) (cli-logging.c:190)
  ==21429==    by 0x6163BE: do_cfunc(cmd_list_element*, char*, int) (cli-decode.c:105)
  ==21429==    by 0x6193C1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1913)
  ==21429==    by 0x8DB790: execute_command(char*, int) (top.c:674)
  ==21429==    by 0x632AE6: mi_execute_cli_command(char const*, int, char const*) (mi-main.c:2343)
  ==21429==    by 0x6329BA: mi_cmd_execute(mi_parse*) (mi-main.c:2306)
  ==21429==    by 0x631E19: captured_mi_execute_command(ui_out*, mi_parse*) (mi-main.c:1998)
  ==21429==    by 0x632389: mi_execute_command(char const*, int) (mi-main.c:2163)
  ==21429==
[...]
  ==26635== 24 bytes in 1 blocks are definitely lost in loss record 20,740 of 34,995
  ==26635==    at 0x4C29216: operator new(unsigned long) (vg_replace_malloc.c:334)
  ==26635==    by 0x61C355: handle_redirections(int) (cli-logging.c:131)
  ==26635==    by 0x61C6EC: set_logging_on(char*, int) (cli-logging.c:190)
  ==26635==    by 0x6163BE: do_cfunc(cmd_list_element*, char*, int) (cli-decode.c:105)
  ==26635==    by 0x6193C1: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1913)
  ==26635==    by 0x8DB7BC: execute_command(char*, int) (top.c:674)
  ==26635==    by 0x7B9132: command_handler(char*) (event-top.c:590)
  ==26635==    by 0x7B94F7: command_line_handler(char*) (event-top.c:780)
  ==26635==    by 0x7B8ABB: gdb_rl_callback_handler(char*) (event-top.c:213)
  ==26635==    by 0x933CE9: rl_callback_read_char (callback.c:220)
  ==26635==    by 0x7B89ED: gdb_rl_callback_read_char_wrapper_noexcept() (event-top.c:175)
  ==26635==    by 0x7B8A49: gdb_rl_callback_read_char_wrapper(void*) (event-top.c:192)

One is fixed by transfering ownership of the log file to the tee.  In
pseudo-patch, since the code was moved at the same time:

 -     out = new tee_file (curr_output, false, logfile.get (), false);
 +     out = new tee_file (curr_output, false, logfile.get (), true);

The other is this bit in mi_set_logging:

    else
      {
 +      delete mi->raw_stdout;

I tried to split the leak fixes to a smaller preparatory patch, but
that was difficult exactly because of the tee hack in
handle_redirections -> mi_set_logging.

gdb/ChangeLog:
2017-02-02  Pedro Alves  <palves@redhat.com>

	* cli/cli-interp.c (struct saved_output_files, saved_output):
	Moved from cli/cli-logging.c.
	(cli_set_logging): New function.
	(cli_interp_procs): Install cli_set_logging.
	* cli/cli-interp.h (make_logging_output, cli_set_logging):
	Declare.
	* cli/cli-logging.c (struct saved_output_files, saved_output):
	Moved to cli/cli-interp.c.
	(pop_output_files): Don't save outputs here.
	(make_logging_output): New function.
	(handle_redirections): Don't build tee nor save previous outputs
	here.
	* interps.c (current_interp_set_logging): Change prototype.
	Assume there's always a set_logging_proc method installed.
	* interps.h (interp_set_logging_ftype): Change prototype.
	(current_interp_set_logging): Change prototype and adjust comment.
	* mi/mi-interp.c (mi_set_logging): Change protototype.  Adjust to
	use make_logging_output.
	* tui/tui-interp.c (tui_interp_procs): Install cli_set_logging.
---
 gdb/ChangeLog         | 21 ++++++++++++
 gdb/cli/cli-interp.c  | 59 ++++++++++++++++++++++++++++++++-
 gdb/cli/cli-interp.h  | 18 ++++++++++
 gdb/cli/cli-logging.c | 92 +++++++++++++++++----------------------------------
 gdb/interps.c         | 13 +++-----
 gdb/interps.h         | 22 ++++++------
 gdb/mi/mi-interp.c    | 30 ++++++-----------
 gdb/tui/tui-interp.c  |  2 +-
 8 files changed, 155 insertions(+), 102 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index df5238c..fbbcc07 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,26 @@
 2017-02-02  Pedro Alves  <palves@redhat.com>
 
+	* cli/cli-interp.c (struct saved_output_files, saved_output):
+	Moved from cli/cli-logging.c.
+	(cli_set_logging): New function.
+	(cli_interp_procs): Install cli_set_logging.
+	* cli/cli-interp.h (make_logging_output, cli_set_logging):
+	Declare.
+	* cli/cli-logging.c (struct saved_output_files, saved_output):
+	Moved to cli/cli-interp.c.
+	(pop_output_files): Don't save outputs here.
+	(make_logging_output): New function.
+	(handle_redirections): Don't build tee nor save previous outputs
+	here.
+	* interps.c (current_interp_set_logging): Change prototype.
+	Assume there's always a set_logging_proc method installed.
+	* interps.h (interp_set_logging_ftype): Change prototype.
+	(current_interp_set_logging): Change prototype and adjust comment.
+	* mi/mi-interp.c (mi_set_logging): Change protototype.  Adjust to
+	use make_logging_output.
+	* tui/tui-interp.c (tui_interp_procs): Install cli_set_logging.
+2017-02-02  Pedro Alves  <palves@redhat.com>
+
 	* cli/cli-logging.c (maybe_warn_already_logging): New factored out
 	from ...
 	(set_logging_overwrite): ... here.
diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 8802a5a..e0327f6 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -373,6 +373,63 @@ cli_ui_out (struct interp *self)
   return cli->cli_uiout;
 }
 
+/* These hold the pushed copies of the gdb output files.
+   If NULL then nothing has yet been pushed.  */
+struct saved_output_files
+{
+  ui_file *out;
+  ui_file *err;
+  ui_file *log;
+  ui_file *targ;
+  ui_file *targerr;
+};
+static saved_output_files saved_output;
+
+/* See cli-interp.h.  */
+
+void
+cli_set_logging (struct interp *interp,
+		 ui_file_up logfile, bool logging_redirect)
+{
+  if (logfile != NULL)
+    {
+      saved_output.out = gdb_stdout;
+      saved_output.err = gdb_stderr;
+      saved_output.log = gdb_stdlog;
+      saved_output.targ = gdb_stdtarg;
+      saved_output.targerr = gdb_stdtargerr;
+
+      /* A raw pointer since ownership is transferred to
+	 gdb_stdout.  */
+      ui_file *output = make_logging_output (gdb_stdout,
+					     std::move (logfile),
+					     logging_redirect);
+      gdb_stdout = output;
+      gdb_stdlog = output;
+      gdb_stderr = output;
+      gdb_stdtarg = output;
+      gdb_stdtargerr = output;
+    }
+  else
+    {
+      /* Only delete one of the files -- they are all set to the same
+	 value.  */
+      delete gdb_stdout;
+
+      gdb_stdout = saved_output.out;
+      gdb_stderr = saved_output.err;
+      gdb_stdlog = saved_output.log;
+      gdb_stdtarg = saved_output.targ;
+      gdb_stdtargerr = saved_output.targerr;
+
+      saved_output.out = NULL;
+      saved_output.err = NULL;
+      saved_output.log = NULL;
+      saved_output.targ = NULL;
+      saved_output.targerr = NULL;
+    }
+}
+
 /* The CLI interpreter's vtable.  */
 
 static const struct interp_procs cli_interp_procs = {
@@ -381,7 +438,7 @@ static const struct interp_procs cli_interp_procs = {
   cli_interpreter_suspend,	/* suspend_proc */
   cli_interpreter_exec,		/* exec_proc */
   cli_ui_out,			/* ui_out_proc */
-  NULL,                       	/* set_logging_proc */
+  cli_set_logging,		/* set_logging_proc */
   cli_interpreter_pre_command_loop, /* pre_command_loop_proc */
   cli_interpreter_supports_command_editing, /* supports_command_editing_proc */
 };
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index ef86372..f93548c 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -20,6 +20,24 @@
 
 struct interp;
 
+/* Make the output ui_file to use when logging is enabled.
+   CURR_OUTPUT is the stream where output is currently being sent to
+   (e.g., gdb_stdout for the CLI, raw output stream for the MI).
+   LOGFILE is the log file already opened by the caller.
+   LOGGING_REDIRECT is the value of the "set logging redirect"
+   setting.  If true, the resulting output is the logfile.  If false,
+   the output stream is a tee, with the log file as one of the
+   outputs.  Ownership of LOGFILE is transferred to the returned
+   output file, which is an owning pointer.  */
+extern ui_file *make_logging_output (ui_file *curr_output,
+				     ui_file_up logfile,
+				     bool logging_redirect);
+
+/* The CLI interpreter's set_logging_proc method.  Exported so other
+   interpreters can reuse it.  */
+extern void cli_set_logging (struct interp *interp,
+			     ui_file_up logfile, bool logging_redirect);
+
 extern int cli_interpreter_supports_command_editing (struct interp *interp);
 
 extern void cli_interpreter_pre_command_loop (struct interp *self);
diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index edb8313..e8ec444 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -22,17 +22,6 @@
 #include "ui-out.h"
 #include "interps.h"
 
-/* These hold the pushed copies of the gdb output files.
-   If NULL then nothing has yet been pushed.  */
-struct saved_output_files
-{
-  struct ui_file *out;
-  struct ui_file *err;
-  struct ui_file *log;
-  struct ui_file *targ;
-  struct ui_file *targerr;
-};
-static struct saved_output_files saved_output;
 static char *saved_filename;
 
 static char *logging_filename;
@@ -90,37 +79,35 @@ show_logging_redirect (struct ui_file *file, int from_tty,
 static void
 pop_output_files (void)
 {
-  if (current_interp_set_logging (0, NULL, NULL) == 0)
-    {
-      /* Only delete one of the files -- they are all set to the same
-	 value.  */
-      delete gdb_stdout;
-
-      gdb_stdout = saved_output.out;
-      gdb_stderr = saved_output.err;
-      gdb_stdlog = saved_output.log;
-      gdb_stdtarg = saved_output.targ;
-      gdb_stdtargerr = saved_output.targerr;
-    }
-
-  saved_output.out = NULL;
-  saved_output.err = NULL;
-  saved_output.log = NULL;
-  saved_output.targ = NULL;
-  saved_output.targerr = NULL;
+  current_interp_set_logging (NULL, false);
 
   /* Stay consistent with handle_redirections.  */
   if (!current_uiout->is_mi_like_p ())
     current_uiout->redirect (NULL);
 }
 
+/* See cli-interp.h.  */
+
+ui_file *
+make_logging_output (ui_file *curr_output, ui_file_up logfile,
+		     bool logging_redirect)
+{
+  if (logging_redirect)
+    return logfile.release ();
+  else
+    {
+      /* Note that the "tee" takes ownership of the log file.  */
+      ui_file *out = new tee_file (curr_output, false,
+				   logfile.get (), true);
+      logfile.release ();
+      return out;
+    }
+}
+
 /* This is a helper for the `set logging' command.  */
 static void
 handle_redirections (int from_tty)
 {
-  ui_file_up output;
-  ui_file_up no_redirect_file;
-
   if (saved_filename != NULL)
     {
       fprintf_unfiltered (gdb_stdout, "Already logging to %s.\n",
@@ -133,46 +120,27 @@ handle_redirections (int from_tty)
     perror_with_name (_("set logging"));
 
   /* Redirects everything to gdb_stdout while this is running.  */
-  if (!logging_redirect)
+  if (from_tty)
     {
-      no_redirect_file = std::move (log);
-      output.reset (new tee_file (gdb_stdout, 0, no_redirect_file.get (), 0));
-
-      if (from_tty)
+      if (!logging_redirect)
 	fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
 			    logging_filename);
-    }
-  else
-    {
-      output = std::move (log);
-
-      if (from_tty)
+      else
 	fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
 			    logging_filename);
     }
 
   saved_filename = xstrdup (logging_filename);
-  saved_output.out = gdb_stdout;
-  saved_output.err = gdb_stderr;
-  saved_output.log = gdb_stdlog;
-  saved_output.targ = gdb_stdtarg;
-  saved_output.targerr = gdb_stdtargerr;
 
   /* Let the interpreter do anything it needs.  */
-  if (current_interp_set_logging (1, output.get (),
-				  no_redirect_file.get ()) == 0)
-    {
-      gdb_stdout = output.get ();
-      gdb_stdlog = output.get ();
-      gdb_stderr = output.get ();
-      gdb_stdtarg = output.get ();
-      gdb_stdtargerr = output.get ();
-    }
-
-  output.release ();
-  no_redirect_file.release ();
-
-  /* Don't do the redirect for MI, it confuses MI's ui-out scheme.  */
+  current_interp_set_logging (std::move (log), logging_redirect);
+
+  /* Redirect the current ui-out object's output to the log.  Use
+     gdb_stdout, not log, since the interpreter may have created a tee
+     that wraps the log.  Don't do the redirect for MI, it confuses
+     MI's ui-out scheme.  Note that we may get here with MI as current
+     interpreter, but with the current ui_out as a CLI ui_out, with
+     '-interpreter-exec console "set logging on"'.  */
   if (!current_uiout->is_mi_like_p ())
     current_uiout->redirect (gdb_stdout);
 }
diff --git a/gdb/interps.c b/gdb/interps.c
index a2b7ffe..06e7ccf 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -346,18 +346,15 @@ interp_ui_out (struct interp *interp)
   return interp->procs->ui_out_proc (interp);
 }
 
-int
-current_interp_set_logging (int start_log, struct ui_file *out,
-			    struct ui_file *logfile)
+void
+current_interp_set_logging (ui_file_up logfile,
+			    bool logging_redirect)
 {
   struct ui_interp_info *ui_interp = get_current_interp_info ();
   struct interp *interp = ui_interp->current_interpreter;
 
-  if (interp == NULL
-      || interp->procs->set_logging_proc == NULL)
-    return 0;
-
-  return interp->procs->set_logging_proc (interp, start_log, out, logfile);
+  return interp->procs->set_logging_proc (interp, std::move (logfile),
+					  logging_redirect);
 }
 
 /* Temporarily overrides the current interpreter.  */
diff --git a/gdb/interps.h b/gdb/interps.h
index 5ec2b05..ef2ceeb 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -49,9 +49,9 @@ typedef struct gdb_exception (interp_exec_ftype) (void *data,
 typedef void (interp_pre_command_loop_ftype) (struct interp *self);
 typedef struct ui_out *(interp_ui_out_ftype) (struct interp *self);
 
-typedef int (interp_set_logging_ftype) (struct interp *self, int start_log,
-					struct ui_file *out,
-					struct ui_file *logfile);
+typedef void (interp_set_logging_ftype) (struct interp *self,
+					 ui_file_up logfile,
+					 bool logging_redirect);
 
 typedef int (interp_supports_command_editing_ftype) (struct interp *self);
 
@@ -109,13 +109,15 @@ extern int current_interp_named_p (const char *name);
 
 /* Call this function to give the current interpreter an opportunity
    to do any special handling of streams when logging is enabled or
-   disabled.  START_LOG is 1 when logging is starting, 0 when it ends,
-   and OUT is the stream for the log file; it will be NULL when
-   logging is ending.  LOGFILE is non-NULL if the output streams
-   are to be tees, with the log file as one of the outputs.  */
-
-extern int current_interp_set_logging (int start_log, struct ui_file *out,
-				       struct ui_file *logfile);
+   disabled.  LOGFILE is the stream for the log file when logging is
+   starting and is NULL when logging is ending.  LOGGING_REDIRECT is
+   the value of the "set logging redirect" setting.  If true, the
+   interpreter should configure the output streams to send output only
+   to the logfile.  If false, the interpreter should configure the
+   output streams to send output to both the current output stream
+   (i.e., the terminal) and the log file.  */
+extern void current_interp_set_logging (ui_file_up logfile,
+					bool logging_redirect);
 
 /* Returns opaque data associated with the top-level interpreter.  */
 extern void *top_level_interpreter_data (void);
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index f167a53..aa76989 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -1373,33 +1373,25 @@ mi_ui_out (struct interp *interp)
 /* Do MI-specific logging actions; save raw_stdout, and change all
    the consoles to use the supplied ui-file(s).  */
 
-static int
-mi_set_logging (struct interp *interp, int start_log,
-		struct ui_file *out, struct ui_file *logfile)
+static void
+mi_set_logging (struct interp *interp,
+	        ui_file_up logfile, bool logging_redirect)
 {
   struct mi_interp *mi = (struct mi_interp *) interp_data (interp);
 
-  if (!mi)
-    return 0;
+  gdb_assert (mi != NULL);
 
-  if (start_log)
+  if (logfile != NULL)
     {
-      /* The tee created already is based on gdb_stdout, which for MI
-	 is a console and so we end up in an infinite loop of console
-	 writing to ui_file writing to console etc.  So discard the
-	 existing tee (it hasn't been used yet, and MI won't ever use
-	 it), and create one based on raw_stdout instead.  */
-      if (logfile)
-	{
-	  delete out;
-	  out = new tee_file (mi->raw_stdout, false, logfile, false);
-	}
-
       mi->saved_raw_stdout = mi->raw_stdout;
-      mi->raw_stdout = out;
+      mi->raw_stdout = make_logging_output (mi->raw_stdout,
+					    std::move (logfile),
+					    logging_redirect);
+
     }
   else
     {
+      delete mi->raw_stdout;
       mi->raw_stdout = mi->saved_raw_stdout;
       mi->saved_raw_stdout = NULL;
     }
@@ -1409,8 +1401,6 @@ mi_set_logging (struct interp *interp, int start_log,
   mi->log->set_raw (mi->raw_stdout);
   mi->targ->set_raw (mi->raw_stdout);
   mi->event_channel->set_raw (mi->raw_stdout);
-
-  return 1;
 }
 
 /* The MI interpreter's vtable.  */
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 7893904..e2c0605 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -302,7 +302,7 @@ static const struct interp_procs tui_interp_procs = {
   tui_suspend,
   tui_exec,
   tui_ui_out,
-  NULL,
+  cli_set_logging,
   cli_interpreter_pre_command_loop,
   cli_interpreter_supports_command_editing,
 };
-- 
2.5.5