[PATCH] gdb: Don't flush line wrap buffer before adding newline

[PATCH] gdb: Don't flush line wrap buffer before adding newline

[Andrew Burgess]

A bug was introduced in commit:

    commit cbe5657196d0d3acbeca39973f93f333ecedacda
    Date:   Mon Sep 3 22:56:33 2018 -0600

        Add output styles to gdb

As GDB produces output, the output is stored in a temporary buffer.
In this way GDB can spot when a line wrap occurs, and can insert a
newline before flushing the contents of the temporary buffer out.

The bug was that the temporary buffer was being flushed out _before_
the newline was added to the output stream, this resulted in incorrect
line wrapping.  Here is an example from how GDB announces that a
breakpoint has been hit, first with the bug:

Breakpoint 1, function () at /a/long/pa
    th/to/the/source/file.c:123

Now with the bug fixed (this matches the behaviour before the
offending commit):

Breakpoint 1, function ()
    at /a/long/path/to/the/source/file.c:123

There are some other issues with line wrapping in current HEAD of GDB,
but these are caused by an unrelated bug.

gdb/ChangeLog:

	* utils.c (fputs_maybe_filtered): Don't flush the line wrap buffer
	before inserting a newline for line-wrapping.

gdb/testsuite/ChangeLog:

	* gdb.base/line-wrapping.c: New file.
	* gdb.base/line-wrapping.exp: New file.
---
 gdb/ChangeLog                            |  5 ++++
 gdb/testsuite/ChangeLog                  |  5 ++++
 gdb/testsuite/gdb.base/line-wrapping.c   | 28 ++++++++++++++++++++++
 gdb/testsuite/gdb.base/line-wrapping.exp | 41 ++++++++++++++++++++++++++++++++
 gdb/utils.c                              |  1 -
 5 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/line-wrapping.c
 create mode 100644 gdb/testsuite/gdb.base/line-wrapping.exp

diff --git a/gdb/testsuite/gdb.base/line-wrapping.c b/gdb/testsuite/gdb.base/line-wrapping.c
new file mode 100644
index 00000000000..5fdb4462e65
--- /dev/null
+++ b/gdb/testsuite/gdb.base/line-wrapping.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+some_rather_long_function_name ()
+{
+  return 0;
+}
+
+int
+main ()
+{
+  return some_rather_long_function_name ();
+}
diff --git a/gdb/testsuite/gdb.base/line-wrapping.exp b/gdb/testsuite/gdb.base/line-wrapping.exp
new file mode 100644
index 00000000000..94b8bc6ea13
--- /dev/null
+++ b/gdb/testsuite/gdb.base/line-wrapping.exp
@@ -0,0 +1,41 @@
+# Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This tests some line-wrapping cases in GDB.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+gdb_test_no_output "set width 55"
+
+if ![runto_main] then {
+    fail "can't run to main"
+    return 0
+}
+
+# Use GDB_TEST here not GDB_CONTINUE_TO_BREAKPOINT as we want to check
+# specifically that the lines are broken at the correct place, and the
+# correct whitespace is added before the "at".
+gdb_breakpoint "some_rather_long_function_name"
+gdb_test "continue" \
+    [multi_line \
+	 "Breakpoint 2, some_rather_long_function_name \\(\\)" \
+	 "    at \[^\r\n\]+$srcfile:$decimal" \
+	 "$decimal\[^\r\n\]+"] \
+    "line wrapping at a breakpoint"
+
diff --git a/gdb/utils.c b/gdb/utils.c
index 6fb5736abb5..7bcc4b97b7c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1788,7 +1788,6 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	      if (wrap_column)
 		{
 		  emit_style_escape (ui_file_style ());
-		  flush_wrap_buffer (stream);
 		  fputc_unfiltered ('\n', stream);
 		}
 
-- 
2.14.5

Re: [PATCH] gdb: Don't flush line wrap buffer before adding newline

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> 	* utils.c (fputs_maybe_filtered): Don't flush the line wrap buffer
Andrew> 	before inserting a newline for line-wrapping.

Thanks, this is ok.  I actually have the same patch and a very similar
test on my branch to fix up the pager when styles are enabled.  That
branch has a few more fixes, too; but it doesn't hurt to land this one
now.

Tom

Re: [PATCH] gdb: Don't flush line wrap buffer before adding newline

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2019-02-05 08:27:29 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> 	* utils.c (fputs_maybe_filtered): Don't flush the line wrap buffer
> Andrew> 	before inserting a newline for line-wrapping.
> 
> Thanks, this is ok.

Really, I was just teasing - this patch is rubbish :-/

(Not really, I spotted my mistake immediately after hitting send.)

The code used to look like this:

    if (wrap_column)
    {
      emit_style_escape (ui_file_style ());
      flush_wrap_buffer (stream);
      fputc_unfiltered ('\n', stream);
    }

The problem is that 'emit_style_escape' sends the escape string to the
wrap_buffer, while the newline is sent directly to the console.

This means that GDB is still styled when the newline is sent, which I
think means that if we hit a pager prompt this could be styled.

I did consider having a second version of 'emit_style_escape' that
by-passed the wrap_buffer, but I worried that this might be too open
to incorrect use.  Instead I thought a better approach would be backup
and clear the wrap_buffer before calling 'emit_style_escape' in the
above code, the 'flush_wrap_buffer' can then be used safely.

The patch below shows a new proposal for utils.c.

Do you agree with the issues I see above?

What do you think of the new approach?

Thanks,
Andrew


---

diff --git a/gdb/utils.c b/gdb/utils.c
index 6fb5736abb5..153de6cd0f8 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1787,9 +1787,12 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	         anyway; if it's wrong, let us keep going.  */
 	      if (wrap_column)
 		{
+		  std::string tmp = wrap_buffer;
+		  wrap_buffer.clear ();
 		  emit_style_escape (ui_file_style ());
 		  flush_wrap_buffer (stream);
 		  fputc_unfiltered ('\n', stream);
+		  wrap_buffer = tmp;
 		}
 
 	      /* Possible new page.  Note that

Re: [PATCH] gdb: Don't flush line wrap buffer before adding newline

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2019-02-05 09:11:52 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> The patch below shows a new proposal for utils.c.
> Andrew> Do you agree with the issues I see above?
> Andrew> What do you think of the new approach?
> 
> Could you try my branch?  It fixes this problem and a few more
> besides.

Yes, this fixes the original issue, and a second issue that I had an
almost complete patch for.

I'll wait for your fixes to land.

Thanks,
Andrew


> I am in the process -- kind of slow due to travel etc -- of writing the
> last couple of test cases to make sure it is all correct... Philippe
> found a number of bugs in this area a while back and the branch attempts
> to fix them all.
> 
> It's the branch submit/fix-pager on my github.
> It's in an ugly form right now (I didn't squash the patches yet), but
> that shouldn't matter for trying it.
> 
> Tom

Re: [PATCH] gdb: Don't flush line wrap buffer before adding newline

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> The patch below shows a new proposal for utils.c.
Andrew> Do you agree with the issues I see above?
Andrew> What do you think of the new approach?

Could you try my branch?  It fixes this problem and a few more besides.
I am in the process -- kind of slow due to travel etc -- of writing the
last couple of test cases to make sure it is all correct... Philippe
found a number of bugs in this area a while back and the branch attempts
to fix them all.

It's the branch submit/fix-pager on my github.
It's in an ugly form right now (I didn't squash the patches yet), but
that shouldn't matter for trying it.

Tom