gdb: styling fixes around and for the pagination prompt

This commit fixes a couple of issues relating to the pagination
prompt and styling.  The pagination prompt is this one:

  --Type <RET> for more, q to quit, c to continue without paging--

I did try to split this into multiple patches, based on the three
issues I describe below, but in the end, the fixes were all too
interconnected, so it ended up as one patch that makes two related,
but slightly different changes:

  1. Within the pager_file class, relying on the m_applied_style
  attribute of the wrapped m_stream, as is done when calling
  m_stream->emit_style_escape, is not correct, so stop doing that, and

  2. Failing to update m_applied_style within the pager_file class can
  leave that attribute out of date, which can then lead to styling
  errors later on, so ensure m_applied_style is always updated.

The problems I have seen are:

  1. After quitting from a pagination prompt, the next command can
  incorrectly style its output.  This was reported as bug PR
  gdb/31033, and is fixed by this commit.

  2. The pagination prompt itself could be styled.  The pagination
  prompt should always be shown in the default style.

  3. After continuing the output at a pagination prompt, GDB can fail
  to restore the default style the next time the output (within the
  same command) switches back to the default style.

There are tests for all these issues as part of this patch.

The pager_file class is a sub-class of wrapped_file, this means that a
pager_file is itself a ui_file, while it also manages a pointer to a
ui_file object (called m_stream).  An instance of pager_file can be
installed as the gdb_stdout ui_file object.

Output sent to a pager_file is stored within an internal
buffer (called m_wrap_buffer) until we have a complete line, when the
content is flushed to the wrapped m_stream.  If sufficient lines have
been written out then the pager_file will present the pagination
prompt and allow the user to continue viewing output, or quit the
current command.

As a pager_file is a ui_file, it has an m_applied_style member
variable.

The managed stream (m_stream) is also a ui_file, and so also has an
m_applied_style member variable.

In some places within the pager_file class we attempt to change the
current style of the m_stream using calls like this:

  m_stream->emit_style_escape (style);

See pager_file::emit_style_escape, pager_file::prompt_for_continue,
and pager_file::puts.  These calls will end up in
ui_file::emit_style_escape, which tries to skip emitting unnecessary
style escapes by checking if the requested style matches the current
m_applied_style value.

The m_applied_style value is updated by calls to the emit_style_escape
function.

The problem here is that most of the time pager_file doesn't change
the style of m_stream by calling m_stream->emit_style_escape.  Most of
the time, style changes are performed by pager_file writing the escape
sequence into m_wrap_buffer, and then later flushing this buffer to
m_stream by calling m_stream->puts.

It has to be done this way.  Calling m_stream->emit_style_escape
would, if it actually changed the style, immediately change the style
by emitting an escape sequence.  But pager_file doesn't want that, it
wants the style change to happen later, when m_wrap_buffer is
flushed.

To avoid excessive style escape sequences being written into
m_wrap_buffer, the pager_file::m_applied_style performs a function
similar to the m_applied_style within m_stream, it tracks the current
style for the end of m_wrap_buffer, and only allows style escape
sequences to be emitted if the style is actually changing.

However, a consequence of this is the m_applied_style within m_stream,
is not updated, which means it will be out of sync with the actual
current style of m_stream.  If we then try to make a call to
m_stream->emit_style_escape, if the style we are changing too happens
to match the out of date style in m_stream->m_applied_style, then the
style change will be ignored.

And this is indeed what we see in pager_file::prompt_for_continue with
the call:

  m_stream->emit_style_escape (ui_file_style ());

As m_stream->m_applied_style is not being updated, it will always be
the default style, however m_stream itself might not actually be in
the default style.  This call then will not emit an escape sequence as
the desired style matches the out of date m_applied_style.

The fix in this case is to call m_stream->puts directly, passing in
the escape sequence for the desired style.  This will result in an
immediate change of style for m_stream, which fixes some of the
problems described above.

In fact, given that m_stream's m_applied_style is always going to be
out of sync, I think we should change all of the
m_stream->emit_style_escape calls to instead call m_stream->puts.

However, just changing to use puts doesn't fix all the problems.

I found that, if I run 'apropos time', then quit at the first
pagination prompt.  If for the next command I run 'maintenance time' I
see the expected output:

  "maintenance time" takes a numeric argument.

