Patchwork [1/9] Change file close behavior for tee_file

login
register
mail settings
Submitter Alan Hayward
Date May 14, 2019, 3:12 p.m.
Message ID <20190514151238.8765-2-alan.hayward@arm.com>
Download mbox | patch
Permalink /patch/32677/
State New
Headers show

Comments

Alan Hayward - May 14, 2019, 3:12 p.m.
Instead of using two bools to decide if the files should close when tee_file
is closed, make file one stay open and file two close.  This simplifies the
use cases for it.

Inline the make_logging_output into the calling functions (the logic here
looks ugly in order to simplify a later change).

Expand ui-redirect.exp to cover the changes, similar to mi-logging.exp.

gdb/ChangeLog:

2019-05-14  Alan Hayward  <alan.hayward@arm.com>
	    Tom Tromey  <tromey@adacore.com>

	* cli/cli-interp.c (cli_interp_base::set_logging): Create tee_file
	directly.
	* cli/cli-interp.h (make_logging_output): Remove declaration.
	* cli/cli-logging.c (make_logging_output): Remove function.
	* mi/mi-interp.c (mi_interp::set_logging): Create tee_file
	directly.
	* ui-file.c (tee_file::tee_file): Remove bools.
	(tee_file::~tee_file): Remove deletes.
	* ui-file.h (tee_file): Remove bools.

gdb/testsuite/ChangeLog:

2019-05-14  Alan Hayward  <alan.hayward@arm.com>

	* gdb.base/ui-redirect.exp: Test redirection.
---
 gdb/cli/cli-interp.c                   | 38 +++++++++++++++-----------
 gdb/cli/cli-interp.h                   | 13 ---------
 gdb/cli/cli-logging.c                  | 18 ------------
 gdb/mi/mi-interp.c                     | 19 +++++++++----
 gdb/testsuite/gdb.base/ui-redirect.exp | 30 ++++++++++++++++----
 gdb/ui-file.c                          | 11 ++------
 gdb/ui-file.h                          | 15 ++++------
 7 files changed, 69 insertions(+), 75 deletions(-)

-- 
2.20.1 (Apple Git-117)
Tom Tromey - May 16, 2019, 6:57 p.m.
>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> Instead of using two bools to decide if the files should close when tee_file
Alan> is closed, make file one stay open and file two close.  This simplifies the
Alan> use cases for it.

Alan> Inline the make_logging_output into the calling functions (the logic here
Alan> looks ugly in order to simplify a later change).

Alan> Expand ui-redirect.exp to cover the changes, similar to mi-logging.exp.

Thanks, this is ok.

Tom

Patch

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c

index fc4b39a9c2a..7876f910806 100644

--- a/gdb/cli/cli-interp.c

+++ b/gdb/cli/cli-interp.c

@@ -403,7 +403,7 @@  static saved_output_files saved_output;

 void
 cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect)
 {
-  if (logfile != NULL)

+  if (logfile != nullptr)

     {
       saved_output.out = gdb_stdout;
       saved_output.err = gdb_stderr;
@@ -411,16 +411,22 @@  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;

+      /* If logging is being redirected, then grab logfile.  */

+      ui_file *logfile_p = nullptr;

+      if (logging_redirect)

+	logfile_p = logfile.release ();

+

+      /* If logging is not being redirected, then a tee containing both the

+	 logfile and stdout.  */

+      ui_file *tee = nullptr;

+      if (!logging_redirect)

+	tee = new tee_file (gdb_stdout, std::move (logfile));

+

+      gdb_stdout = logging_redirect ? logfile_p : tee;

+      gdb_stdlog = logging_redirect ? logfile_p : tee;

+      gdb_stderr = logging_redirect ? logfile_p : tee;

+      gdb_stdtarg = logging_redirect ? logfile_p : tee;

+      gdb_stdtargerr = logging_redirect ? logfile_p : tee;

     }
   else
     {
@@ -434,11 +440,11 @@  cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect)

       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;

+      saved_output.out = nullptr;

+      saved_output.err = nullptr;

+      saved_output.log = nullptr;

+      saved_output.targ = nullptr;

+      saved_output.targerr = nullptr;

     }
 }
 
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h

