Linux: make target_is_async_p return false when async is off

[Pedro Alves <palves@redhat.com>]

On 01/22/2015 01:36 PM, Pedro Alves wrote:
> On 01/22/2015 12:29 PM, Metzger, Markus T wrote:
>>> -----Original Message-----
>>> From: Metzger, Markus T
>>> Sent: Tuesday, January 20, 2015 4:08 PM
>>> To: Jan Kratochvil
>>> Cc: palves@redhat.com; gdb-patches@sourceware.org
>>
>>
>>> I can't reproduce this fail; I don't get that far.  This test fails for me with
>>>
>>> 	FAIL: gdb.btrace/multi-thread-step.exp: continue to breakpoint: cont
>>> to multi-thread-step.c:34 (timeout)
>>
>> This fail seems to be caused by 588dcc3edbde19f90e76de969dbfa7ab3e17951a
>> "Consolidate the custom TUI query hook with the default query hook".  It is not
>> related to btrace.
>>
>> The failing test program looks like this:
>>
>>     pthread_barrier_wait (&barrier);
>>     global = 42; /* bp.1 */
>>     pthread_barrier_wait (&barrier);
>>
>> There are two threads, both are at bp.1 between the two barriers.  When I now
>> delete all breakpoints like this:
>>
>>     (gdb) del
>>     Delete all breakpoints? (y or n) y
>>
>> and then continue the inferior, only the current thread is resumed.  The other
>> thread remains at its current location.  The resumed thread waits at the barrier
>> and the test runs into a timeout.
>>
>> Here's a complete debug session:
>>
>>     (gdb) b 30
>>     Breakpoint 1 at 0x400776: file gdb.btrace/multi-thread-step.c, line 30.
>>     (gdb) r
>>     Starting program: gdb.btrace/multi-thread-step 
>>     [Thread debugging using libthread_db enabled]
>>     Using host libthread_db library "/lib64/libthread_db.so.1".
>>     [New Thread 0x7ffff7fce700 (LWP 22156)]
>>
>>     Breakpoint 1, test (arg=0x0) at gdb.btrace/multi-thread-step.c:30
>>     30	  global = 42; /* bp.1 */
>>     (gdb) del
>>     Delete all breakpoints? (y or n) y
>>      (gdb) info thr
>>       Id   Target Id         Frame 
>>       2    Thread 0x7ffff7fce700 (LWP 22156) "multi-thread-st" test (arg=0x0) at gdb.btrace/multi-thread-step.c:30
>>     * 1    Thread 0x7ffff7fcf740 (LWP 22152) "multi-thread-st" test (arg=0x0) at gdb.btrace/multi-thread-step.c:30
>>     (gdb) c
>>     Continuing.
>>     ^C
>>     Program received signal SIGINT, Interrupt.
>>     0x000000384380c20c in pthread_barrier_wait () from /lib64/libpthread.so.0
>>     (gdb) info thr
>>       Id   Target Id         Frame 
>>       2    Thread 0x7ffff7fce700 (LWP 22156) "multi-thread-st" test (arg=0x0) at gdb.btrace/multi-thread-step.c:30
>>     * 1    Thread 0x7ffff7fcf740 (LWP 22152) "multi-thread-st" 0x000000384380c20c in pthread_barrier_wait () from     /lib64/libpthread.so.0
>>
>> When I set debug infrun, I get the this:
>>
>>     (gdb) del
>>     Delete all breakpoints? (y or n) y
>>      (gdb)
>>     infrun: target_wait (-1, status) =
>>     infrun:   -1 [process -1],
>>     infrun:   status->kind = no-resumed
>>     infrun: TARGET_WAITKIND_NO_RESUMED (ignoring)
>>     infrun: prepare_to_wait
>>
>> I don't see this with the old query behaviour or when I remove breakpoints like this
>>
>>     (gdb) del 1
> 
> Hmm, gdb_readline_wrapper believes the target was async to begin
> with.  That seems to be an issue with linux_nat_is_async_p.  And
> then, gdb_readline_wrapper_cleanup sets the target async again,
> which triggers the target_wait call.  It's normal that only
> one thread is resumed, because the other thread has an event
> pending already.  Normally that works because at the very end of
> linux_nat_resume, we'll re-enable async, which, if we were
> sync before, tells the event loop to poll them.  But in this
> case, we're reaching linux_nat_resume already async, do nothing
> wakes up the event loop, and so the pending event is never
> collected and handled by infrun.
> 
> Let me see if I can come up with a fix.

