[PATCH 0/3] Do not let logging use pager

[PATCH 0/3] Do not let logging use pager

[Tom Tromey]

PR gdb/29787 points out a crash in gdb when logging is used.  I
tracked this down to the CLI accidentally enabling paging for
gdb_stdlog when logging is enabled.

This series applies a couple of minor cleanups, then fixes the
underlying problem.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 1/3] Remove 'saved_output' global

[Tom Tromey]

CLI redirect uses a global variable, 'saved_output'.  However, globals
are generally bad, and there is no need for this one -- it can be a
member of cli_interp_base.  This patch makes this change.
---
 gdb/cli/cli-interp.c | 47 ++++++++++++++++----------------------------
 gdb/cli/cli-interp.h | 16 +++++++++++++++
 2 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index ca3a1abcaae..8c2fb207486 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -370,20 +370,6 @@ cli_interp::interp_ui_out ()
   return m_cli_uiout.get ();
 }
 
-/* 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;
-  ui_file_up file_to_delete;
-  ui_file_up log_to_delete;
-};
-static std::unique_ptr<saved_output_files> saved_output;
-
 /* See cli-interp.h.  */
 
 void
@@ -392,12 +378,13 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
 {
   if (logfile != nullptr)
     {
-      saved_output.reset (new saved_output_files);
-      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;
+      gdb_assert (m_saved_output == nullptr);
+      m_saved_output.reset (new saved_output_files);
+      m_saved_output->out = gdb_stdout;
+      m_saved_output->err = gdb_stderr;
+      m_saved_output->log = gdb_stdlog;
+      m_saved_output->targ = gdb_stdtarg;
+      m_saved_output->targerr = gdb_stdtargerr;
 
       /* If something is not being redirected, then a tee containing both the
 	 logfile and stdout.  */
@@ -406,29 +393,29 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
       if (!logging_redirect || !debug_redirect)
 	{
 	  tee = new tee_file (gdb_stdout, std::move (logfile));
-	  saved_output->file_to_delete.reset (tee);
+	  m_saved_output->file_to_delete.reset (tee);
 	}
       else
-	saved_output->file_to_delete = std::move (logfile);
+	m_saved_output->file_to_delete = std::move (logfile);
 
-      saved_output->log_to_delete.reset
+      m_saved_output->log_to_delete.reset
 	(new timestamped_file (debug_redirect ? logfile_p : tee));
 
       gdb_stdout = logging_redirect ? logfile_p : tee;
-      gdb_stdlog = saved_output->log_to_delete.get ();
+      gdb_stdlog = m_saved_output->log_to_delete.get ();
       gdb_stderr = logging_redirect ? logfile_p : tee;
       gdb_stdtarg = logging_redirect ? logfile_p : tee;
       gdb_stdtargerr = logging_redirect ? logfile_p : tee;
     }
   else
     {
-      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;
+      gdb_stdout = m_saved_output->out;
+      gdb_stderr = m_saved_output->err;
+      gdb_stdlog = m_saved_output->log;
+      gdb_stdtarg = m_saved_output->targ;
+      gdb_stdtargerr = m_saved_output->targerr;
 
-      saved_output.reset (nullptr);
+      m_saved_output.reset (nullptr);
     }
 }
 
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index 1620305c26b..3c233c0a229 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -32,6 +32,22 @@ class cli_interp_base : public interp
 		    bool debug_redirect) override;
   void pre_command_loop () override;
   bool supports_command_editing () override;
