[PATCH] Fix inferior deadlock with "target remote | CMD"

[PATCH] Fix inferior deadlock with "target remote | CMD"

[Pedro Alves]

Comparing test results between

  --target_board=native-gdbserver
  --target_board=native-stdio-gdbserver

I noticed that gdb.base/bigcore.exp is failing with native-stdio-gdbserver:

  Running src/gdb/testsuite/gdb.base/bigcore.exp ...
  FAIL: gdb.base/bigcore.exp: continue (timeout)
  ...

The problem is that:

  1. When debugging with "target remote | CMD", the inferior's
     stdout/stderr streams are connected to a pipe.

  2. The bigcore.c program prints a lot to the screen before it
     reaches the breakpoint location that the "continue" shown above
     wants to reach.

  3. GDB is not flushing the inferior's output pipe while the inferior
     is running.

  4. The pipe becomes full.

  5. The inferior thus deadlocks.

The bug is #3 above, which is what this commit fixes.  A new test is
added, that specifically exercises this scenario.  The test fails
before the fix, and passes after, and gdb.base/bigcore.exp also starts
passing.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* ser-base.c (ser_base_read_error_fd): Delete the file handler if
	async.
	(handle_error_fd): New function.
	(ser_base_async): Add/delete an event loop file handler for
	error_fd.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/long-inferior-output.c: New file.
	* long-inferior-output.exp: New file.
---
 gdb/ser-base.c                                  | 20 +++++++
 gdb/testsuite/gdb.base/long-inferior-output.c   | 38 +++++++++++++
 gdb/testsuite/gdb.base/long-inferior-output.exp | 75 +++++++++++++++++++++++++
 3 files changed, 133 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/long-inferior-output.c
 create mode 100644 gdb/testsuite/gdb.base/long-inferior-output.exp

diff --git a/gdb/ser-base.c b/gdb/ser-base.c
index 790cb1b..6c895ef 100644
--- a/gdb/ser-base.c
+++ b/gdb/ser-base.c
@@ -288,6 +288,8 @@ ser_base_read_error_fd (struct serial *scb, int close_fd)
 	  if (s == 0 && close_fd)
 	    {
 	      /* End of file.  */
+	      if (serial_is_async_p (scb))
+		delete_file_handler (scb->error_fd);
 	      close (scb->error_fd);
 	      scb->error_fd = -1;
 	      break;
@@ -313,6 +315,17 @@ ser_base_read_error_fd (struct serial *scb, int close_fd)
     }
 }
 
+/* Event-loop callback for a serial's error_fd.  Flushes any error
+   output we might have.  */
+
+static void
+handle_error_fd (int error, gdb_client_data client_data)
+{
+  serial *scb = (serial *) client_data;
+
+  ser_base_read_error_fd (scb, 0);
+}
+
 /* Read a character with user-specified timeout.  TIMEOUT is number of
    seconds to wait, or -1 to wait forever.  Use timeout of 0 to effect
    a poll.  Returns char if successful.  Returns SERIAL_TIMEOUT if
@@ -589,6 +602,10 @@ ser_base_async (struct serial *scb,
 	fprintf_unfiltered (gdb_stdlog, "[fd%d->asynchronous]\n",
 			    scb->fd);
       reschedule (scb);
+
+      if (scb->error_fd != -1)
+	add_file_handler (scb->error_fd, handle_error_fd, scb);
+
     }
   else
     {
@@ -607,5 +624,8 @@ ser_base_async (struct serial *scb,
 	  delete_timer (scb->async_state);
 	  break;
 	}
+
+      if (scb->error_fd != -1)
+	delete_file_handler (scb->error_fd);
     }
 }
diff --git a/gdb/testsuite/gdb.base/long-inferior-output.c b/gdb/testsuite/gdb.base/long-inferior-output.c
new file mode 100644
index 0000000..eb3d5c5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/long-inferior-output.c
@@ -0,0 +1,38 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2017 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/>.  */
+
+#include <stdio.h>
+
+static size_t total_bytes = 0;
+
+int
+main ()
+{
+  int i = 0;
+
+  /* When testing with "target remote |", stdout is a pipe, and thus
+     block buffered by default.  Force it to be line buffered.  */
+  setvbuf (stdout, NULL, _IOLBF, 0);
+
+  /* This outputs > 70 KB which is larger than the default pipe buffer
+     size on most systems (typically 16 KB or 64 KB).  */
+  for (i = 0; i < 3000; i++)
+    total_bytes += printf ("this is line number %d\n", i);
+
+  printf ("total bytes written = %u\n", (unsigned) total_bytes);
+  return 0; /* printing done */
+}
diff --git a/gdb/testsuite/gdb.base/long-inferior-output.exp b/gdb/testsuite/gdb.base/long-inferior-output.exp
new file mode 100644
index 0000000..ae9386c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/long-inferior-output.exp
@@ -0,0 +1,75 @@
+# Copyright 2017 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/>.
+
+# When debugging with "target remote |", the inferior's output is
+# connected to a pipe, and if GDB doesn't flush the pipe while the
+# inferior is running and the pipe becomes full, then the inferior
+# deadlocks:
+#
+#  1. User sets breakpoint, and types "continue"
+#
+#  2. Inferior prints to stdout/stderr before reaching breakpoint
+#     location.
+#
+#  3. The output pipe becomes full, so the inferior blocks forever in
+#     the printf/write call.
+#
+#  4. The breakpoint is never reached.
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping because of noinferiorio."
+    return
+}
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare" $testfile {} {debug}] {
+    return -1
+}
+
+if { ![runto_main] } then {
+    fail "run to main"
+    return
+}
+
+set printing_done_line [gdb_get_line_number "printing done"]
+gdb_test "break $printing_done_line" ".*" "set breakpoint after printing"
+
+send_gdb "continue\n"
+
+set expected_lines 3000
+set more 1
+set i 0
+while {$more} {
+    set more 0
+    gdb_expect {
+	-i $inferior_spawn_id
+	-ex "this is line number $i" {
+	    incr i
+           if {$i != $expected_lines} {
+		set more 1
+	    }
+	}
+    }
+}
+
+gdb_assert {$i == $expected_lines} "saw all lines"
+
+set test "breakpoint reached"
+gdb_test_multiple "" $test {
+    -re "Breakpoint .* main .*$srcfile:$printing_done_line.*$gdb_prompt $" {
+	pass $test
+    }
+}
-- 
2.5.5

