Fixes for a couple of infrun bugs (thread hop, revert to step thread).

Pedro Alves
2009-05-27T22:00:19Z

[ Hi Ulrich, I'm CCing you because this touches code you've
touched recently. ]

Given a testcase like this:

void *
thread_function (void *arg) {
pthread_exit (NULL);
}

void hoop_me (void) {}

int
main (int argc, char **argv)
{
pthread_t thread;
pthread_create (&thread, NULL, thread_function, NULL);
pthread_join (thread, NULL);

hop_me (); /* <<< set thread "thread_function" specific breakpoint here */
}

Say you're stopped inside thread_function (in thread 2), and set a
breakpoint at the "hop_me" call, and make it trigger only on
thread 2 (like so: "break hoopme thread 2"). Then, step over
thread 2's exit. GDB is expected to be able to make the main thread
hop over the thread specific breakpoint, but, it gets in
trouble. E.g.,

25 pthread_exit (NULL);
(gdb) b 37 thread 2
Breakpoint 2 at 0x400653: file ../../../src/gdb/testsuite/gdb.threads/thread-exits.c, line 37.
(gdb) n
[Thread 0x40800950 (LWP 27593) exited]
warning: Error removing breakpoint 0
warning: Error removing breakpoint 0
warning: Error removing breakpoint 0
Cannot step over breakpoint hit in wrong thread
(gdb)

The problem here is that we tried to remove memory breakpoints
with inferior_ptid still pointing at the thread that had just
exited. We're already switching context to the event thread a
bit after removing breakpoints, so the fix here is just to
do that *before* removing breakpoints. This is this hunk in
the patch at the bottom:

@@ -2896,6 +2897,11 @@ targets should add new threads to the th
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n");

+ /* Switch context before touching inferior memory, the
+ previous thread may have exited. */
+ if (!ptid_equal (inferior_ptid, ecs->ptid))
+ context_switch (ecs->ptid);
+
/* Saw a breakpoint, but it was hit by the wrong thread.
Just continue. */

@@ -2922,9 +2928,6 @@ targets should add new threads to the th
error (_("Cannot step over breakpoint hit in wrong thread"));
else
{ /* Single step */
- if (!ptid_equal (inferior_ptid, ecs->ptid))
- context_switch (ecs->ptid);
-
if (!non_stop)
{
/* Only need to require the next event from this

The new threxit-hop-specific.exp test exercises this.

((
As I've been saying, we're incrementally reaching a state
where we'll always be able to switch inferior_ptid early and
be done with it... In this particular case, I *think* we could move
it yet a bit higher up, to the beginning of the thread
hop handling without much trouble, but, there's a bit of
software-single-stepping related thread hoping code that makes
me want to do that as a separate change. Note that
if we switch context before thread hoping, in practice means
that we move the thread hoping to after the all mighty line:

/* See if something interesting happened to the non-current thread. If
so, then switch to that thread. */
))

Ok, on to the next issue, and actually, the original issue that
made be see the above. This triggers with stepping over an
execl call, in a threaded app, but the thread exit example
is simpler. The test for this at the end exercises the execl
variant.

Let's try the same pthread_exit example again, but now
with the remove-breakpoints bug fixed. We'll add a little twist.
The code that should be thread-hopped stays in a loop, something
like so:

void *
thread_function (void *arg) {
pthread_exit (NULL);
}

void hoop_me (void) {}

int
main (int argc, char **argv)
{
pthread_t thread;
pthread_create (&thread, NULL, thread_function, NULL);
pthread_join (thread, NULL);

while (1)
+ hop_me (); /* <<< set thread "thread_function" specific breakpoint here */
}

Here's what one sees (line number don't match, but should show the
example OK):

0x0) at ../../../src/gdb/testsuite/gdb.threads/threxit-hop-specific.c:26
26 pthread_exit (NULL);
(gdb) n
During symbol reading, incomplete CFI data; unspecified registers (e.g., rax) at 0x4005cf.
[Thread 0x40800950 (LWP 7559) exited]
Couldn't get registers: No such process.
(gdb)

