[4/7] gdb/ui-file: Add newline tracking

Message ID 20230227194212.348003-4-amerey@redhat.com
State New
Headers
Series [1/7] gdb/debuginfod: Add debuginfod_section_query |

Commit Message

Aaron Merey Feb. 27, 2023, 7:42 p.m. UTC
  Add a new field line_state to ui_file's that indicates whether
the character most recently written to the ui_file's stream is
a newline.

The purpose of this is to help debuginfod progress messages
always print on their own line.  Until now they have always
done so without any special intervention.  But with the addition
of deferred debuginfo downloading it is possible for progress
messages to occur during the printing of frame information.

Without any newline tracking in this case, we can end up with
poorly formatted output:

    (gdb) backtrace
    [...]
    #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (Downloading separate debug info for /lib64/libpython3.11.so.1.0
    function_cache=0x561221b224d0, state=<optimized out>...

With newline tracking, the progress message writer can detect
whether the message should start with a newline.  This ensures
that all progress messages print on their own line:

    (gdb) backtrace
    [...]
    #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (
    Downloading separate debug info for /lib64/libpython3.11.so.1.0
    function_cache=0x561221b224d0, state=<optimized out>...
---
 gdb/cli-out.c       | 18 +++++++++++++++---
 gdb/mi/mi-console.c |  4 ++++
 gdb/tui/tui-file.c  |  4 ++++
 gdb/ui-file.c       | 31 +++++++++++++++++++++++++++++++
 gdb/ui-file.h       | 44 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 98 insertions(+), 3 deletions(-)
  

Comments

Tom Tromey March 7, 2023, 7:33 p.m. UTC | #1
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Without any newline tracking in this case, we can end up with
Aaron> poorly formatted output:

Aaron>     (gdb) backtrace
Aaron>     [...]
Aaron>     #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (Downloading separate debug info for /lib64/libpython3.11.so.1.0
Aaron>     function_cache=0x561221b224d0, state=<optimized out>...

Aaron> With newline tracking, the progress message writer can detect
Aaron> whether the message should start with a newline.  This ensures
Aaron> that all progress messages print on their own line:

Aaron>     (gdb) backtrace
Aaron>     [...]
Aaron>     #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (
Aaron>     Downloading separate debug info for /lib64/libpython3.11.so.1.0
Aaron>     function_cache=0x561221b224d0, state=<optimized out>...

Still kind of badly formatted though?

A long time ago, IIRC, the backtrace code had a second loop to do some
unwinding precisely to cause debuginfo to be loaded.  I wonder if it
makes sense to resurrect this idea.  It's unclear if this would work
with frame filters though.

Tom
  
Aaron Merey March 7, 2023, 8:30 p.m. UTC | #2
On Tue, Mar 7, 2023 at 2:34 PM Tom Tromey <tom@tromey.com> wrote:
> Aaron>     (gdb) backtrace
> Aaron>     [...]
> Aaron>     #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (
> Aaron>     Downloading separate debug info for /lib64/libpython3.11.so.1.0
> Aaron>     function_cache=0x561221b224d0, state=<optimized out>...
>
> Still kind of badly formatted though?
>
> A long time ago, IIRC, the backtrace code had a second loop to do some
> unwinding precisely to cause debuginfo to be loaded.  I wonder if it
> makes sense to resurrect this idea.  It's unclear if this would work
> with frame filters though.

Can we accumulate the backtrace output in a buffer and print the whole
thing in one shot after all frames have been computed/filtered?  Then all
debuginfod messages would print together, followed by the uninterrupted
backtrace.

Aaron
  
Tom Tromey March 7, 2023, 8:47 p.m. UTC | #3
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> On Tue, Mar 7, 2023 at 2:34 PM Tom Tromey <tom@tromey.com> wrote:
Aaron> (gdb) backtrace
Aaron> [...]
Aaron> #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (
Aaron> Downloading separate debug info for /lib64/libpython3.11.so.1.0
Aaron> function_cache=0x561221b224d0, state=<optimized out>...

>> Still kind of badly formatted though?

>> A long time ago, IIRC, the backtrace code had a second loop to do some
>> unwinding precisely to cause debuginfo to be loaded.  I wonder if it
>> makes sense to resurrect this idea.  It's unclear if this would work
>> with frame filters though.

Aaron> Can we accumulate the backtrace output in a buffer and print the whole
Aaron> thing in one shot after all frames have been computed/filtered?  Then all
Aaron> debuginfod messages would print together, followed by the uninterrupted
Aaron> backtrace.

I like this idea.

To make it still seem "incremental", it seems like we'd only need to
print a single frame to a ui_file.  Then after that frame is done, the
buffer can be printed and gdb can move on to the next one.

Tom
  