+
+private:
+  struct saved_output_files
+  {
+    ui_file *out;
+    ui_file *err;
+    ui_file *log;
+    ui_file *targ;
+    ui_file *targerr;
+    ui_file_up file_to_delete;
+    ui_file_up log_to_delete;
+  };
+
+  /* These hold the pushed copies of the gdb output files.  If NULL
+     then nothing has yet been pushed.  */
+  std::unique_ptr<saved_output_files> m_saved_output;
 };
 
 /* Returns true if the current stop should be printed to
-- 
2.34.3

[PATCH 2/3] Don't let tee_file own a stream

[Tom Tromey]

Right now, tee_file owns the second stream it writes to.  This is done
for the convenience of the users.  In a subsequent patch, this will no
longer be convenient, so this patch moves the responsibility for
ownership to the users of tee_file.
---
 gdb/cli/cli-interp.c | 11 ++++++-----
 gdb/cli/cli-interp.h |  1 +
 gdb/mi/mi-interp.c   | 15 ++++++---------
 gdb/mi/mi-interp.h   |  2 +-
 gdb/ui-file.c        |  4 ++--
 gdb/ui-file.h        |  8 ++++----
 6 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 8c2fb207486..2e8f6135405 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -386,17 +386,18 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
       m_saved_output->targ = gdb_stdtarg;
       m_saved_output->targerr = gdb_stdtargerr;
 
+      ui_file *logfile_p = logfile.get ();
+      m_saved_output->file_to_delete = std::move (logfile);
+
       /* If something is not being redirected, then a tee containing both the
 	 logfile and stdout.  */
-      ui_file *logfile_p = logfile.get ();
       ui_file *tee = nullptr;
       if (!logging_redirect || !debug_redirect)
 	{
-	  tee = new tee_file (gdb_stdout, std::move (logfile));
-	  m_saved_output->file_to_delete.reset (tee);
+	  m_saved_output->tee_to_delete.reset
+	    (new tee_file (gdb_stdout, logfile_p));
+	  tee = m_saved_output->tee_to_delete.get ();
 	}
-      else
-	m_saved_output->file_to_delete = std::move (logfile);
 
       m_saved_output->log_to_delete.reset
 	(new timestamped_file (debug_redirect ? logfile_p : tee));
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index 3c233c0a229..fa007d78621 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -41,6 +41,7 @@ class cli_interp_base : public interp
     ui_file *log;
     ui_file *targ;
     ui_file *targerr;