Here it is.  I added a new test based on gdb.btrace/multi-thread-step.c
that does not depend on btrace.  My machine doesn't do btrace, so I can't
check that one.

------------
From: Pedro Alves <palves@redhat.com>
Subject: [PATCH] Linux: make target_is_async_p return false when async is off

linux_nat_is_async_p currently always returns true, even when the
target is _not_ async.  That confuses
gdb_readline_wrapper/gdb_readline_wrapper_cleanup, which
force-disables target-async while the secondary prompt is active.  As
a result, when gdb_readline_wrapper returns, the target is left async,
even through it was sync to begin with.

That can result in weird bugs, like the one the test added by this
commit exposes.

Ref: https://sourceware.org/ml/gdb-patches/2015-01/msg00592.html

gdb/ChangeLog:
2015-01-22  Pedro Alves  <palves@redhat.com>

	* linux-nat.c (linux_is_async_p): New macro.
	(linux_nat_is_async_p):
	(linux_nat_terminal_inferior): Check whether the target can async
	instead of whether it is already async.
	(linux_nat_terminal_ours): Don't check whether the target is
	async.
	(linux_async_pipe): Use linux_is_async_p.

gdb/testsuite/ChangeLog:
2015-01-22  Pedro Alves  <palves@redhat.com>

	* gdb.threads/continue-pending-after-query.c: New file.
	* gdb.threads/continue-pending-after-query.exp: New file.
---
 gdb/linux-nat.c                                    | 23 +++---
 .../gdb.threads/continue-pending-after-query.c     | 48 ++++++++++++
 .../gdb.threads/continue-pending-after-query.exp   | 90 ++++++++++++++++++++++
 3 files changed, 148 insertions(+), 13 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/continue-pending-after-query.c
 create mode 100644 gdb/testsuite/gdb.threads/continue-pending-after-query.exp

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index be52470..b49cd57 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -219,6 +219,9 @@ struct simple_pid_list *stopped_pids;
    event loop.  */
 static int linux_nat_event_pipe[2] = { -1, -1 };

+/* True if we're currently in async mode.  */
+#define linux_is_async_p() (linux_nat_event_pipe[0] != -1)
+
 /* Flush the event pipe.  */

 static void
@@ -4302,10 +4305,7 @@ linux_trad_target (CORE_ADDR (*register_u_offset)(struct gdbarch *, int, int))
 static int
 linux_nat_is_async_p (struct target_ops *ops)
 {
-  /* NOTE: palves 2008-03-21: We're only async when the user requests
-     it explicitly with the "set target-async" command.
-     Someday, linux will always be async.  */
-  return target_async_permitted;
+  return linux_is_async_p ();
 }

 /* target_can_async_p implementation.  */
