From patchwork Tue Feb 12 18:22:02 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 31423 Received: (qmail 88057 invoked by alias); 12 Feb 2019 18:22:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 88045 invoked by uid 89); 12 Feb 2019 18:22:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=canceled X-HELO: gateway23.websitewelcome.com Received: from gateway23.websitewelcome.com (HELO gateway23.websitewelcome.com) (192.185.50.108) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Feb 2019 18:22:05 +0000 Received: from cm12.websitewelcome.com (cm12.websitewelcome.com [100.42.49.8]) by gateway23.websitewelcome.com (Postfix) with ESMTP id 243E4C1EF for ; Tue, 12 Feb 2019 12:22:04 -0600 (CST) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id tch1gT478iQertch1giY7j; Tue, 12 Feb 2019 12:22:04 -0600 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=SO0BXhgamN3v8hKv7YxlFyS8HTLyY7bhJzJqv/pqc6U=; b=VN9bcdftZbnmDXe/bwV98lQH5m 6EFF1TqsDwY18n/SWw4+y23SGdnYLat5MFUp/VbNNnpZyLyaQ5bea6eDalU+47onC4oS/0KtWHNsr eZR9z8/yLQt+ivli7e0/w9ejX; Received: from 75-166-72-210.hlrn.qwest.net ([75.166.72.210]:32998 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1gtch1-003sgr-3l; Tue, 12 Feb 2019 12:22:03 -0600 From: Tom Tromey To: Joel Brobecker Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [RFC] Fix pager bugs with style output References: <20190209183721.3486-1-tom@tromey.com> <20190210123813.GA31881@adacore.com> Date: Tue, 12 Feb 2019 11:22:02 -0700 In-Reply-To: <20190210123813.GA31881@adacore.com> (Joel Brobecker's message of "Sun, 10 Feb 2019 16:38:13 +0400") Message-ID: <87k1i47tpx.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 >>>>> "Joel" == Joel Brobecker writes: Joel> Thanks for this! I haven't looked at the patch itself (and got Joel> a bit scared by the "hard to reason about" part...), :) Here's an updated version that fixes these problems. Tom commit 38209a5a87bd7532e61b386fa858b1b9011de7b9 Author: Tom Tromey Date: Thu Feb 7 06:23:40 2019 -0700 Fix pager bugs with style output 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. Even writing the tests was hard. 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. As discussed on gdb-patches, this fixes the ada-lang.c problem where filtered and unfiltered printing were mixed. Now user_select_syms uses filtered printing, which is what its callees were already doing. 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. gdb/ChangeLog 2019-02-12 Tom Tromey * ada-lang.c (user_select_syms): Use filtered printing. * 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-07 Tom Tromey * gdb.base/style.exp: Add line-wrapping tests. * gdb.base/page.exp: Add test for quitting during pagination. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 4ab3af01982..b0bef57f52b 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,22 @@ +2019-02-12 Tom Tromey + + * ada-lang.c (user_select_syms): Use filtered printing. + * 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. + 2019-02-11 Alan Hayward * aarch64-linux-tdep.c (aarch64_linux_get_syscall_number): Check diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index a878d4d1af2..d33c7571a47 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -3903,16 +3903,16 @@ user_select_syms (struct block_symbol *syms, int nsyms, int max_results) error (_("\ canceled because the command is ambiguous\n\ See set/show multiple-symbol.")); - + /* If select_mode is "all", then return all possible symbols. Only do that if more than one symbol can be selected, of course. Otherwise, display the menu as usual. */ if (select_mode == multiple_symbols_all && max_results > 1) return nsyms; - printf_unfiltered (_("[0] cancel\n")); + printf_filtered (_("[0] cancel\n")); if (max_results > 1) - printf_unfiltered (_("[1] all\n")); + printf_filtered (_("[1] all\n")); sort_choices (syms, nsyms); @@ -3926,16 +3926,16 @@ See set/show multiple-symbol.")); struct symtab_and_line sal = find_function_start_sal (syms[i].symbol, 1); - printf_unfiltered ("[%d] ", i + first_choice); + printf_filtered ("[%d] ", i + first_choice); ada_print_symbol_signature (gdb_stdout, syms[i].symbol, &type_print_raw_options); if (sal.symtab == NULL) - printf_unfiltered (_(" at :%d\n"), - sal.line); + printf_filtered (_(" at :%d\n"), + sal.line); else - printf_unfiltered (_(" at %s:%d\n"), - symtab_to_filename_for_display (sal.symtab), - sal.line); + printf_filtered (_(" at %s:%d\n"), + symtab_to_filename_for_display (sal.symtab), + sal.line); continue; } else @@ -3951,37 +3951,37 @@ See set/show multiple-symbol.")); if (SYMBOL_LINE (syms[i].symbol) != 0 && symtab != NULL) { - printf_unfiltered ("[%d] ", i + first_choice); + printf_filtered ("[%d] ", i + first_choice); ada_print_symbol_signature (gdb_stdout, syms[i].symbol, &type_print_raw_options); - printf_unfiltered (_(" at %s:%d\n"), - symtab_to_filename_for_display (symtab), - SYMBOL_LINE (syms[i].symbol)); + printf_filtered (_(" at %s:%d\n"), + symtab_to_filename_for_display (symtab), + SYMBOL_LINE (syms[i].symbol)); } else if (is_enumeral && TYPE_NAME (SYMBOL_TYPE (syms[i].symbol)) != NULL) { - printf_unfiltered (("[%d] "), i + first_choice); + printf_filtered (("[%d] "), i + first_choice); ada_print_type (SYMBOL_TYPE (syms[i].symbol), NULL, gdb_stdout, -1, 0, &type_print_raw_options); - printf_unfiltered (_("'(%s) (enumeral)\n"), - SYMBOL_PRINT_NAME (syms[i].symbol)); + printf_filtered (_("'(%s) (enumeral)\n"), + SYMBOL_PRINT_NAME (syms[i].symbol)); } else { - printf_unfiltered ("[%d] ", i + first_choice); + printf_filtered ("[%d] ", i + first_choice); ada_print_symbol_signature (gdb_stdout, syms[i].symbol, &type_print_raw_options); if (symtab != NULL) - printf_unfiltered (is_enumeral - ? _(" in %s (enumeral)\n") - : _(" at %s:?\n"), - symtab_to_filename_for_display (symtab)); + printf_filtered (is_enumeral + ? _(" in %s (enumeral)\n") + : _(" at %s:?\n"), + symtab_to_filename_for_display (symtab)); else - printf_unfiltered (is_enumeral - ? _(" (enumeral)\n") - : _(" at ?\n")); + printf_filtered (is_enumeral + ? _(" (enumeral)\n") + : _(" at ?\n")); } } } diff --git a/gdb/source.c b/gdb/source.c index c32b7c66b15..f99215f9810 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; @@ -1343,13 +1342,10 @@ print_source_lines_base (struct symtab *s, int line, int stopline, line, symtab_to_filename_for_display (s), s->nlines); const char *iter = lines.c_str (); - while (nlines-- > 0) + while (nlines-- > 0 && *iter != '\0') { char buf[20]; - c = *iter++; - if (c == '\0') - break; last_line_listed = current_source_line; if (flags & PRINT_SOURCE_LINES_FILENAME) { @@ -1358,33 +1354,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') { - xsnprintf (buf, sizeof (buf), "%c", c); + /* Treat either \r or \r\n as a single newline. */ + ++iter; + if (*iter == '\n') + ++iter; + break; + } + else if (*iter == '\n') + { + ++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/ChangeLog b/gdb/testsuite/ChangeLog index d963997d8bf..906b774c8ba 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2019-02-07 Tom Tromey + + * gdb.base/style.exp: Add line-wrapping tests. + * gdb.base/page.exp: Add test for quitting during pagination. + 2019-02-10 Joel Brobecker * gdb.ada/mi_ref_changeable: New testcase. 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