[3/7] Adjust the bsd-uthread target

[3/7] Adjust the bsd-uthread target

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

This patch adjusts the bsd-uthread target to use thread_change_ptid,
and to never let the core see an event happen in a thread not in
the thread table.

Tested on i386-unknown-freebsd6.0 with a little hack in the
testsuite to link the gdb.thread tests against -lc_r instead
of -lpthread, and on i386-unknown-openbsd4.3, where no hack is
needed.

This patch depends on patch 1 and 2.

OK?

-- 
Pedro Alves

[-- Attachment #2: 003-bsd_uthreads.diff --]
[-- Type: text/x-diff, Size: 1719 bytes --]

2008-08-08  Pedro Alves  <pedro@codesourcery.com>

	* bsd-uthread.c (bsd_uthread_wait): Decorate the main thread with
	thread_change_ptid.  Check for exited threads.
	(bsd_uthread_find_new_threads): Check for exited threads.

---
 gdb/bsd-uthread.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Index: src/gdb/bsd-uthread.c
===================================================================
--- src.orig/gdb/bsd-uthread.c	2008-07-29 12:06:38.000000000 +0100
+++ src/gdb/bsd-uthread.c	2008-07-29 12:17:34.000000000 +0100
@@ -361,14 +361,16 @@ bsd_uthread_wait (ptid_t ptid, struct ta
 	}
     }
 
-  /* HACK: Twiddle INFERIOR_PTID such that the initial thread of a
-     process isn't recognized as a new thread.  */
-  if (ptid_get_tid (ptid) != 0 && !in_thread_list (ptid)
-      && ptid_get_tid (inferior_ptid) == 0)
-    {
-      add_thread_silent (ptid);
-      inferior_ptid = ptid;
-    }
+  /* If INFERIOR_PTID doesn't have a tid member yet, and we now have a
+     ptid with tid set, then ptid is still the initial thread of
+     the process.  Notify GDB core about it.  */
+  if (ptid_get_tid (inferior_ptid) == 0
+      && ptid_get_tid (ptid) != 0 && !in_thread_list (ptid))
+    thread_change_ptid (inferior_ptid, ptid);
+
+  /* Don't let the core see a ptid without a corresponding thread.  */
+  if (!in_thread_list (ptid) || is_exited (ptid))
+    add_thread (ptid);
 
   return ptid;
 }
