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>

0 files changed, 0 insertions, 0 deletions