[PATCH] gdb: make timestamped_file implement can_emit_style_escape

[PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Simon Marchi]

In our AMDGPU downstream port, we use styling in some logging output.
We noticed it stopped working after the gdb_printf changes.  Making
timestamped_file implement can_emit_style_escape (returning the value of
the stream it wraps) fixes it.  To show that it works, modify some
logging statements in auto-load.c to output style filenames.  You can
see it in action by setting "set debug auto-load 1" and running a
program.  We can incrementally add styling to other debug statements
throughout GDB, as needed.

Change-Id: I78a2fd1e078f80f2263251cf6bc53b3a9de9c17a
---
 gdb/auto-load.c | 9 +++++----
 gdb/ui-file.h   | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index 057104d97b08..b6056f5d60a9 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -733,8 +733,8 @@ auto_load_objfile_script_1 (struct objfile *objfile, const char *realname,
   gdb_file_up input = gdb_fopen_cloexec (filename.c_str (), "r");
   debugfile = filename.c_str ();
 
-  auto_load_debug_printf ("Attempted file \"%s\" %s.",
-			  debugfile,
+  auto_load_debug_printf ("Attempted file \"%ps\" %s.",
+			  styled_string (file_name_style.style (), debugfile),
 			  input != nullptr ? "exists" : "does not exist");
 
   std::string debugfile_holder;
@@ -763,8 +763,9 @@ auto_load_objfile_script_1 (struct objfile *objfile, const char *realname,
 
 	  input = gdb_fopen_cloexec (debugfile, "r");
 
-	  auto_load_debug_printf ("Attempted file \"%s\" %s.",
-				  debugfile,
+	  auto_load_debug_printf ("Attempted file \"%ps\" %s.",
+				  styled_string (file_name_style.style (),
+						 debugfile),
 				  (input != nullptr
 				   ? "exists"
 				   : "does not exist"));
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index f8e1fe8be3c2..e420555b8a6d 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -402,6 +402,9 @@ class timestamped_file : public ui_file
   {
   }
 
+  bool can_emit_style_escape () override
+  { return m_stream->can_emit_style_escape (); }
+
   DISABLE_COPY_AND_ASSIGN (timestamped_file);
 
   void write (const char *buf, long len) override;

base-commit: 0653f01479ecbcbf3c4dfa6083187a5b2c2258c2
-- 
2.35.1

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Tom Tromey]

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

Simon> In our AMDGPU downstream port, we use styling in some logging output.
Simon> We noticed it stopped working after the gdb_printf changes.

Sorry about that.

Simon> Making
Simon> timestamped_file implement can_emit_style_escape (returning the value of
Simon> the stream it wraps) fixes it.  To show that it works, modify some
Simon> logging statements in auto-load.c to output style filenames.  You can
Simon> see it in action by setting "set debug auto-load 1" and running a
Simon> program.  We can incrementally add styling to other debug statements
Simon> throughout GDB, as needed.

This looks good to me.

I've also wondered what it would take to style error() text.  Maybe just
checking the styling of gdb_stdout when creating the error would be good
enough, it's not like gdb often stores exceptions and then later reuses
them.

Another thought was to have styling always be emitted, then optionally
strip it out just before display.  However, user 'printf's can also emit
styles and one wouldn't want to strip those...

Tom

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Simon Marchi]

> Simon> Making
> Simon> timestamped_file implement can_emit_style_escape (returning the value of
> Simon> the stream it wraps) fixes it.  To show that it works, modify some
> Simon> logging statements in auto-load.c to output style filenames.  You can
> Simon> see it in action by setting "set debug auto-load 1" and running a
> Simon> program.  We can incrementally add styling to other debug statements
> Simon> throughout GDB, as needed.
> 
> This looks good to me.

Thanks, will push.

> I've also wondered what it would take to style error() text.  Maybe just
> checking the styling of gdb_stdout when creating the error would be good
> enough, it's not like gdb often stores exceptions and then later reuses
> them.

Indeed.

> Another thought was to have styling always be emitted, then optionally
> strip it out just before display.  However, user 'printf's can also emit
> styles and one wouldn't want to strip those...

Or the error string could be saved in an "unrendered" form that
includes the text and its styling.  And only render it (with or without
style) when we print it.  As an exercise, we can try to think how we
would do it if different uiouts in the same GDB had different methods
of styling, and the error had to be printed on all those uiouts.  We
couldn't bake a styled string when emitting the error, we would have to
render it once for each uiout.

Simon

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Simon Marchi]

On 2022-04-04 17:58, Simon Marchi via Gdb-patches wrote:
>> Simon> Making
>> Simon> timestamped_file implement can_emit_style_escape (returning the value of
>> Simon> the stream it wraps) fixes it.  To show that it works, modify some
>> Simon> logging statements in auto-load.c to output style filenames.  You can
>> Simon> see it in action by setting "set debug auto-load 1" and running a
>> Simon> program.  We can incrementally add styling to other debug statements
>> Simon> throughout GDB, as needed.
>>
>> This looks good to me.
> 
> Thanks, will push.
> 
>> I've also wondered what it would take to style error() text.  Maybe just
>> checking the styling of gdb_stdout when creating the error would be good
>> enough, it's not like gdb often stores exceptions and then later reuses
>> them.
> 
> Indeed.
> 
>> Another thought was to have styling always be emitted, then optionally
>> strip it out just before display.  However, user 'printf's can also emit
>> styles and one wouldn't want to strip those...
> 
> Or the error string could be saved in an "unrendered" form that
> includes the text and its styling.  And only render it (with or without
> style) when we print it.  As an exercise, we can try to think how we
> would do it if different uiouts in the same GDB had different methods
> of styling, and the error had to be printed on all those uiouts.  We
> couldn't bake a styled string when emitting the error, we would have to
> render it once for each uiout.
> 
> Simon

Also, the person who reported this to me suggested that timestamped_file
should probably implement more methods to defer to m_stream.  If we look
at tee_file, it implements a bunch:

  bool isatty () override;
  bool term_out () override;
  bool can_emit_style_escape () override;
  void flush () override;

I'm thinking that isatty and flush should be implemented, since their
default behavior from ui_file is probably not adequate?

Simon

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Tom Tromey]

