[RFC] Fix pager bugs with style output
Andrew Burgess
andrew.burgess@embecosm.com
Tue Feb 12 13:53:00 GMT 2019
* Joel Brobecker <brobecker@adacore.com> [2019-02-10 16:38:13 +0400]:
> Hi Tom,
>
> On Sat, Feb 09, 2019 at 11:37:21AM -0700, Tom Tromey wrote:
> > I believe this fixes all the pager output problems with styling that
> > Philippe pointed out, plus at least one more. The patch is somewhat
> > hard to reason about, so you may wish to give it a try.
>
> Thanks for this! I haven't looked at the patch itself (and got
> a bit scared by the "hard to reason about" part...), but what
> I could do was give it a try with AdaCore's testsuite. From
> what I can tell from the testsuite, we have two issues left.
> As far as I can tell, the first one (out-of-order output in MCQ)
> was pre-existing, while the second one (list issue) appears to be
> caused by this patch.
>
> We can reproduce the first one using the example inside
> gdb.ada/sym_print_name. I tried to do the same with a C++ program,
> but couldn't trigger the multiple-choice menu... So here goes
> with the Ada one:
>
> $ gnatmake -g foo
> $ gdb -q foo
> Reading symbols from foo...
> (gdb) p integervar
> Multiple matches for integervar
> [0] cancel
> [1] at pck.ads:18
> [2] at pck.ads:22
> pck.first.integervarpck.second.integervar>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> The "pck.first.integervar" and "pck.second.integervar" labels
> are printed out of order. The expected output should be:
>
> (gdb) p integervar
> Multiple matches for integervar
> [0] cancel
> [1] pck.first.integervar at pck.ads:18
> [2] pck.second.integervar at pck.ads:22
> >
I don't have a solution, but the issue here is that in
user_select_syms output is produced with printf_unfiltered, by passing
the line wrapping buffer, however, the symbol name is produced by a
call to ada_print_symbol_signature which uses fprintf_filtered,
placing the output into the line wrap buffer.
You'll need to reconcile these two things.
Given that ada_print_symbol_signature only seems to be called in areas
that are using unfiltered printing, simply changing
ada_print_symbol_signature to also use unfiltered printing should
provide a solution.
Hope that helps,
Thanks,
Andrew
>
> This causes a number of testcases in AdaCore's gdb-testsuite
> to timeout, waiting for the '^> ' line, which indicates the
> multiple-choice prompt.
>
> As for the second one, I can present the symptoms here, but so far,
> my efforts to reproduce it with a piece of code we can share here,
> has remained fruitless :-(. Only with the example we have, which
> unfortunately is code sent to us by a customer, can I reproduce.
> In our testsuite, we named the testcase XXXX-XXX__separate. It uses
> the "list" command, and it's as if the source had some extra empty
> lines that are not there. Eg:
>
> $ gdb -q xxx
> (gdb) list xxx.adb:1
> 1 xxxx xxxx_xx; xxx xxxx_xx;
> 2
> 3
> 4
> 5 xxxx xxxxxxx; <<<--- This should be at line 3, not 5
> 6
> 7
> 8
> 9 xxxxxxxxx xxx xx <<<--- This should be at line 5, not 9
> 10
>
> Instead, the correct output is:
>
> (xxx) xxxx xxx.xxx:1
> 1 xxxx xxxx_xx; xxx xxxx_xx;
> 2
> 3 xxxx xxxxxxx;
> 4
> 5 xxxxxxxxx xxx xx
> 6 xxxxx
> 7 xxx_xxxx("xxx-xxxx");
> 8
> 9 xxxxxxx.xxxx;
> 10 xxxxxxx.xxxxxxxxxx.xxxxxxx; -- xxxxx
>
> Not sure if it makes a difference or not, but I'm not using
> GNU Highlight (yet).
>
> > This removes the style caching, because it was difficult to keep the
> > style cache correct in all cases. Since this would cause more style
> > escapes to be emitted, instead it changes fputs_styled to try to avoid
> > unnecessary changes.
> >
> > Another bug was that the wrap buffer was not flushed in the case where
> > wrap_column==0. In the old (pre-patch series) code, characters were
> > directly emitted in this case; so flushing the wrap buffer here
> > restores this behavior.
> >
> > On error the wrap buffer must be emptied. Otherwise, interrupting
> > output can leave characters in the buffer that will be emitted later.
> >
> > Finally, it was possible for source line highlighting to be garbled
> > (and invalid escape sequences emitted) if the pager was invoked at the
> > wrong spot. To fix this, the patch arranges for source line escapes
> > to always be emitted as a unit.
> >
> > Tested by the buildbot and by trying various scenarios interactively.
> >
> > gdb/ChangeLog
> > 2019-02-09 Tom Tromey <tom@tromey.com>
> >
> > * utils.c (wrap_style): New global.
> > (desired_style): Remove.
> > (emit_style_escape): Add stream parameter.
> > (set_output_style, reset_terminal_style, prompt_for_continue):
> > Update.
> > (flush_wrap_buffer): Only flush gdb_stdout.
> > (wrap_here): Set wrap_style.
> > (fputs_maybe_filtered): Clear the wrap buffer on exception. Don't
> > treat escape sequences as a character. Change when wrap buffer is
> > flushed.
> > (fputs_styled): Do not set the output style when the default is
> > requested.
> > * ui-style.h (struct ui_file_style) <is_default>: New method.
> > * source.c (print_source_lines_base): Emit escape sequences in one
> > piece.
> >
> > gdb/testsuite/ChangeLog
> > 2019-02-09 Tom Tromey <tom@tromey.com>
> >
> > * gdb.base/style.exp: Add line-wrapping tests.
> > * gdb.base/page.exp: Add test for quitting during pagination.
> > ---
> > gdb/ChangeLog | 18 +++++++
> > gdb/source.c | 60 ++++++++++++++--------
> > gdb/testsuite/ChangeLog | 5 ++
> > gdb/testsuite/gdb.base/page.exp | 14 ++++++
> > gdb/testsuite/gdb.base/style.exp | 18 +++++++
> > gdb/ui-style.h | 10 ++++
> > gdb/utils.c | 86 +++++++++++++++++++++-----------
> > 7 files changed, 163 insertions(+), 48 deletions(-)
> >
> > diff --git a/gdb/source.c b/gdb/source.c
> > index c32b7c66b15..8a700fa8517 100644
> > --- a/gdb/source.c
> > +++ b/gdb/source.c
> > @@ -1249,7 +1249,6 @@ static void
> > print_source_lines_base (struct symtab *s, int line, int stopline,
> > print_source_lines_flags flags)
> > {
> > - int c;
> > scoped_fd desc;
> > int noprint = 0;
> > int nlines = stopline - line;
> > @@ -1347,8 +1346,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
> > {
> > char buf[20];
> >
> > - c = *iter++;
> > - if (c == '\0')
> > + if (*iter == '\0')
> > break;
> > last_line_listed = current_source_line;
> > if (flags & PRINT_SOURCE_LINES_FILENAME)
> > @@ -1358,33 +1356,55 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
> > }
> > xsnprintf (buf, sizeof (buf), "%d\t", current_source_line++);
> > uiout->text (buf);
> > - do
> > +
> > + while (*iter != '\0')
> > {
> > - if (c < 040 && c != '\t' && c != '\n' && c != '\r' && c != '\033')
> > + /* Find a run of characters that can be emitted at once.
> > + This is done so that escape sequences are kept
> > + together. */
> > + const char *start = iter;
> > + while (true)
> > {
> > - xsnprintf (buf, sizeof (buf), "^%c", c + 0100);
> > - uiout->text (buf);
> > + int skip_bytes;
> > +
> > + char c = *iter;
> > + if (c == '\033' && skip_ansi_escape (iter, &skip_bytes))
> > + iter += skip_bytes;
> > + else if (c < 040 && c != '\t')
> > + break;
> > + else if (c == 0177)
> > + break;
> > + else
> > + ++iter;
> > }
> > - else if (c == 0177)
> > - uiout->text ("^?");
> > - else if (c == '\r')
> > + if (iter > start)
> > {
> > - /* Skip a \r character, but only before a \n. */
> > - if (*iter != '\n')
> > - printf_filtered ("^%c", c + 0100);
> > + std::string text (start, iter);
> > + uiout->text (text.c_str ());
> > }
> > - else
> > + if (*iter == '\r')
> > + {
> > + /* Treat either \r or \r\n as a single newline. */
> > + ++iter;
> > + if (iter[1] == '\n')
> > + ++iter;
> > + break;
> > + }
> > + else if (*iter == '\n')
> > {
> > - xsnprintf (buf, sizeof (buf), "%c", c);
> > + ++iter;
> > + break;
> > + }
> > + else if (*iter > 0 && *iter < 040)
> > + {
> > + xsnprintf (buf, sizeof (buf), "^%c", *iter + 0100);
> > uiout->text (buf);
> > }
> > + else if (*iter == 0177)
> > + uiout->text ("^?");
> > }
> > - while (c != '\n' && (c = *iter++) != '\0');
> > - if (c == '\0')
> > - break;
> > + uiout->text ("\n");
> > }
> > - if (!lines.empty() && lines.back () != '\n')
> > - uiout->text ("\n");
> > }
> >
> >
> > diff --git a/gdb/testsuite/gdb.base/page.exp b/gdb/testsuite/gdb.base/page.exp
> > index 74615911d25..10ebf0d43b2 100644
> > --- a/gdb/testsuite/gdb.base/page.exp
> > +++ b/gdb/testsuite/gdb.base/page.exp
> > @@ -80,6 +80,20 @@ gdb_expect_list "paged count remainder" "${gdb_prompt} " {
> > 11
> > }
> >
> > +set fours [string repeat 4 40]
> > +set str "1\\n2\\n3\\n$fours\\n5\\n"
> > +
> > +# Avoid some confusing output from readline.
> > +gdb_test_no_output "set editing off"
> > +
> > +gdb_test_no_output "set width 30"
> > +send_gdb "printf \"$str\"\n"
> > +gdb_expect_list "paged count for interrupt" \
> > + ".*$pagination_prompt" \
> > + [list 1\r\n 2\r\n 3\r\n 444444444444444444444444444444]
> > +
> > +gdb_test "q" "Quit" "quit while paging"
> > +
> > gdb_exit
> > return 0
> >
> > diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
> > index 78d04b02903..010d9592f34 100644
> > --- a/gdb/testsuite/gdb.base/style.exp
> > +++ b/gdb/testsuite/gdb.base/style.exp
> > @@ -45,6 +45,24 @@ save_vars { env(TERM) } {
> >
> > gdb_test "print &main" " = .* \033\\\[34m$hex\033\\\[m <$main_expr>"
> >
> > + # Regression test for a bug where line-wrapping would occur at the
> > + # wrong spot with styling. There were different bugs at different
> > + # widths, so try two.
> > + foreach width {20 30} {
> > + gdb_test_no_output "set width $width"
> > + # There was also a bug where the styling could be wrong in the
> > + # line listing; this is why the words from the source code are
> > + # spelled out in the final result line of the test.
> > + gdb_test "frame" \
> > + [multi_line \
> > + "#0 *$main_expr.*$arg_expr.*" \
> > + ".*$arg_expr.*" \
> > + ".* at .*$file_expr.*" \
> > + "\[0-9\]+.*return.* break here .*"
> > + ] \
> > + "frame when width=$width"
> > + }
> > +
> > gdb_exit
> > gdb_spawn
> >
> > diff --git a/gdb/ui-style.h b/gdb/ui-style.h
> > index d719438c726..04a1d448ba9 100644
> > --- a/gdb/ui-style.h
> > +++ b/gdb/ui-style.h
> > @@ -163,6 +163,16 @@ struct ui_file_style
> > /* Return the ANSI escape sequence for this style. */
> > std::string to_ansi () const;
> >
> > + /* Return true if this style is the default style; false
> > + otherwise. */
> > + bool is_default () const
> > + {
> > + return (m_foreground == NONE
> > + && m_background == NONE
> > + && m_intensity == NORMAL
> > + && !m_reverse);
> > + }
> > +
> > /* Return true if this style specified reverse display; false
> > otherwise. */
> > bool is_reverse () const
> > diff --git a/gdb/utils.c b/gdb/utils.c
> > index 6fb5736abb5..c383a4bff06 100644
> > --- a/gdb/utils.c
> > +++ b/gdb/utils.c
> > @@ -72,6 +72,7 @@
> > #include <algorithm>
> > #include "common/pathstuff.h"
> > #include "cli/cli-style.h"
> > +#include "common/scope-exit.h"
> >
> > void (*deprecated_error_begin_hook) (void);
> >
> > @@ -1282,6 +1283,9 @@ static const char *wrap_indent;
> > /* Column number on the screen where wrap_buffer begins, or 0 if wrapping
> > is not in effect. */
> > static int wrap_column;
> > +
> > +/* The style applied at the time that wrap_here was called. */
> > +static ui_file_style wrap_style;
> >
> >
> > /* Initialize the number of lines per page and chars per line. */
> > @@ -1427,21 +1431,19 @@ set_screen_width_and_height (int width, int height)
> >
> > static ui_file_style applied_style;
> >
> > -/* The currently desired style. This can differ from the applied
> > - style when showing the pagination prompt. */
> > -
> > -static ui_file_style desired_style;
> > -
> > -/* Emit an ANSI style escape for STYLE to the wrap buffer. */
> > +/* Emit an ANSI style escape for STYLE. If STREAM is nullptr, emit to
> > + the wrap buffer; otherwise emit to STREAM. */
> >
> > static void
> > -emit_style_escape (const ui_file_style &style)
> > +emit_style_escape (const ui_file_style &style,
> > + struct ui_file *stream = nullptr)
> > {
> > - if (applied_style == style)
> > - return;
> > applied_style = style;
> >
> > - wrap_buffer.append (style.to_ansi ());
> > + if (stream == nullptr)
> > + wrap_buffer.append (style.to_ansi ());
> > + else
> > + fputs_unfiltered (style.to_ansi ().c_str (), stream);
> > }
> >
> > /* See utils.h. */
> > @@ -1465,11 +1467,9 @@ can_emit_style_escape (struct ui_file *stream)
> > static void
> > set_output_style (struct ui_file *stream, const ui_file_style &style)
> > {
> > - if (!can_emit_style_escape (stream)
> > - || style == desired_style)
> > + if (!can_emit_style_escape (stream))
> > return;
> >
> > - desired_style = style;
> > emit_style_escape (style);
> > }
> >
> > @@ -1482,9 +1482,8 @@ reset_terminal_style (struct ui_file *stream)
> > {
> > /* Force the setting, regardless of what we think the setting
> > might already be. */
> > - desired_style = ui_file_style ();
> > - applied_style = desired_style;
> > - wrap_buffer.append (desired_style.to_ansi ());
> > + applied_style = ui_file_style ();
> > + wrap_buffer.append (applied_style.to_ansi ());
> > }
> > }
> >
> > @@ -1551,7 +1550,7 @@ prompt_for_continue (void)
> > pagination_disabled_for_command = disable_pagination;
> >
> > /* Restore the current styling. */
> > - emit_style_escape (desired_style);
> > + emit_style_escape (applied_style);
> >
> > dont_repeat (); /* Forget prev cmd -- CR won't repeat it. */
> > }
> > @@ -1589,7 +1588,7 @@ reinitialize_more_filter (void)
> > static void
> > flush_wrap_buffer (struct ui_file *stream)
> > {
> > - if (!wrap_buffer.empty ())
> > + if (stream == gdb_stdout && !wrap_buffer.empty ())
> > {
> > fputs_unfiltered (wrap_buffer.c_str (), stream);
> > wrap_buffer.clear ();
> > @@ -1644,6 +1643,7 @@ wrap_here (const char *indent)
> > wrap_indent = "";
> > else
> > wrap_indent = indent;
> > + wrap_style = applied_style;
> > }
> > }
> >
> > @@ -1743,6 +1743,14 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
> > return;
> > }
> >
> > + auto buffer_clearer
> > + = make_scope_exit ([&] ()
> > + {
> > + wrap_buffer.clear ();
> > + wrap_column = 0;
> > + wrap_indent = "";
> > + });
> > +
> > /* Go through and output each character. Show line extension
> > when this is necessary; prompt user for new page when this is
> > necessary. */
> > @@ -1759,6 +1767,8 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
> >
> > while (*lineptr && *lineptr != '\n')
> > {
> > + int skip_bytes;
> > +
> > /* Print a single line. */
> > if (*lineptr == '\t')
> > {
> > @@ -1769,6 +1779,14 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
> > chars_printed = ((chars_printed >> 3) + 1) << 3;
> > lineptr++;
> > }
> > + else if (*lineptr == '\033'
> > + && skip_ansi_escape (lineptr, &skip_bytes))
> > + {
> > + wrap_buffer.append (lineptr, skip_bytes);
> > + /* Note that we don't consider this a character, so we
> > + don't increment chars_printed here. */
> > + lineptr += skip_bytes;
> > + }
> > else
> > {
> > wrap_buffer.push_back (*lineptr);
> > @@ -1782,15 +1800,18 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
> >
> > chars_printed = 0;
> > lines_printed++;
> > - /* If we aren't actually wrapping, don't output newline --
> > - if chars_per_line is right, we probably just overflowed
> > - anyway; if it's wrong, let us keep going. */
> > if (wrap_column)
> > {
> > - emit_style_escape (ui_file_style ());
> > - flush_wrap_buffer (stream);
> > + if (can_emit_style_escape (stream))
> > + emit_style_escape (ui_file_style (), stream);
> > + /* If we aren't actually wrapping, don't output
> > + newline -- if chars_per_line is right, we
> > + probably just overflowed anyway; if it's wrong,
> > + let us keep going. */
> > fputc_unfiltered ('\n', stream);
> > }
> > + else
> > + flush_wrap_buffer (stream);
> >
> > /* Possible new page. Note that
> > PAGINATION_DISABLED_FOR_COMMAND might be set during
> > @@ -1803,8 +1824,8 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
> > if (wrap_column)
> > {
> > fputs_unfiltered (wrap_indent, stream);
> > - emit_style_escape (desired_style);
> > - flush_wrap_buffer (stream);
> > + if (can_emit_style_escape (stream))
> > + emit_style_escape (wrap_style, stream);
> > /* FIXME, this strlen is what prevents wrap_indent from
> > containing tabs. However, if we recurse to print it
> > and count its chars, we risk trouble if wrap_indent is
> > @@ -1828,6 +1849,8 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
> > lineptr++;
> > }
> > }
> > +
> > + buffer_clearer.release ();
> > }
> >
> > void
> > @@ -1842,9 +1865,16 @@ void
> > fputs_styled (const char *linebuffer, const ui_file_style &style,
> > struct ui_file *stream)
> > {
> > - set_output_style (stream, style);
> > - fputs_maybe_filtered (linebuffer, stream, 1);
> > - set_output_style (stream, ui_file_style ());
> > + /* This just makes it so we emit somewhat fewer escape
> > + sequences. */
> > + if (style.is_default ())
> > + fputs_maybe_filtered (linebuffer, stream, 1);
> > + else
> > + {
> > + set_output_style (stream, style);
> > + fputs_maybe_filtered (linebuffer, stream, 1);
> > + set_output_style (stream, ui_file_style ());
> > + }
> > }
> >
> > int
> > --
> > 2.17.2
>
> --
> Joel
More information about the Gdb-patches
mailing list