However, everything after the first double quote is given the command
name style rather than only styling the text between the double
quotes.

Here is GDB's stack while printing the above output:

  #2  0x0000000001050d56 in ui_out::vmessage (this=0x7fff1238a150, in_style=..., format=0x1c05af0 "", args=0x7fff1238a288) at ../../src/gdb/ui-out.c:754
  #3  0x000000000104db88 in ui_file::vprintf (this=0x3f9edb0, format=0x1c05ad0 "\"%ps\" takes a numeric argument.\n", args=0x7fff1238a288) at ../../src/gdb/ui-file.c:73
  #4  0x00000000010bc754 in gdb_vprintf (stream=0x3f9edb0, format=0x1c05ad0 "\"%ps\" takes a numeric argument.\n", args=0x7fff1238a288) at ../../src/gdb/utils.c:1905
  #5  0x00000000010bca20 in gdb_printf (format=0x1c05ad0 "\"%ps\" takes a numeric argument.\n") at ../../src/gdb/utils.c:1945
  #6  0x0000000000b6b29e in maintenance_time_display (args=0x0, from_tty=1) at ../../src/gdb/maint.c:128

The interesting frames here are #3, in here `this` is the pager_file
for GDB's stdout, and this passes its m_applied_style to frame #2 as
the `in_style` argument.

If the m_applied_style is wrong, then frame #2 will believe that the
wrong style is currently in use as the default style, and so, after
printing 'maintenance time' GDB will switch back to the wrong style.

So the question is, why is pager_file::m_applied_style wrong?

In pager_file::prompt_for_continue, there is an attempt to switch back
to the default style using:

  m_stream->emit_style_escape (ui_file_style ());

If this is changed to a puts call (see above) then this still leaves
pager_file::m_applied_style out of date.

The right fix in this case is, I think, to instead do this:

  this->emit_style_escape (ui_file_style ());

this will update pager_file::m_applied_style, and also send the
default style to m_stream using a puts call.

While writing the tests I noticed that I was getting unnecessary style
reset sequences emitted.

The problem is that, around pagination, we don't really know what
style is currently applied to m_stream.  The
pager_file::m_applied_style tracks the style at the end of
m_wrap_buffer, but this can run ahead of the current m_stream style.
For example, if the screen is currently full, such that the next
character of output will trigger the pagination prompt, if the next
call is actually to pager_file::emit_style_escape, then
pager_file::m_applied_style will be updated, but the style of m_stream
will remain unchanged.  When the next character is written to
pager_file::puts then the pagination prompt will be presented, and GDB
will try to switch m_stream back to the default style.  Whether an
escape is emitted or not will depend on the m_applied_style value,
which we know is different than the actual style of m_stream.

It is, after all, only when m_wrap_buffer is flushed to m_stream that
the style of m_stream actually change.

And so, this commit also adds pager_file::m_stream_style.  This new
variable tracks the current style of m_stream.  This really is a
replacement for m_stream's ui_file::m_applied_style, which is not
accessible from pager_file.

When content is flushed from m_wrap_buffer to m_stream then the
current value of pager_file::m_applied_style becomes the current style
of m_stream.  But, when m_wrap_buffer is filling up, but before it is
flushed, then pager_file::m_applied_style can change, but
m_stream_style will remain unchanged.

Now in pager_file::emit_style_escape we are able to skip some of the
direct calls to m_stream->puts() used to emit style escapes.

After all this there are still a few calls to
m_stream->emit_style_escape().  These are all in the wrap_here support
code.  I think that these calls are technically broken, but don't
actually cause any issues due to the way styling works in GDB.  I
certainly haven't been able to trigger any bugs from these calls yet.
I plan to "fix" these in the next commit just for completeness.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31033

Approved-By: Tom Tromey <tom@tromey.com>

diff --git a/gdb/pager.h b/gdb/pager.h
index 052337d976eeb9d374fcb967b3db85d021df255c..9fbb310d0d0c94f1bda9642f9b24deeeb30be202 100644 (file)
--- a/gdb/pager.h
+++ b/gdb/pager.h
@@ -82,6 +82,12 @@ private:
   /* The style applied at the time that wrap_here was called.  */
   ui_file_style m_wrap_style;
 
