From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 112454 invoked by alias); 10 Feb 2019 12:38:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 112445 invoked by uid 89); 10 Feb 2019 12:38:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=cancel, screen X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 10 Feb 2019 12:38:24 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id C8F13560DB; Sun, 10 Feb 2019 07:38:22 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 96VsCbW30JRS; Sun, 10 Feb 2019 07:38:22 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 99F9656001; Sun, 10 Feb 2019 07:38:20 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9857E838B8; Sun, 10 Feb 2019 16:38:13 +0400 (+04) Date: Sun, 10 Feb 2019 12:38:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Fix pager bugs with style output Message-ID: <20190210123813.GA31881@adacore.com> References: <20190209183721.3486-1-tom@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190209183721.3486-1-tom@tromey.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-SW-Source: 2019-02/txt/msg00112.txt.bz2 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 > 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 > > * 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) : New method. > * source.c (print_source_lines_base): Emit escape sequences in one > piece. > > gdb/testsuite/ChangeLog > 2019-02-09 Tom Tromey > > * 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 > #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