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

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