Simon> I'm thinking that isatty and flush should be implemented, since their
Simon> default behavior from ui_file is probably not adequate?

How about the appended?

Tom

commit b490690480b50918b1b4f5533e6391e9fb1e8072
Author: Tom Tromey <tom@tromey.com>
Date:   Tue Apr 5 07:44:59 2022 -0600

    Introduce wrapped_file
    
    Simon pointed out that timestamped_file probably needed to implement a
    few more methods.  This patch introduces a new file-wrapping file that
    forwards most of its calls, making it simpler to implement new such
    files.  It also converts timestamped_file and pager_file to use it.
    
    Regression tested on x86-64 Fedora 34.

diff --git a/gdb/pager.h b/gdb/pager.h
index 0151a283629..d2a3a2b8ec8 100644
--- a/gdb/pager.h
+++ b/gdb/pager.h
@@ -23,16 +23,21 @@
 
 /* A ui_file that implements output paging and unfiltered output.  */
 
-class pager_file : public ui_file
+class pager_file : public wrapped_file
 {
 public:
   /* Create a new pager_file.  The new object takes ownership of
      STREAM.  */
   explicit pager_file (ui_file *stream)
-    : m_stream (stream)
+    : wrapped_file (stream)
   {
   }
 
+  ~pager_file ()
+  {
+    delete m_stream;
+  }
+
   DISABLE_COPY_AND_ASSIGN (pager_file);
 
   void write (const char *buf, long length_buf) override;
@@ -44,31 +49,11 @@ class pager_file : public ui_file
     m_stream->write_async_safe (buf, length_buf);
   }
 
-  bool term_out () override
-  {
-    return m_stream->term_out ();
-  }
-
-  bool isatty () override
-  {
-    return m_stream->isatty ();
-  }
-
-  bool can_emit_style_escape () override
-  {
-    return m_stream->can_emit_style_escape ();
-  }
-
   void emit_style_escape (const ui_file_style &style) override;
   void reset_style () override;
 
   void flush () override;
 
-  int fd () const override
-  {
-    return m_stream->fd ();
-  }
-
   void wrap_here (int indent) override;
 
   void puts_unfiltered (const char *str) override
@@ -98,9 +83,6 @@ class pager_file : public ui_file
   /* The style applied at the time that wrap_here was called.  */
   ui_file_style m_wrap_style;
 
