[1/4] Add debug redirect option

Message ID 20190423130624.94781-2-alan.hayward@arm.com
State New
Headers show
Series
  • Capture GDB debug when running the testsuite
Related show

Commit Message

Alan Hayward April 23, 2019, 1:06 p.m.
When logging is enabled, output will be sent to both a logfile
and standard terminal output.  The redirect option sends output
only to the logfile.  This includes all debug output.

Add the option to redirect debug output seperately to normal
output, using the cli command:

  set logging debugredirect on

By setting this and enabling logging, all output and debug will
be sent to the logfile.  The user will still see all output but
no debug output.

This causes a change in behaviour for anyone currently using
logging redirect, as now only output will be redirected.  Users
will have to issue the additional command above to also redirect
debug.

[
An alternative would be to add an extra option to the original
redirect command:
  set logging redirect on|off|debug
This method is less flexible, but may be more intuative - setting to
debug would cause just debug to be redirected.  It would break
existing scripts that enable using "set logging redirect 1" instead
of "set logging redirect on".
]

Remove the cli_set_logging extern - the function no longer exists
anywhere.

The new version of make_logging_output became too awkward so I placed
the code inside the set_logging callers.

[ I'm also a little unsure if I have the "release" logic correct. ]

gdb/ChangeLog:

2019-04-23  Alan Hayward  <alan.hayward@arm.com>

	* cli/cli-interp.c (cli_interp_base::set_logging): Add
	debug_redirect parameter.
	* cli/cli-interp.h (make_logging_output): Likewise.
	(make_logging_output): Remove declaration.
	(cli_set_logging): Likewise.
	* cli/cli-logging.c (debug_redirect): Add variable.
	(pop_output_files): Add default param.
	(make_logging_output): Remove function.
	(handle_redirections): Print debug setting.
	(show_logging_command): Likewise.
	(_initialize_cli_logging): Add debugredirect command.
	* interps.c (current_interp_set_logging): Add
	debug_redirect parameter.
	* interps.h (current_interp_named_p): Likewise.
	(current_interp_set_logging): Likewise.
	* mi/mi-common.h: Likewise.
	* mi/mi-interp.c (mi_interp::set_logging): Likewise.
---
 gdb/cli/cli-interp.c  | 25 +++++++++++++-----------
 gdb/cli/cli-interp.h  | 21 ++------------------
 gdb/cli/cli-logging.c | 45 ++++++++++++++++++++++++-------------------
 gdb/interps.c         |  6 +++---
 gdb/interps.h         | 10 +++++++---
 gdb/mi/mi-common.h    |  3 ++-
 gdb/mi/mi-interp.c    | 15 +++++++++++----
 7 files changed, 64 insertions(+), 61 deletions(-)

-- 
2.20.1 (Apple Git-117)

Comments

Tom Tromey April 25, 2019, 2:26 p.m. | #1
>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:


Alan> Add the option to redirect debug output seperately to normal
Alan> output, using the cli command:

Alan>   set logging debugredirect on

[...]

Alan> [
Alan> An alternative would be to add an extra option to the original
Alan> redirect command:
Alan>   set logging redirect on|off|debug
Alan> This method is less flexible, but may be more intuative - setting to
Alan> debug would cause just debug to be redirected.  It would break
Alan> existing scripts that enable using "set logging redirect 1" instead
Alan> of "set logging redirect on".
Alan> ]

I think the choice you implemented here is fine.

Alan> [ I'm also a little unsure if I have the "release" logic correct. ]

One way to improve this code would be to change tee_file to use
ui_file_up.  That way the call to the constructor wouldn't need to use
release(), just std::move.  See the appended.

Tom

diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index 3a5e14de3c7..8e34bfc12bc 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -97,13 +97,7 @@ make_logging_output (ui_file *curr_output, ui_file_up logfile,
   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;
-    }
+    return new tee_file (curr_output, std::move (logfile));
 }
 
 /* This is a helper for the `set logging' command.  */
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 77f6b31ce4b..f18738af624 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -283,20 +283,13 @@ stderr_file::stderr_file (FILE *stream)
 
 
 
-tee_file::tee_file (ui_file *one, bool close_one,
-		    ui_file *two, bool close_two)
+tee_file::tee_file (ui_file *one, ui_file_up &&two)
   : m_one (one),
-    m_two (two),
-    m_close_one (close_one),
-    m_close_two (close_two)
+    m_two (std::move (two))
 {}
 
 tee_file::~tee_file ()
 {
-  if (m_close_one)
-    delete m_one;
-  if (m_close_two)
-    delete m_two;
 }
 
 void
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 6e6ca1c9cdc..a932f215416 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -243,11 +243,9 @@ public:
 class tee_file : public ui_file
 {
 public:
-  /* Create a file which writes to both ONE and TWO.  CLOSE_ONE and
-     CLOSE_TWO indicate whether the original files should be closed
-     when the new file is closed.  */
-  tee_file (ui_file *one, bool close_one,
-	    ui_file *two, bool close_two);
+  /* Create a file which writes to both ONE and TWO.  ONE will remain
+     open when this object is destroyed; but TWO will be closed.  */
+  tee_file (ui_file *one, ui_file_up &&two);
   ~tee_file () override;
 
   void write (const char *buf, long length_buf) override;
@@ -258,10 +256,9 @@ public:
   void flush () override;
 
 private:
-  /* The two underlying ui_files, and whether they should each be
-     closed on destruction.  */
-  ui_file *m_one, *m_two;
-  bool m_close_one, m_close_two;
+  /* The two underlying ui_files.  */
+  ui_file *m_one;
+  ui_file_up m_two;
 };
 
 #endif

Patch

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index c150f40fee..331b4b2a65 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -401,7 +401,8 @@  static saved_output_files saved_output;
 /* See cli-interp.h.  */
 
 void
-cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect)
+cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
+			      bool debug_redirect)
 {
   if (logfile != NULL)
     {
@@ -411,16 +412,18 @@  cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect)
       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;
+      ui_file *tee = nullptr;
+      /* Note that the "tee" takes ownership of the log file.  */
+      if (!logging_redirect || !debug_redirect)
+	tee = new tee_file (gdb_stdout, false, logfile.get (), true);
+
+      ui_file *released_log = logfile.release ();
+
+      gdb_stdout = logging_redirect ? released_log : tee;
+      gdb_stdlog = debug_redirect ? released_log : tee;
+      gdb_stderr = logging_redirect ? released_log : tee;
+      gdb_stdtarg = logging_redirect ? released_log : tee;
+      gdb_stdtargerr = logging_redirect ? released_log : tee;
     }
   else
     {
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index 77d73a23d0..5c93413113 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -28,29 +28,12 @@  public:
   explicit cli_interp_base (const char *name);
   virtual ~cli_interp_base () = 0;
 
-  void set_logging (ui_file_up logfile, bool logging_redirect) override;
+  void set_logging (ui_file_up logfile, bool logging_redirect,
+		    bool debug_redirect) override;
   void pre_command_loop () override;
   bool supports_command_editing () override;
 };
 
