From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65277 invoked by alias); 12 Feb 2019 13:53:57 -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 65266 invoked by uid 89); 12 Feb 2019 13:53:57 -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=placing, menu, screen X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Feb 2019 13:53:52 +0000 Received: by mail-wr1-f65.google.com with SMTP id o17so2790500wrw.3 for ; Tue, 12 Feb 2019 05:53:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=XwhWN9FFTMJMpfUQgfGC7S961yobI9yBvMV/LhOXCp8=; b=YnQeFGRDUHuuq4XsbALMjIo9vRHhIARo/9XcpGbJ7fyMiQaUGaogiJPT6i9pO8kmdC 9cSaYMo+xKJnXk2gksNLoGVulIfgIRoZW0cYMDLEuf0S2cB8mGGxFksDtWBV2nffsYn+ OVh6uOoTOUpnB6BrXYCfSPLRmf+PY6yLKjrZ99KmWi8FTmXAXM80glXmlT7e5RAUDevn alX3J+0VDEUiDFMSWc4MM5qOfxTMfCdZHo2y4UBcRSGYwRMCM3gME2+uob4wRQ/L5StV QyJvPFMgZoJ8n7wJm7I6Xk4WkPwzQPQQeugJObmALKKEuFK1mNWyovf2QhxF4Jij1uAQ 6hLw== Return-Path: Received: from localhost (host81-151-161-9.range81-151.btcentralplus.com. [81.151.161.9]) by smtp.gmail.com with ESMTPSA id b12sm2110114wmj.3.2019.02.12.05.53.48 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 12 Feb 2019 05:53:48 -0800 (PST) Date: Tue, 12 Feb 2019 13:53:00 -0000 From: Andrew Burgess To: Joel Brobecker Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [RFC] Fix pager bugs with style output Message-ID: <20190212135348.GY2829@embecosm.com> References: <20190209183721.3486-1-tom@tromey.com> <20190210123813.GA31881@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190210123813.GA31881@adacore.com> X-Fortune: Your love life will be happy and harmonious. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00135.txt.bz2 * Joel Brobecker [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 > > > > * 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