-  /* The unfiltered output stream.  */
-  ui_file_up m_stream;
-
   /* This is temporarily set when paging.  This will cause some
      methods to change their behavior to ignore the wrap buffer.  */
   bool m_paging = false;
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index bffdeaa28c4..3af769e7ae6 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -391,32 +391,92 @@ class no_terminal_escape_file : public stdio_file
   }
 };
 
-/* A ui_file that optionally puts a timestamp at the start of each
-   line of output.  */
+/* A base class for ui_file types that that wrap another ui_file.  */
 
-class timestamped_file : public ui_file
+class wrapped_file :  public ui_file
 {
 public:
-  explicit timestamped_file (ui_file *stream)
-    : m_stream (stream)
+
+  bool isatty () override
+  {
+    return m_stream->isatty ();
+  }
+
+  bool term_out () override
   {
+    return m_stream->term_out ();
   }
 
   bool can_emit_style_escape () override
-  { return m_stream->can_emit_style_escape (); }
+  {
+    return m_stream->can_emit_style_escape ();
+  }
+
+  void flush () override
+  {
+    m_stream->flush ();
+  }
+
+  void wrap_here (int indent) override
+  {
+    m_stream->wrap_here (indent);
+  }
+
+  void emit_style_escape (const ui_file_style &style) override
+  {
+    m_stream->emit_style_escape (style);
+  }
+
+  /* Rest the current output style to the empty style.  */
+  void reset_style () override
+  {
+    m_stream->reset_style ();
+  }
+
+  int fd () const override
+  {
+    return m_stream->fd ();
+  }
+
+  void puts_unfiltered (const char *str) override
+  {
+    m_stream->puts_unfiltered (str);
+  }
 
   void write_async_safe (const char *buf, long length_buf) override
   { return m_stream->write_async_safe (buf, length_buf); }
 
+protected:
+
+  /* Note that this class does not assume ownership of the stream.
+     However, a subclass may choose to, by adding a 'delete' to its
+     destructor.  */
+  explicit wrapped_file (ui_file *stream)
+    : m_stream (stream)
+  {
+  }
+
+  /* The underlying stream.  */
+  ui_file *m_stream;
+};
+
+/* A ui_file that optionally puts a timestamp at the start of each
+   line of output.  */
+
+class timestamped_file : public wrapped_file
+{
+public:
+  explicit timestamped_file (ui_file *stream)
+    : wrapped_file (stream)
+  {
+  }
+
   DISABLE_COPY_AND_ASSIGN (timestamped_file);
 
   void write (const char *buf, long len) override;
 
 private:
 
-  /* Output is sent here.  */
-  ui_file *m_stream;
-
   /* True if the next output should be timestamped.  */
   bool m_needs_timestamp = true;
 };

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Simon Marchi]

On 2022-04-05 10:25, Tom Tromey wrote:
> Simon> I'm thinking that isatty and flush should be implemented, since their
> Simon> default behavior from ui_file is probably not adequate?
>
> How about the appended?
>
> Tom
>
> commit b490690480b50918b1b4f5533e6391e9fb1e8072
> Author: Tom Tromey <tom@tromey.com>
> Date:   Tue Apr 5 07:44:59 2022 -0600
>
>     Introduce wrapped_file
>
>     Simon pointed out that timestamped_file probably needed to implement a
>     few more methods.  This patch introduces a new file-wrapping file that
>     forwards most of its calls, making it simpler to implement new such
>     files.  It also converts timestamped_file and pager_file to use it.

Nice, I thought of doing something like this but didn't have time to.

One formatting nit below.

