* [RFC] Fix pager bugs with style output
@ 2019-02-09 18:37 Tom Tromey
2019-02-10 12:38 ` Joel Brobecker
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2019-02-09 18:37 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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.
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");
}
\f
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;
\f
/* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-09 18:37 [RFC] Fix pager bugs with style output Tom Tromey
@ 2019-02-10 12:38 ` Joel Brobecker
2019-02-12 13:53 ` Andrew Burgess
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Joel Brobecker @ 2019-02-10 12:38 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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 <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");
> }
> \f
>
> 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;
> \f
>
> /* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-10 12:38 ` Joel Brobecker
@ 2019-02-12 13:53 ` Andrew Burgess
2019-02-12 17:08 ` Tom Tromey
2019-02-12 17:13 ` Tom Tromey
2019-02-12 18:22 ` Tom Tromey
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2019-02-12 13:53 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Tom Tromey, gdb-patches
* 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");
> > }
> > \f
> >
> > 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;
> > \f
> >
> > /* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-12 13:53 ` Andrew Burgess
@ 2019-02-12 17:08 ` Tom Tromey
2019-02-14 3:34 ` Joel Brobecker
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2019-02-12 17:08 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Joel Brobecker, Tom Tromey, gdb-patches
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
Andrew> I don't have a solution, but the issue here is that in
Andrew> user_select_syms output is produced with printf_unfiltered, by passing
Andrew> the line wrapping buffer, however, the symbol name is produced by a
Andrew> call to ada_print_symbol_signature which uses fprintf_filtered,
Andrew> placing the output into the line wrap buffer.
Thanks for looking into this.
Andrew> You'll need to reconcile these two things.
Andrew> Given that ada_print_symbol_signature only seems to be called in areas
Andrew> that are using unfiltered printing, simply changing
Andrew> ada_print_symbol_signature to also use unfiltered printing should
Andrew> provide a solution.
How about having user_select_syms also use filtered output?
That seems like it would be somewhat friendlier in the unlikely event
that the menu was very long -- it would give the user a chance to read
the choices.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-10 12:38 ` Joel Brobecker
2019-02-12 13:53 ` Andrew Burgess
@ 2019-02-12 17:13 ` Tom Tromey
2019-02-12 18:22 ` Tom Tromey
2 siblings, 0 replies; 9+ messages in thread
From: Tom Tromey @ 2019-02-12 17:13 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Tom Tromey, gdb-patches
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> As for the second one, I can present the symptoms here, but so far,
Joel> my efforts to reproduce it with a piece of code we can share here,
Joel> has remained fruitless :-(. Only with the example we have, which
Joel> unfortunately is code sent to us by a customer, can I reproduce.
Joel> In our testsuite, we named the testcase XXXX-XXX__separate. It uses
Joel> the "list" command, and it's as if the source had some extra empty
Joel> lines that are not there.
[...]
Joel> Not sure if it makes a difference or not, but I'm not using
Joel> GNU Highlight (yet).
It does make a difference, thanks for noting that.
It's easy to see the bug with a short C file that uses \r\n line endings.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-10 12:38 ` Joel Brobecker
2019-02-12 13:53 ` Andrew Burgess
2019-02-12 17:13 ` Tom Tromey
@ 2019-02-12 18:22 ` Tom Tromey
2019-02-17 12:31 ` Joel Brobecker
2 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2019-02-12 18:22 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Tom Tromey, gdb-patches
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> 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 <tom@tromey.com>
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 <tom@tromey.com>
* 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) <is_default>: New method.
* source.c (print_source_lines_base): Emit escape sequences in one
piece.
gdb/testsuite/ChangeLog
2019-02-07 Tom Tromey <tom@tromey.com>
* 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 <tom@tromey.com>
+
+ * 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) <is_default>: New method.
+ * source.c (print_source_lines_base): Emit escape sequences in one
+ piece.
+
2019-02-11 Alan Hayward <alan.hayward@arm.com>
* 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 <no source file available>:%d\n"),
- sal.line);
+ printf_filtered (_(" at <no source file available>:%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");
}
\f
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 <tom@tromey.com>
+
+ * gdb.base/style.exp: Add line-wrapping tests.
+ * gdb.base/page.exp: Add test for quitting during pagination.
+
2019-02-10 Joel Brobecker <brobecker@adacore.com>
* 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 <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;
\f
/* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-12 17:08 ` Tom Tromey
@ 2019-02-14 3:34 ` Joel Brobecker
0 siblings, 0 replies; 9+ messages in thread
From: Joel Brobecker @ 2019-02-14 3:34 UTC (permalink / raw)
To: Tom Tromey; +Cc: Andrew Burgess, gdb-patches
> Andrew> I don't have a solution, but the issue here is that in
> Andrew> user_select_syms output is produced with printf_unfiltered, by passing
> Andrew> the line wrapping buffer, however, the symbol name is produced by a
> Andrew> call to ada_print_symbol_signature which uses fprintf_filtered,
> Andrew> placing the output into the line wrap buffer.
>
> Thanks for looking into this.
>
> Andrew> You'll need to reconcile these two things.
>
> Andrew> Given that ada_print_symbol_signature only seems to be called in areas
> Andrew> that are using unfiltered printing, simply changing
> Andrew> ada_print_symbol_signature to also use unfiltered printing should
> Andrew> provide a solution.
>
> How about having user_select_syms also use filtered output?
> That seems like it would be somewhat friendlier in the unlikely event
> that the menu was very long -- it would give the user a chance to read
> the choices.
I think it makes sense, especially now that you enhanced the paging
to allow us to print everything without being asked about paging
anymore. It should be fairly easy to revert back if we find that
this is causing unforseen problems.
--
Joel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-12 18:22 ` Tom Tromey
@ 2019-02-17 12:31 ` Joel Brobecker
2019-02-17 15:34 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Joel Brobecker @ 2019-02-17 12:31 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
Hi Tom,
> Here's an updated version that fixes these problems.
Thanks Tom! This whole thing _is_ complex, and I understand now why
you took out the style caching..
> commit 38209a5a87bd7532e61b386fa858b1b9011de7b9
> Author: Tom Tromey <tom@tromey.com>
> 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 <tom@tromey.com>
>
> * 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) <is_default>: New method.
> * source.c (print_source_lines_base): Emit escape sequences in one
> piece.
>
> gdb/testsuite/ChangeLog
> 2019-02-07 Tom Tromey <tom@tromey.com>
>
> * gdb.base/style.exp: Add line-wrapping tests.
> * gdb.base/page.exp: Add test for quitting during pagination.
Good news: As far as I can tell, this patch fixes all issues detected
by the AdaCore testsuite :-).
I reviewed the patch as best as I could, and the changes make sense
to me. Ingesting more in detail how the pager and the line wrapping
works might yield a more throrough review; but between the new testcases
and AdaCore's testsuite, I think it's pretty good. If there are bugs
still left lurking, we can address them one by one, with corresponding
testcases.
One thought, perhaps: Would it make sense to call can_emit_style_escape
inside emit_style_escape? I'm not entirely sure whether this would
always work or not. On the one hand, the issue with relying on callers
of emit_style_escape to verify that it makes sense is that we're bound
to forget again in the future. On the other hand, I'm trying to think
what would happen is a user changed XTERM from xterm to dump, for
instance. Normally, we should always be in the normal style when the
user has the prompt, so there would be no need to reset it back to
the default style. One thing I did notice is that a couple of calls
to emit_style_escape are unprotected in prompt_for_continue: From
what I can tell, it seems like we'd be emitting ansi sequences when
in fact we shouldn't?
Other than that, everything looked OK for me.
> 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 <tom@tromey.com>
> +
> + * 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) <is_default>: New method.
> + * source.c (print_source_lines_base): Emit escape sequences in one
> + piece.
> +
> 2019-02-11 Alan Hayward <alan.hayward@arm.com>
>
> * 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 <no source file available>:%d\n"),
> - sal.line);
> + printf_filtered (_(" at <no source file available>:%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");
> }
> \f
>
> 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 <tom@tromey.com>
> +
> + * gdb.base/style.exp: Add line-wrapping tests.
> + * gdb.base/page.exp: Add test for quitting during pagination.
> +
> 2019-02-10 Joel Brobecker <brobecker@adacore.com>
>
> * 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 <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;
> \f
>
> /* 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
--
Joel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Fix pager bugs with style output
2019-02-17 12:31 ` Joel Brobecker
@ 2019-02-17 15:34 ` Tom Tromey
0 siblings, 0 replies; 9+ messages in thread
From: Tom Tromey @ 2019-02-17 15:34 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Tom Tromey, gdb-patches
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> One thought, perhaps: Would it make sense to call can_emit_style_escape
Joel> inside emit_style_escape? I'm not entirely sure whether this would
Joel> always work or not. On the one hand, the issue with relying on callers
Joel> of emit_style_escape to verify that it makes sense is that we're bound
Joel> to forget again in the future. On the other hand, I'm trying to think
Joel> what would happen is a user changed XTERM from xterm to dump, for
Joel> instance. Normally, we should always be in the normal style when the
Joel> user has the prompt, so there would be no need to reset it back to
Joel> the default style. One thing I did notice is that a couple of calls
Joel> to emit_style_escape are unprotected in prompt_for_continue: From
Joel> what I can tell, it seems like we'd be emitting ansi sequences when
Joel> in fact we shouldn't?
This is all more subtle than I would like :(
The call to emit_style_escape in set_output_style deliberately does not
pass the stream argument. But, it does test can_emit_style_escape on
the stream. Changing this causes regressions because now the output
won't end up in the wrap buffer. I added a comment to set_output_style.
The calls to in prompt_for_continue do indeed need protection now that
the caching is gone. I've fixed these. Also they were emitting to the
wrap buffer, which seems wrong (at least for the first one), because the
wrap buffer isn't flushed here.
There are still mysteries in here to me, too, like why one call to
prompt_for_continue checks "filter" but the other does not.
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-17 15:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-09 18:37 [RFC] Fix pager bugs with style output Tom Tromey
2019-02-10 12:38 ` Joel Brobecker
2019-02-12 13:53 ` Andrew Burgess
2019-02-12 17:08 ` Tom Tromey
2019-02-14 3:34 ` Joel Brobecker
2019-02-12 17:13 ` Tom Tromey
2019-02-12 18:22 ` Tom Tromey
2019-02-17 12:31 ` Joel Brobecker
2019-02-17 15:34 ` Tom Tromey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).