[pushed] gdbserver/Linux: unbreak thread event randomization (Re: [pushed] native/Linux: internal error if resume is short-circuited)

[Pedro Alves <palves@redhat.com>]

On 03/19/2015 12:36 PM, Pedro Alves wrote:

> So I thought some more about this and realized that it's
> possible to construct a test case that triggers the assertion,
> even without the special-case of a process that disappears.  So I
> wrote that test, rewrote the git commit log in that direction,
> and pushed it in, as below.

When I first wrote that patch, I added an extra assert that
surprisingly failed against gdbserver.  It caught a bug straight
from the OMG-can't-believe-this-wasnt-caught-before department...

I've pushed in the gdbserver fix and the extra test assert, as below.

I've made the test dump the counters to the logs, so I can keep an
eye on the buildbots, to make sure the test isn't at real risk of
being racy in practice.

---
From 8bf3b159e55b42bb084f9da1af400a285025618f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 15 Mar 2015 19:35:26 +0000
Subject: [PATCH] gdbserver/Linux: unbreak thread event randomization

Wanting to make sure the new continue-pending-status.exp test tests
both cases of threads 2 and 3 reporting an event, I added counters to
the test, to make it FAIL if events for both threads aren't seen.
Assuming a well behaved backend, and given a reasonable number of
iterations, it should PASS.

However, running that against GNU/Linux gdbserver, I found that
surprisingly, that FAILed.  GDBserver always reported the breakpoint
hit for the same thread.

Turns out that I broke gdbserver's thread event randomization
recently, with git commit 582511be ([gdbserver] linux-low.c: better
starvation avoidance, handle non-stop mode too).  In that commit I
missed that the thread structure also has a status_pending_p field...
The end result was that count_events_callback always returns 0, and
then if no thread is stepping, select_event_lwp always returns the
event thread.  IOW, no randomization is happening at all.  Quite
curious how all the other changes in that patch were sufficient to fix
non-stop-fair-events.exp anyway even with that broken.

Tested on x86_64 Fedora 20, native and gdbserver.

gdb/gdbserver/ChangeLog:
2015-03-19 Pedro Alves  <palves@redhat.com>

	* linux-low.c (count_events_callback, select_event_lwp_callback):
	Use the lwp's status_pending_p field, not the thread's.

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

	* gdb.threads/continue-pending-status.exp (saw_thread_2)
	(saw_thread_3): New globals.
	(top level): Increment them when an event for the corresponding
	thread is seen.
	(no thread starvation): New test.
---
 gdb/ChangeLog                                         |  5 +++++
 gdb/gdbserver/ChangeLog                               |  5 +++++
 gdb/testsuite/ChangeLog                               |  8 ++++++++
 gdb/gdbserver/linux-low.c                             |  7 +++++--
 gdb/linux-nat.c                                       |  1 +
 gdb/testsuite/gdb.threads/continue-pending-status.exp | 14 ++++++++++++++
 6 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7ae3c58..cfbd576 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-03-19 Pedro Alves  <palves@redhat.com>
+
+	* linux-low.c (count_events_callback, select_event_lwp_callback):
+	Use the lwp's status_pending_p field, not the thread's.
+
 2015-03-19  Pedro Alves  <palves@redhat.com>
 
 	* linux-nat.c (status_callback): Return early if the LWP has no
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 6efab5b..df8f9ab 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2015-03-19 Pedro Alves  <palves@redhat.com>
+
+	* linux-low.c (count_events_callback, select_event_lwp_callback):
+	Use the lwp's status_pending_p field, not the thread's.
+
 2015-03-19  Pedro Alves  <palves@redhat.com>
 
 	* linux-low.c (select_event_lwp_callback): Update comments to
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 94dae82..6bf008a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,13 @@
 2015-03-19  Pedro Alves  <palves@redhat.com>
 