>
>     Regression tested on x86-64 Fedora 34.
>
> diff --git a/gdb/pager.h b/gdb/pager.h
> index 0151a283629..d2a3a2b8ec8 100644
> --- a/gdb/pager.h
> +++ b/gdb/pager.h
> @@ -23,16 +23,21 @@
>
>  /* A ui_file that implements output paging and unfiltered output.  */
>
> -class pager_file : public ui_file
> +class pager_file : public wrapped_file
>  {
>  public:
>    /* Create a new pager_file.  The new object takes ownership of
>       STREAM.  */
>    explicit pager_file (ui_file *stream)
> -    : m_stream (stream)
> +    : wrapped_file (stream)
>    {
>    }
>
> +  ~pager_file ()
> +  {
> +    delete m_stream;
> +  }
> +
>    DISABLE_COPY_AND_ASSIGN (pager_file);
>
>    void write (const char *buf, long length_buf) override;
> @@ -44,31 +49,11 @@ class pager_file : public ui_file
>      m_stream->write_async_safe (buf, length_buf);
>    }
>
> -  bool term_out () override
> -  {
> -    return m_stream->term_out ();
> -  }
> -
> -  bool isatty () override
> -  {
> -    return m_stream->isatty ();
> -  }
> -
> -  bool can_emit_style_escape () override
> -  {
> -    return m_stream->can_emit_style_escape ();
> -  }
> -
>    void emit_style_escape (const ui_file_style &style) override;
>    void reset_style () override;
>
>    void flush () override;
>
> -  int fd () const override
> -  {
> -    return m_stream->fd ();
> -  }
> -
>    void wrap_here (int indent) override;
>
>    void puts_unfiltered (const char *str) override
> @@ -98,9 +83,6 @@ class pager_file : public ui_file
>    /* The style applied at the time that wrap_here was called.  */
>    ui_file_style m_wrap_style;
>
> -  /* The unfiltered output stream.  */
> -  ui_file_up m_stream;
> -
>    /* This is temporarily set when paging.  This will cause some
>       methods to change their behavior to ignore the wrap buffer.  */
>    bool m_paging = false;
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index bffdeaa28c4..3af769e7ae6 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -391,32 +391,92 @@ class no_terminal_escape_file : public stdio_file
>    }
>  };
>
> -/* A ui_file that optionally puts a timestamp at the start of each
> -   line of output.  */
> +/* A base class for ui_file types that that wrap another ui_file.  */
>
> -class timestamped_file : public ui_file
> +class wrapped_file :  public ui_file
>  {
>  public:
> -  explicit timestamped_file (ui_file *stream)
> -    : m_stream (stream)
> +
> +  bool isatty () override
> +  {
> +    return m_stream->isatty ();
> +  }
> +
> +  bool term_out () override
>    {
> +    return m_stream->term_out ();
>    }
>
>    bool can_emit_style_escape () override
> -  { return m_stream->can_emit_style_escape (); }
> +  {
> +    return m_stream->can_emit_style_escape ();
> +  }
> +
> +  void flush () override
> +  {
> +    m_stream->flush ();
> +  }
> +
> +  void wrap_here (int indent) override
> +  {
> +    m_stream->wrap_here (indent);
> +  }
> +
> +  void emit_style_escape (const ui_file_style &style) override
> +  {
> +    m_stream->emit_style_escape (style);
> +  }
> +
> +  /* Rest the current output style to the empty style.  */
> +  void reset_style () override
> +  {
> +    m_stream->reset_style ();
> +  }
> +
> +  int fd () const override
> +  {
> +    return m_stream->fd ();
> +  }
> +
> +  void puts_unfiltered (const char *str) override
> +  {
> +    m_stream->puts_unfiltered (str);
> +  }
>
>    void write_async_safe (const char *buf, long length_buf) override
>    { return m_stream->write_async_safe (buf, length_buf); }

The formatting (braces placement) of write_async_safe clashes with the
rest.  Can you make it uniform?  I do like the short version with the
braces on the same line, for simple one-liners, but I don't really mind,
as long as it's consistent.

Simon

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Pedro Alves]

On 2022-04-05 15:25, Tom Tromey wrote:

> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -391,32 +391,92 @@ class no_terminal_escape_file : public stdio_file
>    }
>  };
>  
> -/* A ui_file that optionally puts a timestamp at the start of each
> -   line of output.  */
> +/* A base class for ui_file types that that wrap another ui_file.  */


"that that" -> "that"

>  
> -class timestamped_file : public ui_file
> +class wrapped_file :  public ui_file

Spurious double space before "public".

>  {
>  public:
> -  explicit timestamped_file (ui_file *stream)

(Otherwise LGTM.)

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Tom Tromey]

Simon> The formatting (braces placement) of write_async_safe clashes with the
Simon> rest.  Can you make it uniform?  I do like the short version with the
Simon> braces on the same line, for simple one-liners, but I don't really mind,
Simon> as long as it's consistent.

I went with the one-line approach.

Tom

Re: [PATCH] gdb: make timestamped_file implement can_emit_style_escape

[Tom Tromey]

Pedro> "that that" -> "that"

Fixed.

>> -class timestamped_file : public ui_file
>> +class wrapped_file :  public ui_file

Pedro> Spurious double space before "public".

Fixed.

I'm checking in the updated patch.

Tom