@@ -4355,7 +4355,11 @@ static int async_terminal_is_ours = 1;
 static void
 linux_nat_terminal_inferior (struct target_ops *self)
 {
-  if (!target_is_async_p ())
+  /* Like target_terminal_inferior, use target_can_async_p, not
+     target_is_async_p, since at this point the target is not async
+     yet.  If it can async, then we know it will become async prior to
+     resume.  */
+  if (!target_can_async_p ())
     {
       /* Async mode is disabled.  */
       child_terminal_inferior (self);
@@ -4385,13 +4389,6 @@ linux_nat_terminal_inferior (struct target_ops *self)
 static void
 linux_nat_terminal_ours (struct target_ops *self)
 {
-  if (!target_is_async_p ())
-    {
-      /* Async mode is disabled.  */
-      child_terminal_ours (self);
-      return;
-    }
-
   /* GDB should never give the terminal to the inferior if the
      inferior is running in the background (run&, continue&, etc.),
      but claiming it sure should.  */
@@ -4444,7 +4441,7 @@ handle_target_event (int error, gdb_client_data client_data)
 static int
 linux_async_pipe (int enable)
 {
-  int previous = (linux_nat_event_pipe[0] != -1);
+  int previous = linux_is_async_p ();

   if (previous != enable)
     {
diff --git a/gdb/testsuite/gdb.threads/continue-pending-after-query.c b/gdb/testsuite/gdb.threads/continue-pending-after-query.c
new file mode 100644
index 0000000..9510ce8
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/continue-pending-after-query.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013-2015 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 <pthread.h>
+
+static int global;
+
+static void
+break_function (void)
+{
+  global = 42; /* set break here */
+}
+
+static void *
+thread_function (void *arg)
+{
+  break_function ();
+
+  return arg;
+}
+
+int
+main (void)
+{
+  pthread_t th;
+
+  pthread_create (&th, NULL, thread_function, NULL);
+
+  break_function ();
+
+  pthread_join (th, NULL);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/continue-pending-after-query.exp b/gdb/testsuite/gdb.threads/continue-pending-after-query.exp
new file mode 100644
index 0000000..d4d50c9
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/continue-pending-after-query.exp
@@ -0,0 +1,90 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013-2015 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/>.
+
+# Regression test for a bug that would go like this:
+#
+# - Run to a breakpoint that is hit by two threads (A and B)
+#   simultaneously.
+#
+# - One of the breakpoint hits is processed (e.g., thread A) and
+#   causes a user-visible stop.  The other (thread B) is left pending.
+#
+# - The user deletes the breakpoint with "del", which causes a
+#   confirmation query.
+#
+# - By mistake, that would result in the target being left with async
+#   enabled, even though it wasn't to begin with.
+#
+# - GDB reacts to target async enablement by polling for target
+#   events.  As no thread is resumed the target replies
+#   TARGET_WAITKIND_NO_RESUMED.
+#
+# - The user continues the program, expecting it to exit.  The thread
+#   that has an event pending (thread B) is not really resumed.
+#
+# - But, nothing signals the event loop that there's a pending event
+#   waiting to be collected for thread B, so that event is never
+#   processed, thread B is never resumed and the program never exits.
+#
+# Ref: https://sourceware.org/ml/gdb-patches/2015-01/msg00592.html
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] == -1} {
+    return -1
+}
+
+proc test {} {
+    global srcfile gdb_prompt
+
+    if ![runto_main] {
+	return -1
+    }
+
+    delete_breakpoints
+
+    set bp_line [gdb_get_line_number "set break here" $srcfile]
+
+    gdb_breakpoint "break_function"
+    gdb_continue_to_breakpoint "cont to break_function" ".*$srcfile:$bp_line\r\n.*"
+
+    # Do something that causes a query/secondary prompt.
+
+    set test "delete breakpoints, answer prompt"
+    set saw_prompt 0
+    gdb_test_multiple "delete breakpoints" $test {
+	-re "Delete all breakpoints.*y or n.*$" {
+	    set saw_prompt 1
+	    send_gdb "y\n"
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	    gdb_assert $saw_prompt $test
+	}
+    }
+
+    gdb_continue_to_end "" "continue" 1
+}
+
+# Test a few times to make sure an event is left pending.  At the time
+# of writing, the bug always triggers, but that might naturally depend
+# on machine.
+for {set i 1} {$i <= 10} {incr i} {
+    with_test_prefix "iter $i" {
+	test
+    }
+}
-- 
1.9.3