-/* 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 3a5e14de3c..bef5f3939b 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -62,6 +62,7 @@  show_logging_overwrite (struct ui_file *file, int from_tty,
 
 /* Value as configured by the user.  */
 static int logging_redirect;
+static int debug_redirect;
 
 static void
 set_logging_redirect (const char *args,
@@ -81,31 +82,13 @@  show_logging_redirect (struct ui_file *file, int from_tty,
 static void
 pop_output_files (void)
 {
-  current_interp_set_logging (NULL, false);
+  current_interp_set_logging (NULL, false, 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)
@@ -130,12 +113,20 @@  handle_redirections (int from_tty)
       else
 	fprintf_unfiltered (gdb_stdout, "Redirecting output to %s.\n",
 			    logging_filename);
+
+      if (!debug_redirect)
+	fprintf_unfiltered (gdb_stdout, "Copying debug output to %s.\n",
+			    logging_filename);
+      else
+	fprintf_unfiltered (gdb_stdout, "Redirecting debug output to %s.\n",
+			    logging_filename);
     }
 
   saved_filename = xstrdup (logging_filename);
 
   /* Let the interpreter do anything it needs.  */
-  current_interp_set_logging (std::move (log), logging_redirect);
+  current_interp_set_logging (std::move (log), logging_redirect,
+			      debug_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
@@ -203,6 +194,11 @@  show_logging_command (const char *args, int from_tty)
     printf_unfiltered (_("Output will be sent only to the log file.\n"));
   else
     printf_unfiltered (_("Output will be logged and displayed.\n"));
+
+  if (debug_redirect)
+    printf_unfiltered (_("Debug output will be sent only to the log file.\n"));
+  else
+    printf_unfiltered (_("Debug output will be logged and displayed.\n"));
 }
 
 void
@@ -231,6 +227,15 @@  If redirect is on, output will go only to the log file."),
 			   set_logging_redirect,
 			   show_logging_redirect,
 			   &set_logging_cmdlist, &show_logging_cmdlist);
+  add_setshow_boolean_cmd ("debugredirect", class_support,
+			   &debug_redirect, _("\
+Set the logging debug output mode."), _("\
+Show the logging debug output mode."), _("\
+If debug redirect is off, debug will go to both the screen and the log file.\n\
+If debug redirect is on, debug will go only to the log file."),
+			   set_logging_redirect,
+			   show_logging_redirect,
+			   &set_logging_cmdlist, &show_logging_cmdlist);
   add_setshow_filename_cmd ("file", class_support, &logging_filename, _("\
 Set the current logfile."), _("\
 Show the current logfile."), _("\
diff --git a/gdb/interps.c b/gdb/interps.c
index b62e33339a..f502a664d9 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -254,13 +254,13 @@  set_top_level_interpreter (const char *name)
 }
 
 void
-current_interp_set_logging (ui_file_up logfile,
-			    bool logging_redirect)
+current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
+			    bool debug_redirect)
 {
   struct ui_interp_info *ui_interp = get_current_interp_info ();
   struct interp *interp = ui_interp->current_interpreter;
 
-  interp->set_logging (std::move (logfile), logging_redirect);
+  interp->set_logging (std::move (logfile), logging_redirect, debug_redirect);
 }
 
 /* Temporarily overrides the current interpreter.  */
diff --git a/gdb/interps.h b/gdb/interps.h
index 1bdc56c839..7cdb4d97e8 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -61,7 +61,8 @@  public:
   /* Provides a hook for interpreters to do any additional
      setup/cleanup that they might need when logging is enabled or
      disabled.  */
-  virtual void set_logging (ui_file_up logfile, bool logging_redirect) = 0;
+  virtual void set_logging (ui_file_up logfile, bool logging_redirect,
+			    bool debug_redirect) = 0;
 
   /* Called before starting an event loop, to give the interpreter a
      chance to e.g., print a prompt.  */
@@ -141,9 +142,12 @@  extern int current_interp_named_p (const char *name);
    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.  */
+   (i.e., the terminal) and the log file.  DEBUG_REDIRECT is same as
+   LOGGING_REDIRECT, but for the value of "set logging debugredirect"
+   instead.  */
 extern void current_interp_set_logging (ui_file_up logfile,
-					bool logging_redirect);
+					bool logging_redirect,
+					bool debug_redirect);
 
 /* Returns the top-level interpreter.  */
 extern struct interp *top_level_interpreter (void);
