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

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

[Pedro Alves]

[ 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

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

[Ulrich Weigand]

Pedro Alves wrote:

> [ Hi Ulrich, I'm CCing you because this touches code you've
> touched recently. ]
...
> 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.

I agree, those changes look fine to me.  Thanks for the
detailed explanation!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

On Thursday 28 May 2009 12:11:39, Ulrich Weigand wrote:

> I agree, those changes look fine to me.  Thanks for the
> detailed explanation!

Commited.  Thanks!

-- 
Pedro Alves

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

[Joel Brobecker]

[you're explaination just gave me a headache, that's what you get when
you go on vacation for too long :-)]

Coming late on this discussion:

> +	  /* 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.  */

I'm just wondering if it would make sense to explain why you need to do
that in the comment, or perhaps just explain what would happen if you
didn't. There is such a nice description in the body of your email, and
I'm thinking it's worth having a short summary in the code.

WDYT?

(yes, I know, I'm a comments freak)

-- 
Joel

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

[Pedro Alves]

On Tuesday 09 June 2009 20:58:07, Joel Brobecker wrote:

> Coming late on this discussion:
> 
> > +	  /* 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.  */
> 
> I'm just wondering if it would make sense to explain why you need to do
> that in the comment, or perhaps just explain what would happen if you
> didn't. There is such a nice description in the body of your email, and
> I'm thinking it's worth having a short summary in the code.
> 
> WDYT?
> 

Sure!

> (yes, I know, I'm a comments freak)

Something like this?  Or were you thinking on some different
aspect of the issue?  Let me know and I'll cook something
extra.

2009-06-10  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (handle_inferior_event): Update comment around trying
	to revert back to a stepping thread that has exited.

---
 gdb/infrun.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2009-06-10 16:48:24.000000000 +0100
+++ src/gdb/infrun.c	2009-06-10 17:18:20.000000000 +0100
@@ -3496,9 +3496,25 @@ 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 the stepping thread exited, then don't try to switch
+	     back and resume it, which could fail in several different
+	     ways depending on the target.  Instead, just keep going.
+
+	     We can find a stepping dead thread in the thread list in
+	     two cases:
+
+	     - The target supports thread exit events, and when the
+	     target tries to delete the thread from the thread list,
+	     inferior_ptid pointed at the exiting thread.  In such
+	     case, calling delete_thread does not really remove the
+	     thread from the list; instead, the thread is left listed,
+	     with 'exited' state.
+
+	     - The target's debug interface does not support thread
+	     exit events, and so we have no idea whatsoever if the
+	     previously stepping thread is still alive.  For that
+	     reason, we need to synchronously query the target
+	     now.  */
 	  if (is_exited (tp->ptid)
 	      || !target_thread_alive (tp->ptid))
 	    {

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

[Joel Brobecker]

> 2009-06-10  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* infrun.c (handle_inferior_event): Update comment around trying
> 	to revert back to a stepping thread that has exited.

Looks great, Pedro! Thanks for doing that...

-- 
Joel

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

[Ulrich Weigand]

Pedro Alves wrote:

> Index: src/gdb/testsuite/gdb.threads/threxit-hop-specific.exp
[snip]
> +# 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"

I'm seeing failures in this test on an ARM system with debug info
packages installed for libc / libpthread.  The problem is that
pthread_exit is implemented in terms of pthread cancellation,
which under the covers does a longjmp back up to the start_thread
routine in libpthread.

Normally, GDB will consider this routine to be assembler code and
continue stepping until the end.  However, if we actually have
debug info, GDB will stop stepping here (because start_thread
is in fact the parent of the application's thread routine), and
thus the test fails.

This does look like the correct behaviour under the circumstances,
which would imply the test case is not quite correct.  Do you
agree or am I missing something here?  Any thoughts on how to fix
the test?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

On Monday 16 August 2010 18:38:40, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > Index: src/gdb/testsuite/gdb.threads/threxit-hop-specific.exp
> [snip]
> > +# 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"
> 
> I'm seeing failures in this test on an ARM system with debug info
> packages installed for libc / libpthread.  The problem is that
> pthread_exit is implemented in terms of pthread cancellation,
> which under the covers does a longjmp back up to the start_thread
> routine in libpthread.
> 
> Normally, GDB will consider this routine to be assembler code and
> continue stepping until the end.  However, if we actually have
> debug info, GDB will stop stepping here (because start_thread
> is in fact the parent of the application's thread routine), and
> thus the test fails.
> 
> This does look like the correct behaviour under the circumstances,
> which would imply the test case is not quite correct.  Do you
> agree or am I missing something here?  

I agree.

> Any thoughts on how to fix the test?

Replacing the "next" by a "continue" should work.  I've looked over the
original description of the problem this is covering, and, that
would still exercise the problem (which is gdb trying to step
the other (main) thread with inferior_ptid still pointing at
the thread that was being "next"ed, and in the process failing
to remove breakpoints from memory because inferior_ptid pointed
at an inferior thread.

-- 
Pedro Alves

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

[Pedro Alves]

On Monday 16 August 2010 19:30:20, Pedro Alves wrote:
> the thread that was being "next"ed, and in the process failing
> to remove breakpoints from memory because inferior_ptid pointed

> at an inferior thread.

err, probably obvious, but, that should instead
read "at a thread that is gone".

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:

> Replacing the "next" by a "continue" should work.  I've looked over the
> original description of the problem this is covering, and, that
> would still exercise the problem (which is gdb trying to step
> the other (main) thread with inferior_ptid still pointing at
> the thread that was being "next"ed, and in the process failing
> to remove breakpoints from memory because inferior_ptid pointed
> at an inferior thread.

But isn't the code your patch changes under an if that's only true
if another thread is currently being stepped or nexted?  If we just
do "continue" here, that's no longer the case, and the code wouldn't
be exercised at all ...

@@ -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.  */


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

On Monday 16 August 2010 19:40:47, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > Replacing the "next" by a "continue" should work.  I've looked over the
> > original description of the problem this is covering, and, that
> > would still exercise the problem (which is gdb trying to step
> > the other (main) thread with inferior_ptid still pointing at
> > the thread that was being "next"ed, and in the process failing
> > to remove breakpoints from memory because inferior_ptid pointed
> > at an inferior thread.
> 
> But isn't the code your patch changes under an if that's only true
> if another thread is currently being stepped or nexted?  If we just
> do "continue" here, that's no longer the case, and the code wouldn't
> be exercised at all ...

That's also exercised by the other test my original patch added,
IIRC (gdb.thread/thread-execl.exp).  There were two bugs fixed by
that patch.  The specific bug the threxit-hop-specific.exp test is
covering is the "failing to remove breakpoints" one.

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:
> On Monday 16 August 2010 19:40:47, Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > 
> > > Replacing the "next" by a "continue" should work.  I've looked over the
> > > original description of the problem this is covering, and, that
> > > would still exercise the problem (which is gdb trying to step
> > > the other (main) thread with inferior_ptid still pointing at
> > > the thread that was being "next"ed, and in the process failing
> > > to remove breakpoints from memory because inferior_ptid pointed
> > > at an inferior thread.
> > 
> > But isn't the code your patch changes under an if that's only true
> > if another thread is currently being stepped or nexted?  If we just
> > do "continue" here, that's no longer the case, and the code wouldn't
> > be exercised at all ...
> 
> That's also exercised by the other test my original patch added,
> IIRC (gdb.thread/thread-execl.exp).  There were two bugs fixed by
> that patch.  The specific bug the threxit-hop-specific.exp test is
> covering is the "failing to remove breakpoints" one.

Ah, I see.  Well, in that case, I'd certainly be happy with changing
the next to continue ...

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[commit] Re: Fixes for a couple of infrun bugs (thread hop, revert to step thread).

[Ulrich Weigand]

Pedro Alves wrote:
> On Monday 16 August 2010 19:40:47, Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > 
> > > Replacing the "next" by a "continue" should work.  I've looked over the
> > > original description of the problem this is covering, and, that
> > > would still exercise the problem (which is gdb trying to step
> > > the other (main) thread with inferior_ptid still pointing at
> > > the thread that was being "next"ed, and in the process failing
> > > to remove breakpoints from memory because inferior_ptid pointed
> > > at an inferior thread.
> > 
> > But isn't the code your patch changes under an if that's only true
> > if another thread is currently being stepped or nexted?  If we just
> > do "continue" here, that's no longer the case, and the code wouldn't
> > be exercised at all ...
> 
> That's also exercised by the other test my original patch added,
> IIRC (gdb.thread/thread-execl.exp).  There were two bugs fixed by
> that patch.  The specific bug the threxit-hop-specific.exp test is
> covering is the "failing to remove breakpoints" one.

OK, I've now committed the following patch.

Tested on armv7l-linux-gnueabi and i686-linux.

Thanks,
Ulrich


ChangeLog:

	* gdb.threads/threxit-hop-specific.exp: Use "continue" instead
	of "next" to proceed over pthread_exit call.

Index: gdb/testsuite/gdb.threads/threxit-hop-specific.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/threxit-hop-specific.exp,v
retrieving revision 1.2
diff -u -p -r1.2 threxit-hop-specific.exp
--- gdb/testsuite/gdb.threads/threxit-hop-specific.exp	1 Jan 2010 07:32:06 -0000	1.2
+++ gdb/testsuite/gdb.threads/threxit-hop-specific.exp	16 Aug 2010 19:05:38 -0000
@@ -52,7 +52,7 @@ gdb_test "break $bpthrline thread 2" \
 set bpexitline [gdb_get_line_number "set exit breakpoint here"]
 gdb_breakpoint "$bpexitline"
 
-gdb_test "next" \
+gdb_test "continue" \
     ".*set exit breakpoint here.*" \
     "get past the thread specific breakpoint"
 


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com