Again, but with "set debug infrun 1":

infrun: switching back to stepped thread
infrun: Switching context from Thread 0x7ffff7fd66e0 (LWP 7728) to Thread 0x40800950 (LWP 7729)
Couldn't get registers: No such process.
(gdb)

LWP 7729 here was thread 2, which exited. The error happens because
we're trying to resume it, and in the process tried
to read registers from it.

The code in question that tried to resume was this not-so-old
bit of handle_inferior_event:

/* In all-stop mode, if we're currently stepping but have stopped in
some other thread, we need to switch back to the stepped thread. */
if (!non_stop)
{
struct thread_info *tp;
tp = iterate_over_threads (currently_stepping_callback,
ecs->event_thread);
if (tp)
{
...
keep_going (ecs); <<<
return;
}

currently_stepping_callback will not return true for
threads that have a step-resume breakpoint (the thread
was doing a next over a pthread_exit call, so there should be
a step-resume), but, this was tested on native linux,
where we have thread exit events, and, deleting a thread
clears its step-resume breakpoint. But, if the thread was
deleted, how can iterate_over_threads find it? Ah, at the
time it exited, inferior_ptid pointed at it, and, we don't
delete the thread in that case, but instead leave it listed
with state set to "exited". Now, before suggesting that
we should clear enough stepping state of exited threads
so that currently_stepping_callback doesn't find those,
keep in mind targets where we do *not* have thread exit
events. In that case, we'll have to ask the target if
the thread is still alive. But, wait, if there are no
thread exit events, then the original thread should still
have a step-resume breakpoint? Yes, but we could be
single-stepping until the thread exit. Plus, I think
we *do* want to switch back the focus here to the
thread that was nexting.

If we try that against gdbserver, where we don't
have thread exit notifications (there's no such thing
in the remote protocol), GDB doesn't throw any error, but
that's just by luck (bug). Looking closer, we see this:

infrun: stop_pc = 0x400658
infrun: switching back to stepped thread
infrun: Switching context from Thread 32013 to Thread 32105
Sending packet: $Hg7d69#b9...Packet received: E01
Sending packet: $g#67...Packet received: 0000000000000000[... ... ...]

So, the remote target refuses (E01) to select the thread (Hg)
that is gone already, but GDB silently ignores it, and ends up
reading registers from the wrong thread...

We should take advantage of those errors to
realize the thread is gone, but that's for another day.

All this brings me to the patch below, and two new
tests, that should cover these bugs.

Does it look ok? Tested against native x86_64-linux and
against local gdbserver.

--
Pedro Alves

gdb/
2009-05-27 Pedro Alves <pedro@codesourcery.com>

* infrun.c (handle_inferior_event): When thread hoping, switch
inferior_ptid to the event thread before removing breakpoints from
the target. If not stopping, also try to revert back to a thread
that was doing a "next". Check if that thread still exists before
resuming.
(currently_stepping_thread): Delete and merge with ...
(currently_stepping): ... this.
(currently_stepping_callback): Rename to ...
(currently_stepping_or_nexting_callback): ... this, and also
return true if the thread was stepping over a call (has a
step-resume breakpoint).

gdb/testsuite/
2009-05-27 Pedro Alves <pedro@codesourcery.com>

* gdb.threads/threxit-hop-specific.c: New.
* gdb.threads/threxit-hop-specific.exp: New.
* gdb.threads/thread-execl.c: New.
* gdb.threads/thread-execl.exp: New.

---
gdb/infrun.c | 57 +++++++++++++-------
gdb/testsuite/gdb.threads/thread-execl.c | 48 +++++++++++++++++
gdb/testsuite/gdb.threads/thread-execl.exp | 52 ++++++++++++++++++
gdb/testsuite/gdb.threads/threxit-hop-specific.c | 48 +++++++++++++++++
gdb/testsuite/gdb.threads/threxit-hop-specific.exp | 59 +++++++++++++++++++++
5 files changed, 245 insertions(+), 19 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c 2009-05-27 22:00:21.000000000 +0100
+++ src/gdb/infrun.c 2009-05-27 22:44:18.000000000 +0100
@@ -75,7 +75,8 @@ static void set_schedlock_func (char *ar

static int currently_stepping (struct thread_info *tp);

-static int currently_stepping_callback (struct thread_info *tp, void *data);
+static int currently_stepping_or_nexting_callback (struct thread_info *tp,
+ void *data);

static void xdb_handle_command (char *args, int from_tty);

@@ -2896,6 +2897,11 @@ targets should add new threads to the th
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n");

+ /* Switch context before touching inferior memory, the
+ previous thread may have exited. */
+ if (!ptid_equal (inferior_ptid, ecs->ptid))
+ context_switch (ecs->ptid);
+
/* Saw a breakpoint, but it was hit by the wrong thread.
Just continue. */

@@ -2922,9 +2928,6 @@ targets should add new threads to the th
error (_("Cannot step over breakpoint hit in wrong thread"));
else
{ /* Single step */
- if (!ptid_equal (inferior_ptid, ecs->ptid))
- context_switch (ecs->ptid);
-
if (!non_stop)
{
/* Only need to require the next event from this
@@ -3483,7 +3486,7 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
if (!non_stop)
{
struct thread_info *tp;
- tp = iterate_over_threads (currently_stepping_callback,
+ tp = iterate_over_threads (currently_stepping_or_nexting_callback,
ecs->event_thread);
if (tp)
{
@@ -3498,6 +3501,21 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
return;
}

+ /* If the stepping thread exited, then don't try reverting
+ back to it, just keep going. We need to query the target
+ in case it doesn't support thread exit events. */
+ if (is_exited (tp->ptid)
+ || !target_thread_alive (tp->ptid))
+ {
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog, "\
+infrun: not switching back to stepped thread, it has vanished\n");
+
+ delete_thread (tp->ptid);
+ keep_going (ecs);
+ return;
+ }
+
/* Otherwise, we no longer expect a trap in the current thread.
Clear the trap_expected flag before switching back -- this is
what keep_going would do as well, if we called it. */
@@ -3931,28 +3949,29 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
keep_going (ecs);
}

-/* Are we in the middle of stepping? */
+/* Is thread TP in the middle of single-stepping? */

static int
-currently_stepping_thread (struct thread_info *tp)
+currently_stepping (struct thread_info *tp)
{
- return (tp->step_range_end && tp->step_resume_breakpoint == NULL)
- || tp->trap_expected
- || tp->stepping_through_solib_after_catch;
+ return ((tp->step_range_end && tp->step_resume_breakpoint == NULL)
+ || tp->trap_expected
+ || tp->stepping_through_solib_after_catch
+ || bpstat_should_step ());
}

-static int
-currently_stepping_callback (struct thread_info *tp, void *data)
-{
- /* Return true if any thread *but* the one passed in "data" is
- in the middle of stepping. */
- return tp != data && currently_stepping_thread (tp);
-}
+/* Returns true if any thread *but* the one passed in "data" is in the
+ middle of stepping or of handling a "next". */

static int
-currently_stepping (struct thread_info *tp)
+currently_stepping_or_nexting_callback (struct thread_info *tp, void *data)
{
- return currently_stepping_thread (tp) || bpstat_should_step ();
+ if (tp == data)
+ return 0;
+
+ return (tp->step_range_end
+ || tp->trap_expected
+ || tp->stepping_through_solib_after_catch);
}

/* Inferior has stepped into a subroutine call with source code that
Index: src/gdb/testsuite/gdb.threads/threxit-hop-specific.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/threxit-hop-specific.c 2009-05-27 22:40:47.000000000 +0100
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2009 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>
+#include <unistd.h>
+#include <stdlib.h>
+
+void *
+thread_function (void *arg)
+{
+ /* We'll next over this, with scheduler-locking off. */
+ pthread_exit (NULL);
+}
+
+void
+hop_me (void)
+{
+}
+
+int
+main (int argc, char **argv)
+{
+ pthread_t thread;
+
+ pthread_create (&thread, NULL, thread_function, NULL);
+ pthread_join (thread, NULL); /* wait for exit */
+
+ /* The main thread should be able to hoop over the breakpoint set
+ here.*/
+ hop_me (); /* set thread specific breakpoint here */
+
+ /* ... and reach here. */
+ exit (0); /* set exit breakpoint here */
+}
Index: src/gdb/testsuite/gdb.threads/threxit-hop-specific.exp
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/threxit-hop-specific.exp 2009-05-27 22:00:34.000000000 +0100
@@ -0,0 +1,59 @@
+# Copyright (C) 2009 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/>.
+
+# Test that GDB doesn't get stuck when thread hoping over a thread
+# specific breakpoint when the selected thread has gone away.
+
+set testfile "threxit-hop-specific"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+ executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_load ${binfile}
+
+runto_main
+
+# Get ourselves to the thread that exits
+gdb_breakpoint "thread_function"
+gdb_test "continue" ".*thread_function.*" "continue to thread start"
+
+# Set a thread specific breakpoint somewhere the main thread will pass
+# by, but make it specific to the thread that is going to exit. Step
+# over the pthread_exit call. GDB should still be able to step over
+# the thread specific breakpoint, and reach the other breakpoint,
+# which is not thread specific.
+set bpthrline [gdb_get_line_number "set thread specific breakpoint here"]
+gdb_test "break $bpthrline thread 2" \
+ "Breakpoint .*$srcfile.*$bpthrline.*" \
+ "set thread specific breakpoint"
+
+set bpexitline [gdb_get_line_number "set exit breakpoint here"]
+gdb_breakpoint "$bpexitline"
+
+gdb_test "next" \
+ ".*set exit breakpoint here.*" \
+ "get passed the thread specific breakpoint"
+
+return 0
Index: src/gdb/testsuite/gdb.threads/thread-execl.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/thread-execl.c 2009-05-27 22:00:34.000000000 +0100
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2009 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>
+#include <unistd.h>
+#include <stdlib.h>
+
+static const char *image;
+
+void *
+thread_execler (void *arg)
+{
+ /* Exec ourselves again. */
+ if (execl (image, image, NULL) == -1)
+ {
+ perror ("execl");
+ abort ();
+ }
+
+ return NULL;
+}
+
+int
+main (int argc, char **argv)
+{
+ pthread_t thread;
+
+ image = argv[0];
+
+ pthread_create (&thread, NULL, thread_execler, NULL);
+ pthread_join (thread, NULL);
+
+ return 0;
+}
Index: src/gdb/testsuite/gdb.threads/thread-execl.exp
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/thread-execl.exp 2009-05-27 22:00:34.000000000 +0100
@@ -0,0 +1,52 @@
+# Copyright (C) 2009 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/>.
+
+# Test that GDB doesn't get stuck when stepping over an exec call done
+# by a thread other than the main thread.
+
+# There's no support for exec events in the remote protocol.
+if { [is_remote target] } {
+ return 0
+}
+
+set testfile "thread-execl"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+runto_main
+
+# Get ourselves to the thread that execs
+gdb_breakpoint "thread_execler"
+gdb_test "continue" ".*thread_execler.*" "continue to thread start"
+
+# Now set a breakpoint at `main', and step over the execl call. The
+# breakpoint at main should be reached. GDB should not try to revert
+# back to the old thread from the old image and resume stepping it
+# (since it is gone).
+gdb_breakpoint "main"
+gdb_test "next" ".*main.*" "get to main in new image"
+
+return 0