index 77d73a23d04..0c2e73b6b38 100644

--- a/gdb/cli/cli-interp.h

+++ b/gdb/cli/cli-interp.h

@@ -33,19 +33,6 @@  public:

   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,
diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c

index 3a5e14de3c7..670e7e24908 100644

--- a/gdb/cli/cli-logging.c

+++ b/gdb/cli/cli-logging.c

@@ -88,24 +88,6 @@  pop_output_files (void)

     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)
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c

index 4568d398d94..6a19bf02476 100644

--- a/gdb/mi/mi-interp.c

+++ b/gdb/mi/mi-interp.c

@@ -1286,18 +1286,27 @@  mi_interp::set_logging (ui_file_up logfile, bool logging_redirect)

   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 something is being redirected, then grab logfile.  */

+      ui_file *logfile_p = nullptr;

+      if (logging_redirect)

+	logfile_p = logfile.release ();

+

+      /* If something is not being redirected, then a tee containing both the

+	 logfile and stdout.  */

+      ui_file *tee = nullptr;

+      if (!logging_redirect)

+	tee = new tee_file (mi->raw_stdout, std::move (logfile));

+

+      mi->raw_stdout = logging_redirect ? logfile_p : tee;

     }
   else
     {
       delete mi->raw_stdout;
       mi->raw_stdout = mi->saved_raw_stdout;
-      mi->saved_raw_stdout = NULL;

+      mi->saved_raw_stdout = nullptr;

     }
-  

+

   mi->out->set_raw (mi->raw_stdout);
   mi->err->set_raw (mi->raw_stdout);
   mi->log->set_raw (mi->raw_stdout);
diff --git a/gdb/testsuite/gdb.base/ui-redirect.exp b/gdb/testsuite/gdb.base/ui-redirect.exp

index 1ebff790e57..f1d00b939da 100644

--- a/gdb/testsuite/gdb.base/ui-redirect.exp

+++ b/gdb/testsuite/gdb.base/ui-redirect.exp

@@ -34,8 +34,28 @@  gdb_test_multiple $test $test {

 }
 gdb_test_no_output "end"
 
-gdb_test_no_output "set logging file /dev/null"

-gdb_test "set logging on" "Copying output to /dev/null\\."

-gdb_test "save breakpoints /dev/null" "Saved to file '/dev/null'\\."

-gdb_test "set logging off" "Done logging to /dev/null\\."

-gdb_test "help" "List of classes of commands:.*"

+with_test_prefix "logging" {

+    gdb_test_no_output "set logging file /dev/null"

+    gdb_test "set logging on" "Copying output to /dev/null\\."

+    gdb_test "save breakpoints /dev/null" "Saved to file '/dev/null'\\."

+    gdb_test "set logging off" "Done logging to /dev/null\\."

+    gdb_test "help" "List of classes of commands:.*"

+}

+

+with_test_prefix "redirect" {

+    gdb_test "set logging redirect on"

+    gdb_test "set logging on" "Redirecting output to /dev/null\\."

+    gdb_test_no_output "save breakpoints /dev/null"

+    gdb_test "set logging off" "Done logging to /dev/null\\."

+    gdb_test "help" "List of classes of commands:.*"

+}

+

+with_test_prefix "redirect while already logging" {

+    gdb_test_no_output "set logging redirect off"

+    gdb_test "set logging on" "Copying output to /dev/null\\."

+    gdb_test "set logging redirect on" \

+    ".*warning: Currently logging .*Turn the logging off and on to make the new setting effective.*"

+    gdb_test "save breakpoints /dev/null" "Saved to file '/dev/null'\\."

+    gdb_test "set logging off" "Done logging to /dev/null\\."

+    gdb_test "help" "List of classes of commands:.*"

+}

diff --git a/gdb/ui-file.c b/gdb/ui-file.c

index 4139b5deffc..24c914f442a 100644

--- a/gdb/ui-file.c

+++ b/gdb/ui-file.c

@@ -336,20 +336,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 56f0c0f957c..39f56d5ea42 100644

--- a/gdb/ui-file.h

+++ b/gdb/ui-file.h

@@ -267,11 +267,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;
@@ -284,10 +282,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