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.

8 files changed, 155 insertions, 102 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index df5238c9bd..fbbcc07ade 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 8802a5ae27..e0327f6a99 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 ef86372ae1..f93548c13e 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 edb8313803..e8ec4449b9 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 a2b7ffe944..06e7ccfc8a 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 5ec2b05d8b..ef2ceebeb7 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 f167a535bc..aa769891b2 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 7893904fcb..e2c0605b50 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,
 };