+  /* The style currently applied to m_stream.  While m_applied_style is the
+     style that is applied to new content added to m_wrap_buffer, the
+     m_stream_style reflects changes that have been flushed to the managed
+     stream.  */
+  ui_file_style m_stream_style;
+
   /* 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/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index c10be3bc12aa10f5a13856cb14f7fad86f32ff90..503671be8e626da0edf87a90cc7be50ae15d79e7 100644 (file)
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -751,6 +751,280 @@ proc test_enable_styling_warning { } {
     }
 }
 
+# Run an 'apropos' command.  Each line of output starts with a
+# non-default style (command style).  Ensure that pagination triggers
+# during the 'apropos' output such that, at the point pagination kicks
+# in, a non-default style is in effect.
+#
+# Then, at the pagination prompt, quit the command.
+#
+# Next, run a command which switches to a different style, and then
+# back to the current style.
+#
+# At one point, a bug in the pagination code would leave the
+# non-default style from the 'apropos' command recorded as the current
+# style, such that the second command would switch back to the earlier
+# style.
+proc test_pagination_cmd_after_quit_styling {} {
+    with_ansi_styling_terminal {
+       clean_restart
+    }
+
+    # We're going to use 'apropos time'.  Check that with a height of
+    # 12 lines, each line starts with a non-default style, and that we
+    # do see the pagination prompt.  This means that there are more
+    # than 12 lines for this command.
+    with_test_prefix "validate apropos output" {
+       gdb_test_no_output "set height 12"
+
+       set saw_pagination_prompt false
+       gdb_test_multiple "apropos time" "" {
+           -re "^apropos time\r\n" {
+               exp_continue
+           }
+           -re "^\033\\\[39;49;1;27m\[^\r\n\]+\r\n" {
+               exp_continue
+           }
+           -re "^$::pagination_prompt$" {
+               set saw_pagination_prompt true
+               send_gdb "q\n"
+               exp_continue
+           }
+           -re "^q\r\n" {
+               exp_continue
+           }
+           -re "^Quit\r\n" {
+               exp_continue
+           }
+           -re "^$::gdb_prompt $" {
+               gdb_assert { $saw_pagination_prompt } $gdb_test_name
+           }
+           -re "^\[^\r\n\]+\r\n" {
+               exp_continue
+           }
+       }
+    }
+
+    # Now reduce the height to 10 and re-run 'apropos time'.  Based on
+    # the previous check, we know that this is going to present the
+    # pagination prompt when a non-default style is in use.
+    gdb_test_no_output "set height 10"
+
+    set saw_pagination_prompt false
+    gdb_test_multiple "apropos time" "" {
+       -re "$::pagination_prompt" {
+           set saw_pagination_prompt true
+           send_gdb "q\n"
+           exp_continue
+       }
+       -re "\r\n$::gdb_prompt $" {
+           gdb_assert { $saw_pagination_prompt } $gdb_test_name
+       }
+    }
+
+    # The help output for this maintenance command switches to a
+    # different style, and then back to the default.  If the
+    # pagination bug still exists, then this would switch back to the
+    # non-default style that was in use when pagination kicked in
+    # above.
+    gdb_test "maintenance time" \
+       "^\"\033\\\[39;49;1;27mmaintenance time\033\\\[m\" takes a numeric argument\\."
+}
+
+# Helper for test_pagination_prompt_styling.  Return false if STR, a
+# line that appears immediately before a pagination prompt, matches
+# the pattern for needing a style reset at the end, but does not have
+# the style reset.
+#
+# In all other cases, return true.  So lines that don't match the
+# known pattern for neededing a style reset will always return true,
+# as will lines that match the pattern, and do have the style reset.
+proc previous_line_is_ok { str } {
+
+    # Create a copy of STR with all the '\033' characters removed.
+    # Then compare string lengths to get a count of the '\033'
+    # charactes present in STR.
+    regsub -all "\033" $str {} stripped
+    set count [expr [string length $str] - [string length $stripped]]
+
+    # If STR switched styles, then it _must_ switch back again,
+    # otherwise the pagination prompt will be in the wrong style.
+    # This means that there _must_ be an even number of '\033'
+    # characters in STR.  If there is not then we switched style, but
+    # failed to switch back.
+    if { [expr $count % 2] != 0 } {
+       return false
+    }
+
+    # For lines that don't match this pattern, we cannot comment on
+    # where the style reset should occur, so lets just claim the line
+    # is fine.
+    if { ![regexp "\\s+$::hex - $::hex is \[^\r\n\]+ in " $str] } {
+       return true
+    }
+
+    # This line did match the above pattern, so we know that a style
+    # reset _must_ occur at the end of the line.  If it doesn't then
+    # this line is not OK.
+    if { ![regexp "\033\\\[m$" $str] } {
+       return false
+    }
+
+    # All tests passed, this line looks OK.
+    return true
+}
+
+# Test that the pagination prompt is displayed unstyled.  This is done
+# by looking at the 'info files' output and selecting a width that
+# will mean we should get a pagination prompt part way through a
+# styled filename.
+#
+# Then, re-run 'info files' and check that for every pagination
+# prompt, the previous line disables styling as expected.
+proc test_pagination_prompt_styling {} {
+    with_ansi_styling_terminal {
+       clean_restart $::binfile
+    }
+
+    if {![runto_main]} {
+       return
+    }
+
+    # Set height so we actually get a pagination prompt.
+    gdb_test_no_output "set height 3"
+
+    # Scan the 'info files' output and set DESIRED_WIDTH such that it
+    # will trigger pagination part-way through a styled filename.
+    set desired_width 0
+    gdb_test_multiple "info files" "find good test width" {
+       -re "^info files\r\n" {
+           exp_continue
+       }
+
+       -re "^$::pagination_prompt$" {
+           send_gdb "\n"
+           exp_continue
+       }
+
+       -re "^$::gdb_prompt $" {
+       }
+
+       -re "^((\\s+$::hex - $::hex is \[^\r\n\]+ in )\[^\r\n\]+)\r\n" {
+           if { $desired_width == 0 } {
+               set full_line $expect_out(1,string)
+               set inner_line $expect_out(2,string)
+               set desired_width [expr [string length $inner_line] + ([string length $full_line] - [string length $inner_line]) / 2]
+           }
+           exp_continue
+       }
+
+       -re "^\[^\r\n\]*\r\n" {
+           exp_continue
+       }
+    }
+
+    # Now setup the screen width.
+    gdb_test_no_output "set width $desired_width"
+
+    # Re-run 'info files'.  Check that the content before any
+    # pagination prompt correctly disables styling.
+    set saw_bad_line false
+    set prev_line ""
+    gdb_test_multiple "info files" "check pagination prompt styling" {
+       -re "^info files\r\n" {
+           exp_continue
+       }
+
+       -re "^$::pagination_prompt$" {
+           if { ![previous_line_is_ok $prev_line] } {
+               set saw_bad_line true
+           }
+           send_gdb "\n"
+           exp_continue
+       }
+
+       -re "^(\[^\r\n\]+)$::pagination_prompt$" {
+           set prev_line $expect_out(1,string)
+           if { ![previous_line_is_ok $prev_line] } {
+               set saw_bad_line true
+           }
+           send_gdb "\n"
+           exp_continue
+       }
+
+       -re "^$::gdb_prompt $" {
+           gdb_assert { !$saw_bad_line } $gdb_test_name
+       }
+
+       -re "^(\[^\r\n\]*)\r\n" {
+           set prev_line $expect_out(1,string)
+           exp_continue
+       }
+    }
+}
+
+# Test that GDB can correctly restore the current style after a
+# pagination prompt.
+#
+# Set the logging file to a garbage string based on LENGTH (is
+# actually 2x LENGTH), then 'show logging file'.  Press return at the
+# pagination prompt, and check that the reset of the filename is
+# styled correctly, and that GDB correctly switches back to the
+# default style once the logging file has finished.
+proc test_pagination_continue_styling_1 { length } {
+    with_ansi_styling_terminal {
+       clean_restart $::binfile
+    }
+
+    set filename [string repeat "ax" $length]
+
+    gdb_test_no_output "set logging file $filename"
+
+    gdb_test_no_output "set height 3"
+    gdb_test_no_output "set width 80"
+
+    set saw_bad_styling false
+    gdb_test_multiple "show logging file" "" {
+       -re "^show logging file\r\n" {
+           exp_continue
+       }
+
+       -re "^The current logfile is \"\033\\\[32;49;22;27m(?:ax)+\033\\\[m" {
+           exp_continue
+       }
+
+       -re "^\r\n\033\\\[32;49;22;27m(?:ax)+\033\\\[m(?=--)" {
+           exp_continue
+       }
+
+       -re "^\r\n\033\\\[32;49;22;27m(?:ax)+(?=--)" {
+           set saw_bad_styling true
+           exp_continue
+       }
+
+       -re "^\r\n\033\\\[32;49;22;27m(?:ax)+\033\\\[m\"\\.\r\n" {
+           exp_continue
+       }
+
+       -re "^$::gdb_prompt $" {
+           gdb_assert { !$saw_bad_styling } $gdb_test_name
+       }
+
+       -re "^$::pagination_prompt$$" {
+           send_gdb "\n"
+           exp_continue
+       }
+    }
+}
+
+# Wrapper around test_pagination_continue_styling_1, calls that
+# function with different lengths.
+proc test_pagination_continue_styling { } {
+    foreach_with_prefix length { 80 160 } {
+       test_pagination_continue_styling_1 $length
+    }
+}
+
 # Check to see if the Python styling of disassembler output is
 # expected or not, this styling requires Python support in GDB, and
 # the Python pygments module to be available.
@@ -793,3 +1067,6 @@ test_colorsupport_truecolor
 test_colorsupport_truecolor_only
 
 test_enable_styling_warning
+test_pagination_cmd_after_quit_styling
+test_pagination_prompt_styling
+test_pagination_continue_styling

diff --git a/gdb/utils.c b/gdb/utils.c
index 4f48e15b7ebe23f039acdca7c470fbe6f29a11f5..e71f1b962f35cc2931d71738f716664c96a05225 100644 (file)
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1401,7 +1401,30 @@ pager_file::emit_style_escape (const ui_file_style &style)
     {
       m_applied_style = style;
       if (m_paging)
-       m_stream->emit_style_escape (style);
+       {
+         /* Previous style changes will have been sent to m_stream via
+            escape sequences encoded in the m_wrap_buffer.  As a result,
+            the m_stream->m_applied_style will not have been updated.
+
+            If we now use m_stream->emit_style_escape, then the required
+            style might not actually be emitted as the requested style
+            might happen to match the out of date value in
+            m_stream->m_applied_style.
+
+            Instead, send the style change directly using m_stream->puts.
+
+            However, we track what style is currently applied to the
+            underlying stream in m_stream_style, this is updated whenever
+            m_wrap_buffer is flushed to the underlying stream.  And so, if
+            the style we are applying matches what we know is currently
+            applied to the underlying stream, then we can skip sending
+            this style to the stream.  */
+         if (m_stream_style != m_applied_style)
+           {
+             m_stream->puts (style.to_ansi ().c_str ());
+             m_stream_style = m_applied_style;
+           }
+       }
       else
        m_wrap_buffer.append (style.to_ansi ());
     }
@@ -1424,8 +1447,8 @@ pager_file::prompt_for_continue ()
 
   scoped_restore save_paging = make_scoped_restore (&m_paging, true);
 
-  /* Clear the current styling.  */
-  m_stream->emit_style_escape (ui_file_style ());
+  /* Clear the current styling on ourselves and the managed stream.  */
+  this->emit_style_escape (ui_file_style ());
 
   if (annotation_level > 1)
     m_stream->puts (("\n\032\032pre-prompt-for-continue\n"));
@@ -1508,6 +1531,7 @@ pager_file::flush_wrap_buffer ()
   if (!m_paging && !m_wrap_buffer.empty ())
     {
       m_stream->puts (m_wrap_buffer.c_str ());
+      m_stream_style = m_applied_style;
       m_wrap_buffer.clear ();
     }
 }
@@ -1767,7 +1791,7 @@ pager_file::puts (const char *linebuffer)
                  m_wrap_column = 0;    /* And disable fancy wrap */
                }
              else if (did_paginate)
-               m_stream->emit_style_escape (save_style);
+               this->emit_style_escape (save_style);
            }
        }