@@ -413,7 +415,7 @@ bsd_uthread_find_new_threads (void)
     {
       ptid_t ptid = ptid_build (pid, 0, addr);
 
-      if (!in_thread_list (ptid))
+      if (!in_thread_list (ptid) || is_exited (ptid))
 	add_thread (ptid);
 
       addr = read_memory_typed_address (addr + offset,

Re: [3/7] Adjust the bsd-uthread target

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 8 Aug 2008 02:36:03 +0100
> 
> This patch adjusts the bsd-uthread target to use thread_change_ptid,
> and to never let the core see an event happen in a thread not in
> the thread table.
> 
> Tested on i386-unknown-freebsd6.0 with a little hack in the
> testsuite to link the gdb.thread tests against -lc_r instead
> of -lpthread, and on i386-unknown-openbsd4.3, where no hack is
> needed.
> 
> This patch depends on patch 1 and 2.
> 
> OK?
> 
> Index: src/gdb/bsd-uthread.c
> ===================================================================
> --- src.orig/gdb/bsd-uthread.c	2008-07-29 12:06:38.000000000 +0100
> +++ src/gdb/bsd-uthread.c	2008-07-29 12:17:34.000000000 +0100
> @@ -413,7 +415,7 @@ bsd_uthread_find_new_threads (void)
>      {
>        ptid_t ptid = ptid_build (pid, 0, addr);
>  
> -      if (!in_thread_list (ptid))
> +      if (!in_thread_list (ptid) || is_exited (ptid))
>	    add_thread (ptid);

This doesn't make sense to me.  You're adding a threadhere that's no
longer there?

Re: [3/7] Adjust the bsd-uthread target

[Pedro Alves]

A Friday 08 August 2008 22:51:25, Mark Kettenis escreveu:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Fri, 8 Aug 2008 02:36:03 +0100
> >
> > This patch adjusts the bsd-uthread target to use thread_change_ptid,
> > and to never let the core see an event happen in a thread not in
> > the thread table.
> >
> > Tested on i386-unknown-freebsd6.0 with a little hack in the
> > testsuite to link the gdb.thread tests against -lc_r instead
> > of -lpthread, and on i386-unknown-openbsd4.3, where no hack is
> > needed.
> >
> > This patch depends on patch 1 and 2.
> >
> > OK?
> >
> > Index: src/gdb/bsd-uthread.c
> > ===================================================================
> > --- src.orig/gdb/bsd-uthread.c	2008-07-29 12:06:38.000000000 +0100
> > +++ src/gdb/bsd-uthread.c	2008-07-29 12:17:34.000000000 +0100
> > @@ -413,7 +415,7 @@ bsd_uthread_find_new_threads (void)
> >      {
> >        ptid_t ptid = ptid_build (pid, 0, addr);
> >

> > -      if (!in_thread_list (ptid))
> > +      if (!in_thread_list (ptid) || is_exited (ptid))
> >	    add_thread (ptid);
>
> This doesn't make sense to me.  You're adding a threadhere that's no
> longer there?

The "exited" state is what you get *after* you delete_thread the current
thread (inferior_ptid).  We don't delete the thread from the thread list
in that case, but instead tag it as "exited".  It means the thread list
is still holding reference to a thread that has already exited.
If you're seeing an event with a ptid equal to an "exited" thread,
this is the OS reusing the ptid, but, it's a new thread, which
should get a new gdb thread id, so things like thread
specific breakpoints don't think this was the same thread, both the
CLI user or MI see a "new thread event", etc. add_thread handles
that case internally.  This means it is now safe to delete_thread 
(inferior_ptid), regarding context-switching, and infrun state.  It
wasn't a couple of weeks ago.

-- 
Pedro Alves

Re: [3/7] Adjust the bsd-uthread target

[Daniel Jacobowitz]

On Fri, Aug 08, 2008 at 11:41:23PM +0100, Pedro Alves wrote:
> The "exited" state is what you get *after* you delete_thread the current
> thread (inferior_ptid).  We don't delete the thread from the thread list
> in that case, but instead tag it as "exited".  It means the thread list
> is still holding reference to a thread that has already exited.
> If you're seeing an event with a ptid equal to an "exited" thread,
> this is the OS reusing the ptid, but, it's a new thread, which
> should get a new gdb thread id, so things like thread
> specific breakpoints don't think this was the same thread, both the
> CLI user or MI see a "new thread event", etc. add_thread handles
> that case internally.  This means it is now safe to delete_thread 
> (inferior_ptid), regarding context-switching, and infrun state.  It
> wasn't a couple of weeks ago.

How many of the call sites for in_thread_list want to see exited
threads?  Maybe there should be another predicate (I'd suggest
thread_alive except target_thread_alive would confuse things...)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [3/7] Adjust the bsd-uthread target

[Pedro Alves]

On Saturday 09 August 2008 02:31:30, Daniel Jacobowitz wrote:
> On Fri, Aug 08, 2008 at 11:41:23PM +0100, Pedro Alves wrote:
> > The "exited" state is what you get *after* you delete_thread the current
> > thread (inferior_ptid).  We don't delete the thread from the thread list
> > in that case, but instead tag it as "exited".  It means the thread list
> > is still holding reference to a thread that has already exited.
> > If you're seeing an event with a ptid equal to an "exited" thread,
> > this is the OS reusing the ptid, but, it's a new thread, which
> > should get a new gdb thread id, so things like thread
> > specific breakpoints don't think this was the same thread, both the
> > CLI user or MI see a "new thread event", etc. add_thread handles
> > that case internally.  This means it is now safe to delete_thread
> > (inferior_ptid), regarding context-switching, and infrun state.  It
> > wasn't a couple of weeks ago.

> How many of the call sites for in_thread_list want to see exited
> threads?  Maybe there should be another predicate (I'd suggest
> thread_alive except target_thread_alive would confuse things...)

I actually started out using a new predicate, but then got rid
of it, as it looked more confusing to me.

There are a few calls than want to see all threads.  context_switch (while
it still exists :-) ) and handle_inferior_event want to.  The
breakpoints.c and infcmd.c calls don't care currently.

Can I ask you to not do that now, please?  I can come back to it
as an follow-up cleanup.  I've got several patches that touch
contexts where this form was used, and it would be a lot of work
to rebase and retest them all for not much gain.  A single patch
to cleanup and introduce a new predicate can be made in one sweep.

-- 
Pedro Alves

Re: [3/7] Adjust the bsd-uthread target

[Daniel Jacobowitz]

On Mon, Aug 11, 2008 at 02:32:49PM +0100, Pedro Alves wrote:
> I actually started out using a new predicate, but then got rid
> of it, as it looked more confusing to me.
> 
> There are a few calls than want to see all threads.  context_switch (while
> it still exists :-) ) and handle_inferior_event want to.  The
> breakpoints.c and infcmd.c calls don't care currently.
> 
> Can I ask you to not do that now, please?  I can come back to it
> as an follow-up cleanup.  I've got several patches that touch
> contexts where this form was used, and it would be a lot of work
> to rebase and retest them all for not much gain.  A single patch
> to cleanup and introduce a new predicate can be made in one sweep.

Yeah, this patch is OK; we can examine the uses again later.

-- 
Daniel Jacobowitz
CodeSourcery