Re: [RFC] Fix pager bugs with style output

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* 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");
> >  }
> >  
> >  
> > 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