[01/16] Change wrap buffering to use a std::string

Message ID 20181128001435.12703-2-tom@tromey.com
State New
Headers show
Series
  • Add styling to the gdb CLI and TUI
Related show

Commit Message

Tom Tromey Nov. 28, 2018, 12:14 a.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.

This also changes utils.c to always emit characters to the wrap
buffer.  This simplifies future patches which emit terminal escape
sequences, and also makes it possible for the "echo" and "printf"
commands to be used to emit terminal escapes and have these work in
the TUI.

gdb/ChangeLog
2018-11-27  Tom Tromey  <tom@tromey.com>

	* utils.c (filter_initalized): New global.
	(wrap_buffer): Now a std::string.
	(wrap_pointer): Remove.
	(flush_wrap_buffer): New function.
	(filtered_printing_initialized, set_width, wrap_here)
	(fputs_maybe_filtered): Update.
---
 gdb/ChangeLog |  9 ++++++++
 gdb/utils.c   | 63 +++++++++++++++++++++------------------------------
 2 files changed, 35 insertions(+), 37 deletions(-)

-- 
2.17.2

Comments

Joel Brobecker Dec. 23, 2018, 3:25 p.m. | #1
Hi Tom,


> 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 also changes utils.c to always emit characters to the wrap

> buffer.  This simplifies future patches which emit terminal escape

> sequences, and also makes it possible for the "echo" and "printf"

> commands to be used to emit terminal escapes and have these work in

> the TUI.

> 

> gdb/ChangeLog

> 2018-11-27  Tom Tromey  <tom@tromey.com>

> 

> 	* utils.c (filter_initalized): New global.

> 	(wrap_buffer): Now a std::string.

> 	(wrap_pointer): Remove.

> 	(flush_wrap_buffer): New function.

> 	(filtered_printing_initialized, set_width, wrap_here)

> 	(fputs_maybe_filtered): Update.


This patch kept me entertained for quite a bit longer than I thought!

I think just the mechanis of replacing the manually-managed buffer
with a string is a win in and of itself. The question your patch
raised as a side-effect is whether we still need the boolean indicating
whether the wrap buffer has been initialized or not (filter_initalized).
I tried to explore that question, hence the unexpected entertainment,
and the answer is still not clear to me, but I *think* that we should
be able to. It really depends what the semantics of
filtered_printing_initialized is, and it looks like it might be used for
two purposes, rather than one. Eg: one meaning is the wrap_buffer is
indeed iniatilized and therefore can be used; but it looks like the
other use is to determine whether buffering is done at all.

Anyways, this is orthogonal to your patch, so I will stop rambling now.

Overall, I think it is a good patch. I have one comment and one
question.


> ---

>  gdb/ChangeLog |  9 ++++++++

>  gdb/utils.c   | 63 +++++++++++++++++++++------------------------------

>  2 files changed, 35 insertions(+), 37 deletions(-)

> 

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

> index 0577e64ea8..0f1953a66d 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;


Small typo in the name of that static global "filter_initalized" ->
"filter_initialized" :-)

> @@ -1669,6 +1666,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,

>        || top_level_interpreter () == NULL

>        || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())

>      {

> +      flush_wrap_buffer (stream);

>        fputs_unfiltered (linebuffer, stream);

>        return;

>      }


I understand why this is needed today; but I can't determine
whether this is needed as a consequence of your patch, or if this
was a hole in the previous code. Can you explain what made you
make this change?

Thank you,
-- 
Joel
Tom Tromey Dec. 28, 2018, 6:47 p.m. | #2
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:


Joel> The question your patch
Joel> raised as a side-effect is whether we still need the boolean indicating
Joel> whether the wrap buffer has been initialized or not (filter_initalized).
Joel> I tried to explore that question, hence the unexpected entertainment,
Joel> and the answer is still not clear to me, but I *think* that we should
Joel> be able to. It really depends what the semantics of
Joel> filtered_printing_initialized is, and it looks like it might be used for
Joel> two purposes, rather than one. Eg: one meaning is the wrap_buffer is
Joel> indeed iniatilized and therefore can be used; but it looks like the
Joel> other use is to determine whether buffering is done at all.

This is a good point.  I've put a note in my to-do list to remind me to
see if it can be removed.

Joel> Small typo in the name of that static global "filter_initalized" ->
Joel> "filter_initialized" :-)

Thanks, I fixed this.

>> @@ -1669,6 +1666,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,

>> || top_level_interpreter () == NULL

>> || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())

>> {

>> +      flush_wrap_buffer (stream);

>> fputs_unfiltered (linebuffer, stream);

>> return;

>> }


Joel> I understand why this is needed today; but I can't determine
Joel> whether this is needed as a consequence of your patch, or if this
Joel> was a hole in the previous code. Can you explain what made you
Joel> make this change?

It's probably not needed in this particular patch, but eventually style
changes are just written to the buffer; so to get styles to apply
properly when filtering is not in use, the buffer has to be flushed
here.

Tom

Patch

diff --git a/gdb/utils.c b/gdb/utils.c
index 0577e64ea8..0f1953a66d 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
@@ -1521,6 +1512,18 @@  reinitialize_more_filter (void)
   pagination_disabled_for_command = false;
 }
 
+/* Flush the wrap buffer to STREAM, if necessary.  */
+
+static void
+flush_wrap_buffer (struct ui_file *stream)
+{
+  if (!wrap_buffer.empty ())
+    {
+      fputs_unfiltered (wrap_buffer.c_str (), stream);
+      wrap_buffer.clear ();
+    }
+}
+
 /* Indicate that if the next sequence of characters overflows the line,
    a newline should be inserted here rather than when it hits the end.
    If INDENT is non-null, it is a string to be printed to indent the
@@ -1546,17 +1549,11 @@  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';
+  flush_wrap_buffer (gdb_stdout);
   if (chars_per_line == UINT_MAX)	/* No line overflow checking.  */
     {
       wrap_column = 0;
@@ -1669,6 +1666,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
       || top_level_interpreter () == NULL
       || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
     {
+      flush_wrap_buffer (stream);
       fputs_unfiltered (linebuffer, stream);
       return;
     }
@@ -1692,10 +1690,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	  /* Print a single line.  */
 	  if (*lineptr == '\t')
 	    {
-	      if (wrap_column)
-		*wrap_pointer++ = '\t';
-	      else
-		fputc_unfiltered ('\t', stream);
+	      wrap_buffer.push_back ('\t');
 	      /* Shifting right by 3 produces the number of tab stops
 	         we have already passed, and then adding one and
 	         shifting left 3 advances to the next tab stop.  */
@@ -1704,10 +1699,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	    }
 	  else
 	    {
-	      if (wrap_column)
-		*wrap_pointer++ = *lineptr;
-	      else
-		fputc_unfiltered (*lineptr, stream);
+	      wrap_buffer.push_back (*lineptr);
 	      chars_printed++;
 	      lineptr++;
 	    }
@@ -1735,8 +1727,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.  */
+		  flush_wrap_buffer (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 +1736,6 @@  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_column = 0;	/* And disable fancy wrap */
 		}
 	    }