diff --git a/gdb/mi/mi-common.h b/gdb/mi/mi-common.h
index 4fb2d75c0e..55b708abff 100644
--- a/gdb/mi/mi-common.h
+++ b/gdb/mi/mi-common.h
@@ -66,7 +66,8 @@  public:
   void suspend () override;
   gdb_exception exec (const char *command_str) override;
   ui_out *interp_ui_out () override;
-  void set_logging (ui_file_up logfile, bool logging_redirect) override;
+  void set_logging (ui_file_up logfile, bool logging_redirect,
+		    bool debug_redirect) override;
   void pre_command_loop () override;
 
   /* MI's output channels */
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 25c79b150e..db16e990cc 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -1279,17 +1279,24 @@  mi_interp::interp_ui_out ()
    the consoles to use the supplied ui-file(s).  */
 
 void
-mi_interp::set_logging (ui_file_up logfile, bool logging_redirect)
+mi_interp::set_logging (ui_file_up logfile, bool logging_redirect,
+			bool debug_redirect)
 {
   struct mi_interp *mi = this;
 
   if (logfile != NULL)
     {
       mi->saved_raw_stdout = mi->raw_stdout;
-      mi->raw_stdout = make_logging_output (mi->raw_stdout,
-					    std::move (logfile),
-					    logging_redirect);
 
+      if (logging_redirect)
+	mi->raw_stdout = logfile.release ();
+      else
+	{
+	  /* Note that the "tee" takes ownership of the log file.  */
+	  ui_file *tee = new tee_file (gdb_stdout, false, logfile.get (), true);
+	  logfile.release ();
+	  mi->raw_stdout = tee;
+	}
     }
   else
     {