+	* gdb.threads/continue-pending-status.exp (saw_thread_2)
+	(saw_thread_3): New globals.
+	(top level): Increment them when an event for the corresponding
+	thread is seen.
+	(no thread starvation): New test.
+
+2015-03-19  Pedro Alves  <palves@redhat.com>
+
 	* gdb.threads/continue-pending-status.c: New file.
 	* gdb.threads/continue-pending-status.exp: New file.
 
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 9558f46..e53e0fc 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2238,6 +2238,7 @@ static int
 count_events_callback (struct inferior_list_entry *entry, void *data)
 {
   struct thread_info *thread = (struct thread_info *) entry;
+  struct lwp_info *lp = get_thread_lwp (thread);
   int *count = data;
 
   gdb_assert (count != NULL);
@@ -2245,7 +2246,7 @@ count_events_callback (struct inferior_list_entry *entry, void *data)
   /* Count only resumed LWPs that have an event pending. */
   if (thread->last_status.kind == TARGET_WAITKIND_IGNORE
       && thread->last_resume_kind != resume_stop
-      && thread->status_pending_p)
+      && lp->status_pending_p)
     (*count)++;
 
   return 0;
@@ -2273,6 +2274,7 @@ static int
 select_event_lwp_callback (struct inferior_list_entry *entry, void *data)
 {
   struct thread_info *thread = (struct thread_info *) entry;
+  struct lwp_info *lp = get_thread_lwp (thread);
   int *selector = data;
 
   gdb_assert (selector != NULL);
@@ -2280,7 +2282,7 @@ select_event_lwp_callback (struct inferior_list_entry *entry, void *data)
   /* Select only resumed LWPs that have an event pending. */
   if (thread->last_resume_kind != resume_stop
       && thread->last_status.kind == TARGET_WAITKIND_IGNORE
-      && thread->status_pending_p)
+      && lp->status_pending_p)
     if ((*selector)-- == 0)
       return 1;
 
@@ -2324,6 +2326,7 @@ select_event_lwp (struct lwp_info **orig_lp)
 
       /* First see how many events we have.  */
       find_inferior (&all_threads, count_events_callback, &num_events);
+      gdb_assert (num_events > 0);
 
       /* Now randomly pick a LWP out of those that have had
 	 events.  */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index f5f92d9..40f1e1f 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2841,6 +2841,7 @@ select_event_lwp (ptid_t filter, struct lwp_info **orig_lp, int *status)
 
       /* First see how many events we have.  */
       iterate_over_lwps (filter, count_events_callback, &num_events);
+      gdb_assert (num_events > 0);
 
       /* Now randomly pick a LWP out of those that have had
 	 events.  */
diff --git a/gdb/testsuite/gdb.threads/continue-pending-status.exp b/gdb/testsuite/gdb.threads/continue-pending-status.exp
index ff73ce4..1f170f7 100644
--- a/gdb/testsuite/gdb.threads/continue-pending-status.exp
+++ b/gdb/testsuite/gdb.threads/continue-pending-status.exp
@@ -56,6 +56,13 @@ proc get_current_thread {} {
 
 set attempts 20
 
+# These track whether we saw events for both threads 2 and 3.  If the
+# backend always returns the breakpoint hit for the same thread, then
+# it fails to make sure threads aren't starved, and we'll fail the
+# assert after the loop.
+set saw_thread_2 0
+set saw_thread_3 0
+
 for {set i 0} {$i < $attempts} {incr i} {
     with_test_prefix "attempt $i" {
 	gdb_test "b $srcfile:$break_line" \
@@ -71,8 +78,10 @@ for {set i 0} {$i < $attempts} {incr i} {
 	# the resume and go straight to consuming the pending event.
 	set thread [get_current_thread]
 	if {$thread == 2} {
+	    incr saw_thread_2
 	    set thread 3
 	} else {
+	    incr saw_thread_3
 	    set thread 2
 	}
 	gdb_test "thread $thread" \
@@ -108,3 +117,8 @@ for {set i 0} {$i < $attempts} {incr i} {
 	}
     }
 }
+
+verbose -log "saw_thread_2=$saw_thread_2"
+verbose -log "saw_thread_3=$saw_thread_3"
+
+gdb_assert {$saw_thread_2 > 0 && $saw_thread_3 > 0} "no thread starvation"
-- 
1.9.3