[RFC,1/8] Change wrap buffering to use a std::string

Message ID 20180906211303.11029-2-tom@tromey.com
State New
Headers show
Series
  • add terminal styling to gdb
Related show

Commit Message

Tom Tromey Sept. 6, 2018, 9:12 p.m.
Currently wrap buffering is implemented by allocating a string that is
the same width as the window, and then writing characters into it.
However, if gdb emits terminal escapes, then these could possibly
overflow the buffer.

To prevent this, change the wrap buffer to be a std::string and update
the various uses.

gdb/ChangeLog
2018-09-06  Tom Tromey  <tom@tromey.com>

	* utils.c (filter_initalized): New global.
	(wrap_buffer): Now a std::string.
	(wrap_pointer): Remove.
	(filtered_printing_initialized, set_width, wrap_here)
	(fputs_maybe_filtered): Update.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/utils.c   | 47 ++++++++++++++++-------------------------------
 2 files changed, 24 insertions(+), 31 deletions(-)

-- 
2.13.6

Comments

Simon Marchi Oct. 6, 2018, 3:19 p.m. | #1
On 2018-09-06 5:12 p.m., Tom Tromey wrote:
> Currently wrap buffering is implemented by allocating a string that is

> the same width as the window, and then writing characters into it.

> However, if gdb emits terminal escapes, then these could possibly

> overflow the buffer.

> 

> To prevent this, change the wrap buffer to be a std::string and update

> the various uses.


This looks like a good change to me, independently of this series.  I think you
should push it right away.

> diff --git a/gdb/utils.c b/gdb/utils.c

> index 7a8c80c64ed..1982fa20e64 100644

> --- a/gdb/utils.c

> +++ b/gdb/utils.c

> @@ -1268,13 +1268,11 @@ static bool pagination_disabled_for_command;

>     the end of the line, we spit out a newline, the indent, and then

>     the buffered output.  */

>  

> -/* Malloc'd buffer with chars_per_line+2 bytes.  Contains characters which

> -   are waiting to be output (they have already been counted in chars_printed).

> -   When wrap_buffer[0] is null, the buffer is empty.  */

> -static char *wrap_buffer;

> +static bool filter_initalized = false;


Typo, "initalized".

> @@ -1546,17 +1537,13 @@ void

>  wrap_here (const char *indent)

>  {

>    /* This should have been allocated, but be paranoid anyway.  */

> -  if (!wrap_buffer)

> +  if (!filter_initalized)

>      internal_error (__FILE__, __LINE__,

>  		    _("failed internal consistency check"));


This should be gdb_assert (filter_initialized), IMO.

Simon
Tom Tromey Oct. 8, 2018, 10:04 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


>> Currently wrap buffering is implemented by allocating a string that is

>> the same width as the window, and then writing characters into it.

>> However, if gdb emits terminal escapes, then these could possibly

>> overflow the buffer.

>> 

>> To prevent this, change the wrap buffer to be a std::string and update

>> the various uses.


Simon> This looks like a good change to me, independently of this series.  I think you
Simon> should push it right away.

FWIW I think this patch will have to change to accommodate Windows -- or
at least be totally obsoleted by the needed change.  My plan is to have
a vector holding strings with their styling.  This has to happen because
styling on Windows is done via an API, not via an escape sequence.

Tom
Tom Tromey Oct. 18, 2018, 10:15 p.m. | #3
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:


>>> To prevent this, change the wrap buffer to be a std::string and update

>>> the various uses.


Simon> This looks like a good change to me, independently of this series.  I think you
Simon> should push it right away.

Tom> FWIW I think this patch will have to change to accommodate Windows -- or
Tom> at least be totally obsoleted by the needed change.  My plan is to have
Tom> a vector holding strings with their styling.  This has to happen because
Tom> styling on Windows is done via an API, not via an escape sequence.

I think I'm going to change plans here, due to my desire to also
style source code in the TUI.

GNU Source Highlight and Pygments both have ANSI terminal escape back
ends.  And, I already have code that can parse ANSI escapes and turn
them into curses calls.

My new plan for handling Windows is to generalize this ANSI escape
parser and let it be specialized to emit console calls or whatever they
are.

Maybe I'll push this patch in independently after all... not sure yet.

Tom

Patch

diff --git a/gdb/utils.c b/gdb/utils.c
index 7a8c80c64ed..1982fa20e64 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1268,13 +1268,11 @@  static bool pagination_disabled_for_command;
    the end of the line, we spit out a newline, the indent, and then
    the buffered output.  */
 
-/* Malloc'd buffer with chars_per_line+2 bytes.  Contains characters which
-   are waiting to be output (they have already been counted in chars_printed).
-   When wrap_buffer[0] is null, the buffer is empty.  */
-static char *wrap_buffer;
+static bool filter_initalized = false;
 
-/* Pointer in wrap_buffer to the next character to fill.  */
-static char *wrap_pointer;
+/* Contains characters which are waiting to be output (they have
+   already been counted in chars_printed).  */
+static std::string wrap_buffer;
 
 /* String to indent by if the wrap occurs.  Must not be NULL if wrap_column
    is non-zero.  */
@@ -1347,7 +1345,7 @@  init_page_info (void)
 int
 filtered_printing_initialized (void)
 {
-  return wrap_buffer != NULL;
+  return filter_initalized;
 }
 
 set_batch_flag_and_restore_page_info::set_batch_flag_and_restore_page_info ()
@@ -1387,8 +1385,7 @@  set_screen_size (void)
   rl_set_screen_size (rows, cols);
 }
 