+    ui_file_up tee_to_delete;
     ui_file_up file_to_delete;
     ui_file_up log_to_delete;
   };
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index af242208a0b..482166ea3a5 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -1275,21 +1275,16 @@ mi_interp::set_logging (ui_file_up logfile, bool logging_redirect,
     {
       mi->saved_raw_stdout = mi->raw_stdout;
 
-      /* If something is being redirected, then grab logfile.  */
-      ui_file *logfile_p = nullptr;
-      if (logging_redirect || debug_redirect)
-	{
-	  logfile_p = logfile.get ();
-	  mi->saved_raw_file_to_delete = logfile_p;
-	}
+      ui_file *logfile_p = logfile.get ();
+      mi->saved_raw_file_to_delete = logfile.release ();
 
       /* If something is not being redirected, then a tee containing both the
 	 logfile and stdout.  */
       ui_file *tee = nullptr;
       if (!logging_redirect || !debug_redirect)
 	{
-	  tee = new tee_file (mi->raw_stdout, std::move (logfile));
-	  mi->saved_raw_file_to_delete = tee;
+	  tee = new tee_file (mi->raw_stdout, logfile_p);
+	  mi->saved_tee_to_delete = tee;
 	}
 
       mi->raw_stdout = logging_redirect ? logfile_p : tee;
@@ -1297,9 +1292,11 @@ mi_interp::set_logging (ui_file_up logfile, bool logging_redirect,
   else
     {
       delete mi->saved_raw_file_to_delete;
+      delete mi->saved_tee_to_delete;
       mi->raw_stdout = mi->saved_raw_stdout;
       mi->saved_raw_stdout = nullptr;
       mi->saved_raw_file_to_delete = nullptr;
+      mi->saved_tee_to_delete = nullptr;
     }
 
   mi->out->set_raw (mi->raw_stdout);
diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h
index d89439f54c5..d118ffb41e5 100644
--- a/gdb/mi/mi-interp.h
+++ b/gdb/mi/mi-interp.h
@@ -57,7 +57,7 @@ class mi_interp final : public interp
      done.  */
   struct ui_file *saved_raw_stdout;
   struct ui_file *saved_raw_file_to_delete;
-
+  struct ui_file *saved_tee_to_delete;
 
   /* MI's builder.  */
   struct ui_out *mi_uiout;
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 47044e42a67..3343b6b8fc5 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -384,9 +384,9 @@ stderr_file::stderr_file (FILE *stream)
 
 \f
 
-tee_file::tee_file (ui_file *one, ui_file_up &&two)
+tee_file::tee_file (ui_file *one, ui_file *two)
   : m_one (one),
-    m_two (std::move (two))
+    m_two (two)
 {}
 
 tee_file::~tee_file ()
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index e33ae79e753..4a006bbba4c 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -329,9 +329,9 @@ class stderr_file : public stdio_file
 class tee_file : public ui_file
 {
 public:
-  /* 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);
+  /* Create a file which writes to both ONE and TWO.  Ownership of
+     both files is up to the user.  */
+  tee_file (ui_file *one, ui_file *two);
   ~tee_file () override;
 
   void write (const char *buf, long length_buf) override;
@@ -364,7 +364,7 @@ class tee_file : public ui_file
 private:
   /* The two underlying ui_files.  */
   ui_file *m_one;
-  ui_file_up m_two;
+  ui_file *m_two;
 };
 
 /* A ui_file implementation that filters out terminal escape
-- 
2.34.3

Re: [PATCH 2/3] Don't let tee_file own a stream

[Simon Marchi]

On 11/17/22 13:29, Tom Tromey via Gdb-patches wrote:
> Right now, tee_file owns the second stream it writes to.  This is done
> for the convenience of the users.  In a subsequent patch, this will no
> longer be convenient, so this patch moves the responsibility for
> ownership to the users of tee_file.

I think the idea makes sense, from a software design / decoupling point
of view.

> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index af242208a0b..482166ea3a5 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -1275,21 +1275,16 @@ mi_interp::set_logging (ui_file_up logfile, bool logging_redirect,
>      {
>        mi->saved_raw_stdout = mi->raw_stdout;
>  
> -      /* If something is being redirected, then grab logfile.  */
> -      ui_file *logfile_p = nullptr;
> -      if (logging_redirect || debug_redirect)
> -	{
> -	  logfile_p = logfile.get ();
> -	  mi->saved_raw_file_to_delete = logfile_p;
> -	}
> +      ui_file *logfile_p = logfile.get ();
> +      mi->saved_raw_file_to_delete = logfile.release ();
>  
>        /* If something is not being redirected, then a tee containing both the
>  	 logfile and stdout.  */
>        ui_file *tee = nullptr;
>        if (!logging_redirect || !debug_redirect)
>  	{
> -	  tee = new tee_file (mi->raw_stdout, std::move (logfile));
> -	  mi->saved_raw_file_to_delete = tee;
> +	  tee = new tee_file (mi->raw_stdout, logfile_p);
> +	  mi->saved_tee_to_delete = tee;
>  	}
>  
>        mi->raw_stdout = logging_redirect ? logfile_p : tee;
> @@ -1297,9 +1292,11 @@ mi_interp::set_logging (ui_file_up logfile, bool logging_redirect,
>    else
>      {
>        delete mi->saved_raw_file_to_delete;
> +      delete mi->saved_tee_to_delete;
>        mi->raw_stdout = mi->saved_raw_stdout;
>        mi->saved_raw_stdout = nullptr;
>        mi->saved_raw_file_to_delete = nullptr;
> +      mi->saved_tee_to_delete = nullptr;

Should the two _to_delete ones be ui_file_ups?

Otherwise, everything related to logging / redirection confuse me, so I
can't really comment on the change itself.

Simon

Re: [PATCH 2/3] Don't let tee_file own a stream

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

>> +      mi->saved_tee_to_delete = nullptr;

Simon> Should the two _to_delete ones be ui_file_ups?

It would be better.  I'll add on a patch to do this.

Simon> Otherwise, everything related to logging / redirection confuse me, so I
Simon> can't really comment on the change itself.

It's a real mess.

Tom

[PATCH 3/3] Don't let gdb_stdlog use pager

[Tom Tromey]

When using the "set logging" commands, cli_interp_base::set_logging
will send gdb_stdlog output (among others) to the tee it makes for
gdb_stdout.  However, this has the side effect of also causing logging
to use the pager.  This is PR gdb/29787.

This patch fixes the problem by keeping stderr and stdlog separate
from stdout, preserving the rule that only gdb_stdout should page.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29787
---
 gdb/cli/cli-interp.c | 24 ++++++++++++++----------
 gdb/cli/cli-interp.h |  1 +
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 2e8f6135405..3254efc3581 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -389,24 +389,28 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
       ui_file *logfile_p = logfile.get ();
       m_saved_output->file_to_delete = std::move (logfile);
 
-      /* If something is not being redirected, then a tee containing both the
-	 logfile and stdout.  */
-      ui_file *tee = nullptr;
-      if (!logging_redirect || !debug_redirect)
+      /* The new stdout and stderr only depend on whether logging
+	 redirection is being done.  */
+      ui_file *new_stdout = logfile_p;
+      ui_file *new_stderr = logfile_p;
+      if (!logging_redirect)
 	{
 	  m_saved_output->tee_to_delete.reset
 	    (new tee_file (gdb_stdout, logfile_p));
-	  tee = m_saved_output->tee_to_delete.get ();
+	  new_stdout = m_saved_output->tee_to_delete.get ();
+	  m_saved_output->stderr_to_delete.reset
+	    (new tee_file (gdb_stderr, logfile_p));
+	  new_stderr = m_saved_output->stderr_to_delete.get ();
 	}
 
       m_saved_output->log_to_delete.reset
-	(new timestamped_file (debug_redirect ? logfile_p : tee));
+	(new timestamped_file (debug_redirect ? logfile_p : new_stderr));
 
-      gdb_stdout = logging_redirect ? logfile_p : tee;
+      gdb_stdout = new_stdout;
       gdb_stdlog = m_saved_output->log_to_delete.get ();
-      gdb_stderr = logging_redirect ? logfile_p : tee;
-      gdb_stdtarg = logging_redirect ? logfile_p : tee;
-      gdb_stdtargerr = logging_redirect ? logfile_p : tee;
+      gdb_stderr = new_stderr;
+      gdb_stdtarg = new_stderr;
+      gdb_stdtargerr = new_stderr;
     }
   else
     {
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index fa007d78621..978e7f291e4 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -42,6 +42,7 @@ class cli_interp_base : public interp
     ui_file *targ;
     ui_file *targerr;
     ui_file_up tee_to_delete;
+    ui_file_up stderr_to_delete;
     ui_file_up file_to_delete;
     ui_file_up log_to_delete;
   };
-- 
2.34.3

Re: [PATCH 3/3] Don't let gdb_stdlog use pager

[Simon Marchi]

> diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
> index fa007d78621..978e7f291e4 100644
> --- a/gdb/cli/cli-interp.h
> +++ b/gdb/cli/cli-interp.h
> @@ -42,6 +42,7 @@ class cli_interp_base : public interp
>      ui_file *targ;
>      ui_file *targerr;
>      ui_file_up tee_to_delete;
> +    ui_file_up stderr_to_delete;
>      ui_file_up file_to_delete;
>      ui_file_up log_to_delete;

I think it would help me understand if the naming of these fields was a
bit better.  I think the to_delete suffixes are no longer relevant
since using ui_file_up, the types tell us about the ownership.  And I
think that "tee_to_delete" could be named "stdout_tee", and
"stderr_to_delete" could be named "stderr_tee".

Not terribly important, perhaps as a follow up.

Simon

Re: [PATCH 3/3] Don't let gdb_stdlog use pager

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

>> diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
>> index fa007d78621..978e7f291e4 100644
>> --- a/gdb/cli/cli-interp.h
>> +++ b/gdb/cli/cli-interp.h
>> @@ -42,6 +42,7 @@ class cli_interp_base : public interp
>> ui_file *targ;
>> ui_file *targerr;
>> ui_file_up tee_to_delete;
>> +    ui_file_up stderr_to_delete;
>> ui_file_up file_to_delete;
>> ui_file_up log_to_delete;

Simon> I think it would help me understand if the naming of these fields was a
Simon> bit better.  I think the to_delete suffixes are no longer relevant
Simon> since using ui_file_up, the types tell us about the ownership.  And I
Simon> think that "tee_to_delete" could be named "stdout_tee", and
Simon> "stderr_to_delete" could be named "stderr_tee".

Simon> Not terribly important, perhaps as a follow up.

I'll send a follow-up for this as well.

Tom