Patch

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index fdfd0f7f0cf..1639f885cb9 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -307,6 +307,10 @@  cli_ui_out::do_progress_notify (const std::string &msg,
 
   if (info.state == progress_update::START)
     {
+      /* MSG should print on its own line.  */
+      if (stream->get_line_state () == ui_file::LINE_NOT_AT_START)
+	gdb_printf (stream, "\n");
+
       if (stream->isatty ()
 	  && current_ui->input_interactive_p ()
 	  && chars_per_line >= MIN_CHARS_PER_LINE)
@@ -410,10 +414,18 @@  void
 cli_ui_out::do_progress_end ()
 {
   struct ui_file *stream = m_streams.back ();
-  m_progress_info.pop_back ();
 
-  if (stream->isatty ())
-    clear_current_line ();
+  cli_progress_info &info (m_progress_info.back ());
+
+  if (info.state != progress_update::START && stream->isatty ())
+    {
+      clear_current_line ();
+
+      if (stream->get_line_state () != ui_file::LINE_UNDEFINED)
+	stream->set_line_state (ui_file::LINE_AT_START);
+    }
+
+  m_progress_info.pop_back ();
 }
 
 /* local functions */
diff --git a/gdb/mi/mi-console.c b/gdb/mi/mi-console.c
index e0b5648c2df..60f2a823b47 100644
--- a/gdb/mi/mi-console.c
+++ b/gdb/mi/mi-console.c
@@ -46,6 +46,8 @@  mi_console_file::write (const char *buf, long length_buf)
      buffer.  */
   if (strchr (m_buffer.c_str () + prev_size, '\n') != NULL)
     this->flush ();
+
+  set_line_state_from_buf (m_buffer.c_str () + prev_size, length_buf);
 }
 
 void
@@ -63,6 +65,8 @@  mi_console_file::write_async_safe (const char *buf, long length_buf)
 
   char nl = '\n';
   m_raw->write_async_safe (&nl, 1);
+
+  set_line_state (LINE_AT_START);
 }
 
 void
diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index c1619d5ace6..c36ce739497 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -28,6 +28,8 @@  tui_file::puts (const char *linebuffer)
   tui_puts (linebuffer);
   if (!m_buffered)
     tui_refresh_cmd_win ();
+
+  set_line_state_from_buf (linebuffer);
 }
 
 void
@@ -36,6 +38,8 @@  tui_file::write (const char *buf, long length_buf)
   tui_write (buf, length_buf);
   if (!m_buffered)
     tui_refresh_cmd_win ();
+
+  set_line_state_from_buf (buf, length_buf);
 }
 
 void
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index e0814fe2b2d..3f7a55ed252 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -31,6 +31,7 @@ 
 null_file null_stream;
 
 ui_file::ui_file ()
+  : m_line_state (LINE_UNDEFINED)
 {}
 
 ui_file::~ui_file ()
@@ -159,6 +160,30 @@  ui_file::printchar (int c, int quoter, bool async_safe)
     this->write (buf, out);
 }
 
+/* See ui-file.h.  */
+
+void
+ui_file::set_line_state_from_buf (const char *buf)
+{
+  const char *last_nl = strrchr (buf, '\n');
+
+  if (last_nl != nullptr && *++last_nl == '\0')
+    m_line_state = LINE_AT_START;
+  else
+    m_line_state = LINE_NOT_AT_START;
+}
+
+/* See ui-file.h.  */
+
+void
+ui_file::set_line_state_from_buf (const char *buf, long length_buf)
+{
+  if (buf[length_buf - 1] == '\n')
+    m_line_state = LINE_AT_START;
+  else
+    m_line_state = LINE_NOT_AT_START;
+}
+
 
 
 void
@@ -311,6 +336,8 @@  stdio_file::write (const char *buf, long length_buf)
     {
       /* Nothing.  */
     }
+
+  set_line_state_from_buf (buf, length_buf);
 }
 
 void
@@ -323,6 +350,8 @@  stdio_file::write_async_safe (const char *buf, long length_buf)
     {
       /* Nothing.  */
     }
+
+  set_line_state_from_buf (buf, length_buf);
 }
 
 void
@@ -339,6 +368,8 @@  stdio_file::puts (const char *linebuffer)
     {
       /* Nothing.  */
     }
+
+  set_line_state_from_buf (linebuffer);
 }
 
 bool
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index de24620e247..b85c37d81c7 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -132,11 +132,40 @@  class ui_file
     this->puts (str);
   }
 
+  enum line_state
+  {
+    /* Line state is not being tracked.  */
+    LINE_UNDEFINED,
+
+    /* The last character written to this ui_file is not a newline.  */
+    LINE_NOT_AT_START,
+
+    /* The last character written to this ui_file is a newline.  */
+    LINE_AT_START
+  };
+
+  /* Set this ui_file's line_state to STATE.  */
+  virtual void set_line_state (line_state state)
+  { m_line_state = state; }
+
+  /* Return this ui_file's line_state.  */
+  virtual line_state get_line_state ()
+  { return m_line_state; }
+
 protected:
 
   /* The currently applied style.  */
   ui_file_style m_applied_style;
 
+  /* The current line_state.  */
+  line_state m_line_state;
+
+  /* Set line_state based on whether BUF ends in a newline.  */
+  void set_line_state_from_buf (const char *buf);
+
+  /* Set line_state based on whether BUF ends in a newline.  */
+  void set_line_state_from_buf (const char *buf, long length_buf);
+
 private:
 
   /* Helper function for putstr and putstrn.  Print the character C on
@@ -428,6 +457,21 @@  class wrapped_file : public ui_file
   void write_async_safe (const char *buf, long length_buf) override
   { return m_stream->write_async_safe (buf, length_buf); }
 
+  /* Return the line_state of the underlying stream.  */
+  line_state get_line_state () override
+  {
+    if (m_stream == nullptr)
+      return ui_file::LINE_UNDEFINED;
+    return m_stream->get_line_state ();
+  }
+
+  /* Set the line_state of the underlying stream.  */
+  void set_line_state (line_state state) override
+  {
+    if (m_stream != nullptr)
+      m_stream->set_line_state (state);
+  }
+
 protected:
 
   /* Note that this class does not assume ownership of the stream.