Re: [PATCH] Fix inferior deadlock with "target remote | CMD"

[John Baldwin]

On Monday, October 16, 2017 08:44:41 PM Pedro Alves wrote:
> Comparing test results between
> 
>   --target_board=native-gdbserver
>   --target_board=native-stdio-gdbserver
> 
> I noticed that gdb.base/bigcore.exp is failing with native-stdio-gdbserver:
> 
>   Running src/gdb/testsuite/gdb.base/bigcore.exp ...
>   FAIL: gdb.base/bigcore.exp: continue (timeout)
>   ...
> 
> The problem is that:
> 
>   1. When debugging with "target remote | CMD", the inferior's
>      stdout/stderr streams are connected to a pipe.
> 
>   2. The bigcore.c program prints a lot to the screen before it
>      reaches the breakpoint location that the "continue" shown above
>      wants to reach.
> 
>   3. GDB is not flushing the inferior's output pipe while the inferior
>      is running.
> 
>   4. The pipe becomes full.
> 
>   5. The inferior thus deadlocks.
> 
> The bug is #3 above, which is what this commit fixes.  A new test is
> added, that specifically exercises this scenario.  The test fails
> before the fix, and passes after, and gdb.base/bigcore.exp also starts
> passing.

Seems sensible.  It wasn't obvious to me from the code how stdout is
treated as compared to stderr, so I'm less confident on reviewing the
implementation.  One nit:

> @@ -589,6 +602,10 @@ ser_base_async (struct serial *scb,
>  	fprintf_unfiltered (gdb_stdlog, "[fd%d->asynchronous]\n",
>  			    scb->fd);
>        reschedule (scb);
> +
> +      if (scb->error_fd != -1)
> +	add_file_handler (scb->error_fd, handle_error_fd, scb);
> +
>      }

Extra blank line at the end?

> +
> +set expected_lines 3000

Pity this can't be a function of PIPE_BUF, but I suspect there's
no good way to do that.

-- 
John Baldwin

Re: [PATCH] Fix inferior deadlock with "target remote | CMD"

[Pedro Alves]

On 10/18/2017 06:38 PM, John Baldwin wrote:
> On Monday, October 16, 2017 08:44:41 PM Pedro Alves wrote:

> Seems sensible.  It wasn't obvious to me from the code how stdout is
> treated as compared to stderr, so I'm less confident on reviewing the
> implementation.  One nit:

Ah, the manual explains it:

~~~
Programs started with stdio-connected gdbserver have @file{/dev/null} for
@code{stdin}, and @code{stdout},@code{stderr} are sent back to gdb for
display through a pipe connected to gdbserver.
Both @code{stdout} and @code{stderr} use the same pipe.
~~~

> 
>> @@ -589,6 +602,10 @@ ser_base_async (struct serial *scb,
>>  	fprintf_unfiltered (gdb_stdlog, "[fd%d->asynchronous]\n",
>>  			    scb->fd);
>>        reschedule (scb);
>> +
>> +      if (scb->error_fd != -1)
>> +	add_file_handler (scb->error_fd, handle_error_fd, scb);
>> +
>>      }
> 
> Extra blank line at the end?

Indeed.  I fixed it.

> 
>> +
>> +set expected_lines 3000
> 
> Pity this can't be a function of PIPE_BUF, but I suspect there's
> no good way to do that.

Yeah, I don't know of a way.  Well, actually the test itself could
maybe create a pipe and try to fill it up, which would be the
right number when native testing.  That seemed more trouble than
it's worth it, though.

Looking around the web a bit more, I found
<https://github.com/afborchert/pipebuf> which confirms that 70KB
"ought to be enough for everybody".

I pushed the patch in now, also with filename filed in the ChangeLog.

Thanks for the review!

Pedro Alves