-/* Reinitialize WRAP_BUFFER according to the current value of
-   CHARS_PER_LINE.  */
+/* Reinitialize WRAP_BUFFER.  */
 
 static void
 set_width (void)
@@ -1396,14 +1393,8 @@  set_width (void)
   if (chars_per_line == 0)
     init_page_info ();
 
-  if (!wrap_buffer)
-    {
-      wrap_buffer = (char *) xmalloc (chars_per_line + 2);
-      wrap_buffer[0] = '\0';
-    }
-  else
-    wrap_buffer = (char *) xrealloc (wrap_buffer, chars_per_line + 2);
-  wrap_pointer = wrap_buffer;	/* Start it at the beginning.  */
+  wrap_buffer.clear ();
+  filter_initalized = true;
 }
 
 static void
@@ -1546,17 +1537,13 @@  void
 wrap_here (const char *indent)
 {
   /* This should have been allocated, but be paranoid anyway.  */
-  if (!wrap_buffer)
+  if (!filter_initalized)
     internal_error (__FILE__, __LINE__,
 		    _("failed internal consistency check"));
 
-  if (wrap_buffer[0])
-    {
-      *wrap_pointer = '\0';
-      fputs_unfiltered (wrap_buffer, gdb_stdout);
-    }
-  wrap_pointer = wrap_buffer;
-  wrap_buffer[0] = '\0';
+  if (!wrap_buffer.empty ())
+    fputs_unfiltered (wrap_buffer.c_str (), gdb_stdout);
+  wrap_buffer.clear ();
   if (chars_per_line == UINT_MAX)	/* No line overflow checking.  */
     {
       wrap_column = 0;
@@ -1693,7 +1680,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	  if (*lineptr == '\t')
 	    {
 	      if (wrap_column)
-		*wrap_pointer++ = '\t';
+		wrap_buffer.push_back ('\t');
 	      else
 		fputc_unfiltered ('\t', stream);
 	      /* Shifting right by 3 produces the number of tab stops
@@ -1705,7 +1692,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	  else
 	    {
 	      if (wrap_column)
-		*wrap_pointer++ = *lineptr;
+		wrap_buffer.push_back (*lineptr);
 	      else
 		fputc_unfiltered (*lineptr, stream);
 	      chars_printed++;
@@ -1735,8 +1722,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	      if (wrap_column)
 		{
 		  fputs_unfiltered (wrap_indent, stream);
-		  *wrap_pointer = '\0';	/* Null-terminate saved stuff, */
-		  fputs_unfiltered (wrap_buffer, stream); /* and eject it.  */
+		  fputs_unfiltered (wrap_buffer.c_str (), 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
@@ -1745,8 +1731,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 		     if we are printing a long string.  */
 		  chars_printed = strlen (wrap_indent)
 		    + (save_chars - wrap_column);
-		  wrap_pointer = wrap_buffer;	/* Reset buffer */
-		  wrap_buffer[0] = '\0';
+		  wrap_buffer.clear ();
 		  wrap_column = 0;	/* And disable fancy wrap */
 		}
 	    }