Re: [RFC] Fix pager bugs with style output

[Joel Brobecker <brobecker at adacore dot com>]

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");
>  }
>  
>  
> diff --git a/gdb/testsuite/gdb.base/page.exp b/gdb/testsuite/gdb.base/page.exp
> index 74615911d25..10ebf0d43b2 100644
> --- a/gdb/testsuite/gdb.base/page.exp
> +++ b/gdb/testsuite/gdb.base/page.exp
> @@ -80,6 +80,20 @@ gdb_expect_list "paged count remainder" "${gdb_prompt} " {
>      11
>  }
>  
> +set fours [string repeat 4 40]
> +set str "1\\n2\\n3\\n$fours\\n5\\n"
> +
> +# Avoid some confusing output from readline.
> +gdb_test_no_output "set editing off"
> +
> +gdb_test_no_output "set width 30"
> +send_gdb "printf \"$str\"\n"
> +gdb_expect_list "paged count for interrupt" \
> +    ".*$pagination_prompt" \
> +    [list 1\r\n 2\r\n 3\r\n 444444444444444444444444444444]
> +
> +gdb_test "q" "Quit" "quit while paging"
> +
>  gdb_exit
>  return 0
>  
> diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
> index 78d04b02903..010d9592f34 100644
> --- a/gdb/testsuite/gdb.base/style.exp
> +++ b/gdb/testsuite/gdb.base/style.exp
> @@ -45,6 +45,24 @@ save_vars { env(TERM) } {
>  
>      gdb_test "print &main" " = .* \033\\\[34m$hex\033\\\[m <$main_expr>"
>  
> +    # Regression test for a bug where line-wrapping would occur at the
> +    # wrong spot with styling.  There were different bugs at different
> +    # widths, so try two.
> +    foreach width {20 30} {
> +	gdb_test_no_output "set width $width"
> +	# There was also a bug where the styling could be wrong in the
> +	# line listing; this is why the words from the source code are
> +	# spelled out in the final result line of the test.
> +	gdb_test "frame" \
> +	    [multi_line \
> +		 "#0 *$main_expr.*$arg_expr.*" \
> +		 ".*$arg_expr.*" \
> +		 ".* at .*$file_expr.*" \
> +		 "\[0-9\]+.*return.* break here .*"
> +	    ] \
> +	    "frame when width=$width"
> +    }
> +
>      gdb_exit
>      gdb_spawn
>  
> diff --git a/gdb/ui-style.h b/gdb/ui-style.h
> index d719438c726..04a1d448ba9 100644
> --- a/gdb/ui-style.h
> +++ b/gdb/ui-style.h
> @@ -163,6 +163,16 @@ struct ui_file_style
>    /* Return the ANSI escape sequence for this style.  */
>    std::string to_ansi () const;
>  
> +  /* Return true if this style is the default style; false
> +     otherwise.  */
> +  bool is_default () const
> +  {
> +    return (m_foreground == NONE
> +	    && m_background == NONE
> +	    && m_intensity == NORMAL
> +	    && !m_reverse);
> +  }
> +
>    /* Return true if this style specified reverse display; false
>       otherwise.  */
>    bool is_reverse () const
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 6fb5736abb5..c383a4bff06 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -72,6 +72,7 @@
>  #include <algorithm>
>  #include "common/pathstuff.h"
>  #include "cli/cli-style.h"
> +#include "common/scope-exit.h"
>  
>  void (*deprecated_error_begin_hook) (void);
>  
> @@ -1282,6 +1283,9 @@ static const char *wrap_indent;
>  /* Column number on the screen where wrap_buffer begins, or 0 if wrapping
>     is not in effect.  */
>  static int wrap_column;
> +
> +/* The style applied at the time that wrap_here was called.  */
> +static ui_file_style wrap_style;
>  
>  
>  /* Initialize the number of lines per page and chars per line.  */
> @@ -1427,21 +1431,19 @@ set_screen_width_and_height (int width, int height)
>  
>  static ui_file_style applied_style;
>  
> -/* The currently desired style.  This can differ from the applied
> -   style when showing the pagination prompt.  */
> -
> -static ui_file_style desired_style;
> -
> -/* Emit an ANSI style escape for STYLE to the wrap buffer.  */
> +/* Emit an ANSI style escape for STYLE.  If STREAM is nullptr, emit to
> +   the wrap buffer; otherwise emit to STREAM.  */
>  
>  static void
> -emit_style_escape (const ui_file_style &style)
> +emit_style_escape (const ui_file_style &style,
> +		   struct ui_file *stream = nullptr)
>  {
> -  if (applied_style == style)
> -    return;
>    applied_style = style;
>  
> -  wrap_buffer.append (style.to_ansi ());
> +  if (stream == nullptr)
> +    wrap_buffer.append (style.to_ansi ());
> +  else
> +    fputs_unfiltered (style.to_ansi ().c_str (), stream);
>  }
>  
>  /* See utils.h.  */
> @@ -1465,11 +1467,9 @@ can_emit_style_escape (struct ui_file *stream)
>  static void
>  set_output_style (struct ui_file *stream, const ui_file_style &style)
>  {
> -  if (!can_emit_style_escape (stream)
> -      || style == desired_style)
> +  if (!can_emit_style_escape (stream))
>      return;
>  
> -  desired_style = style;
>    emit_style_escape (style);
>  }
>  
> @@ -1482,9 +1482,8 @@ reset_terminal_style (struct ui_file *stream)
>      {
>        /* Force the setting, regardless of what we think the setting
>  	 might already be.  */
> -      desired_style = ui_file_style ();
> -      applied_style = desired_style;
> -      wrap_buffer.append (desired_style.to_ansi ());
> +      applied_style = ui_file_style ();
> +      wrap_buffer.append (applied_style.to_ansi ());
>      }
>  }
>  
> @@ -1551,7 +1550,7 @@ prompt_for_continue (void)
>    pagination_disabled_for_command = disable_pagination;
>  
>    /* Restore the current styling.  */
> -  emit_style_escape (desired_style);
> +  emit_style_escape (applied_style);
>  
>    dont_repeat ();		/* Forget prev cmd -- CR won't repeat it.  */
>  }
> @@ -1589,7 +1588,7 @@ reinitialize_more_filter (void)
>  static void
>  flush_wrap_buffer (struct ui_file *stream)
>  {
> -  if (!wrap_buffer.empty ())
> +  if (stream == gdb_stdout && !wrap_buffer.empty ())
>      {
>        fputs_unfiltered (wrap_buffer.c_str (), stream);
>        wrap_buffer.clear ();
> @@ -1644,6 +1643,7 @@ wrap_here (const char *indent)
>  	wrap_indent = "";
>        else
>  	wrap_indent = indent;
> +      wrap_style = applied_style;
>      }
>  }
>  
> @@ -1743,6 +1743,14 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>        return;
>      }
>  
> +  auto buffer_clearer
> +    = make_scope_exit ([&] ()
> +		       {
> +			 wrap_buffer.clear ();
> +			 wrap_column = 0;
> +			 wrap_indent = "";
> +		       });
> +
>    /* Go through and output each character.  Show line extension
>       when this is necessary; prompt user for new page when this is
>       necessary.  */
> @@ -1759,6 +1767,8 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>  
>        while (*lineptr && *lineptr != '\n')
>  	{
> +	  int skip_bytes;
> +
>  	  /* Print a single line.  */
>  	  if (*lineptr == '\t')
>  	    {
> @@ -1769,6 +1779,14 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>  	      chars_printed = ((chars_printed >> 3) + 1) << 3;
>  	      lineptr++;
>  	    }
> +	  else if (*lineptr == '\033'
> +		   && skip_ansi_escape (lineptr, &skip_bytes))
> +	    {
> +	      wrap_buffer.append (lineptr, skip_bytes);
> +	      /* Note that we don't consider this a character, so we
> +		 don't increment chars_printed here.  */
> +	      lineptr += skip_bytes;
> +	    }
>  	  else
>  	    {
>  	      wrap_buffer.push_back (*lineptr);
> @@ -1782,15 +1800,18 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>  
>  	      chars_printed = 0;
>  	      lines_printed++;
> -	      /* If we aren't actually wrapping, don't output newline --
> -	         if chars_per_line is right, we probably just overflowed
> -	         anyway; if it's wrong, let us keep going.  */
>  	      if (wrap_column)
>  		{
> -		  emit_style_escape (ui_file_style ());
> -		  flush_wrap_buffer (stream);
> +		  if (can_emit_style_escape (stream))
> +		    emit_style_escape (ui_file_style (), stream);
> +		  /* If we aren't actually wrapping, don't output
> +		     newline -- if chars_per_line is right, we
> +		     probably just overflowed anyway; if it's wrong,
> +		     let us keep going.  */
>  		  fputc_unfiltered ('\n', stream);
>  		}
> +	      else
> +		flush_wrap_buffer (stream);
>  
>  	      /* Possible new page.  Note that
>  		 PAGINATION_DISABLED_FOR_COMMAND might be set during
> @@ -1803,8 +1824,8 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>  	      if (wrap_column)
>  		{
>  		  fputs_unfiltered (wrap_indent, stream);
> -		  emit_style_escape (desired_style);
> -		  flush_wrap_buffer (stream);
> +		  if (can_emit_style_escape (stream))
> +		    emit_style_escape (wrap_style, stream);
>  		  /* FIXME, this strlen is what prevents wrap_indent from
>  		     containing tabs.  However, if we recurse to print it
>  		     and count its chars, we risk trouble if wrap_indent is
> @@ -1828,6 +1849,8 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>  	  lineptr++;
>  	}
>      }
> +
> +  buffer_clearer.release ();
>  }
>  
>  void
> @@ -1842,9 +1865,16 @@ void
>  fputs_styled (const char *linebuffer, const ui_file_style &style,
>  	      struct ui_file *stream)
>  {
> -  set_output_style (stream, style);
> -  fputs_maybe_filtered (linebuffer, stream, 1);
> -  set_output_style (stream, ui_file_style ());
> +  /* This just makes it so we emit somewhat fewer escape
> +     sequences.  */
> +  if (style.is_default ())
> +    fputs_maybe_filtered (linebuffer, stream, 1);
> +  else
> +    {
> +      set_output_style (stream, style);
> +      fputs_maybe_filtered (linebuffer, stream, 1);
> +      set_output_style (stream, ui_file_style ());
> +    }
>  }
>  
>  int
> -- 
> 2.17.2

-- 
Joel