[PATCH] Disable thread specific breakpoints when thread dies

[PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

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

Hi,

The attached patch disables thread specific breakpoints if they are hit 
(in another thread) after the specific thread has died.

The rationale is that, once dead, the thread in question can never come 
back, and the breakpoint is then only a, potentially serious, 
performance drain. Even if another thread can be created with that ID it 
would not be appropriate to 'hit' the breakpoint.

The breakpoint is not deleted, only disabled. It may still be easily 
re-enabled when the program is re-run.

Is this OK?

Andrew Stubbs

[-- Attachment #2: thread-break.patch --]
[-- Type: text/plain, Size: 1472 bytes --]

2005-11-09  Andrew Stubbs  <andrew.stubbs@st.com>

	* breakpoint.c (breakpoint_thread_match): Disable thread specific
	breakpoints if we hit one after the thread has ceased to exist.

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2005-05-29 04:13:17.000000000 +0100
+++ src/gdb/breakpoint.c	2005-11-01 13:49:40.000000000 +0000
@@ -1797,9 +1797,23 @@ breakpoint_thread_match (CORE_ADDR pc, p
 
       if ((breakpoint_enabled (bpt->owner)
 	   || bpt->owner->enable_state == bp_permanent)
-	  && bpt->address == pc
-	  && (bpt->owner->thread == -1 || bpt->owner->thread == thread))
+	  && bpt->address == pc)
 	{
+          /* Disable matched breakpoint if thread no longer exists: this
+	     prevents other tasks from hitting the breakpoint and improves
+	     performance.  */
+          if ((bpt->owner->thread != -1) && (bpt->owner->thread != thread))
+          {
+            if (!target_thread_alive(thread_id_to_pid(bpt->owner->thread)))
+              {
+                printf_filtered (
+		      "Breakpoint %d disabled as thread %d no longer alive.\n",
+		      bpt->owner->number, bpt->owner->thread);
+                disable_breakpoint(bpt->owner);
+              }
+            continue;  /* no need to consider this breakpoint any further */
+          }
+
 	  if (overlay_debugging 
 	      && section_is_overlay (bpt->section) 
 	      && !section_is_mapped (bpt->section))

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Wed, Nov 09, 2005 at 05:39:18PM +0000, Andrew STUBBS wrote:
> Hi,
> 
> The attached patch disables thread specific breakpoints if they are hit 
> (in another thread) after the specific thread has died.
> 
> The rationale is that, once dead, the thread in question can never come 
> back, and the breakpoint is then only a, potentially serious, 
> performance drain. Even if another thread can be created with that ID it 
> would not be appropriate to 'hit' the breakpoint.
> 
> The breakpoint is not deleted, only disabled. It may still be easily 
> re-enabled when the program is re-run.
> 
> Is this OK?

Sorry, I don't like this.

First, minor formatting problems:
  - printf_filtered should take a _("gettextized") string now.
  - spaces before opening parentheses (other than in the gettext case)
  - Capital letters and periods in comments.

But about the actual patch...

I'd like to minimize the amount that GDB plays with the user visible
state of breakpoints.  Can we arrange to just not insert breakpoints,
if they are thread-specific to a dead thread?  I think that'll work
too.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> But about the actual patch...
> 
> I'd like to minimize the amount that GDB plays with the user visible
> state of breakpoints.  Can we arrange to just not insert breakpoints,
> if they are thread-specific to a dead thread?  I think that'll work
> too.

So you want to disable it 'unofficially'? I suppose that would be 
preferable, but I wouldn't know that best way to achieve it. I'll have a 
look though.

GDB already plays with watchpoints (deletes them in fact). At least it 
did in 6.3. That said I wouldn't complain if somebody 'fixed' them so 
that they were reinstated when the program returned to the right context.

Thanks

Andrew

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Mon, Nov 14, 2005 at 11:32:00AM +0000, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >But about the actual patch...
> >
> >I'd like to minimize the amount that GDB plays with the user visible
> >state of breakpoints.  Can we arrange to just not insert breakpoints,
> >if they are thread-specific to a dead thread?  I think that'll work
> >too.
> 
> So you want to disable it 'unofficially'? I suppose that would be 
> preferable, but I wouldn't know that best way to achieve it. I'll have a 
> look though.

I'm thinking about a check in insert_breakpoints, just before calling
insert_bp_location.  By the !breakpoint_enabled.  This may have other
side effects, so it would need testing.

> GDB already plays with watchpoints (deletes them in fact). At least it 
> did in 6.3. That said I wouldn't complain if somebody 'fixed' them so 
> that they were reinstated when the program returned to the right context.

Yes, exactly.

Just to clarify: yes, I acknowledge that what you're doing here is
similar to many other places that GDB messes with the state, e.g.
shlib_disabled.  But I've been looking on and off at overhauling our
breakpoint management, and this is one of the bits that I really want
to go away.  If we want to display to the user "sorry, right now I
believe this breakpoint can not/should not be inserted, so I'm not
going to", then it shouldn't show up as "disabled".  

I want to decrease the total number of interactions.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> I'm thinking about a check in insert_breakpoints, just before calling
> insert_bp_location.  By the !breakpoint_enabled.  This may have other
> side effects, so it would need testing.

This seems to work. I have run the regression testsuite on both 
i686-pc-linux-gnu native and sh-elf cross. There were no extra failures.

OK?

Andrew

[-- Attachment #2: thread-break-2.patch --]
[-- Type: text/plain, Size: 1036 bytes --]

2005-11-15  Andrew Stubbs  <andrew.stubbs@st.com>

	* breakpoint.c (insert_breakpoints): Check that a thread exists
	before inserting thread specific breakpoints.

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2005-11-14 18:58:50.000000000 +0000
+++ src/gdb/breakpoint.c	2005-11-15 14:01:40.000000000 +0000
@@ -1142,6 +1142,15 @@ insert_breakpoints (void)
       if (!breakpoint_enabled (b->owner))
 	continue;
 
+      /* There is no point inserting thread-specific breakpoints if the
+	 thread no longer exists.  */
+      if (b->owner->thread != -1
+	  && !target_thread_alive(thread_id_to_pid(b->owner->thread)))
+	{
+	  printf_unfiltered ("Not placing breakpoint %d due to death of thread %d\n", b->owner->number, b->owner->thread);
+	  continue;
+	}
+
       /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
 	 hardware watchpoints are split into multiple loc breakpoints.  */
       if ((b->loc_type == bp_loc_hardware_watchpoint

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

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

Sorry, I sent the wrong version of the patch.

The attached has the diagnosic I used to test it removed.

Thanks

Andrew

[-- Attachment #2: thread-break-2.patch --]
[-- Type: text/plain, Size: 909 bytes --]

2005-11-16  Andrew Stubbs  <andrew.stubbs@st.com>

	* breakpoint.c (insert_breakpoints): Check that a thread exists
	before inserting thread specific breakpoints.

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2005-11-15 14:13:56.000000000 +0000
+++ src/gdb/breakpoint.c	2005-11-15 15:51:50.000000000 +0000
@@ -1142,6 +1142,12 @@ insert_breakpoints (void)
       if (!breakpoint_enabled (b->owner))
 	continue;
 
+      /* There is no point inserting thread-specific breakpoints if the
+	 thread no longer exists.  */
+      if (b->owner->thread != -1
+	  && !target_thread_alive(thread_id_to_pid(b->owner->thread)))
+	continue;
+
       /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
 	 hardware watchpoints are split into multiple loc breakpoints.  */
       if ((b->loc_type == bp_loc_hardware_watchpoint

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Wed, Nov 16, 2005 at 02:52:17PM +0000, Andrew STUBBS wrote:
> +      /* There is no point inserting thread-specific breakpoints if the
> +	 thread no longer exists.  */
> +      if (b->owner->thread != -1
> +	  && !target_thread_alive(thread_id_to_pid(b->owner->thread)))
> +	continue;

You shouldn't need to use the target method here.  Does valid_thread_id
work?

Also, please remember the space before opening parentheses.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> On Wed, Nov 16, 2005 at 02:52:17PM +0000, Andrew STUBBS wrote:
> 
>>+      /* There is no point inserting thread-specific breakpoints if the
>>+	 thread no longer exists.  */
>>+      if (b->owner->thread != -1
>>+	  && !target_thread_alive(thread_id_to_pid(b->owner->thread)))
>>+	continue;
> 
> 
> You shouldn't need to use the target method here.  Does valid_thread_id
> work?
> 
> Also, please remember the space before opening parentheses.

The thread still seems to have a valid ID after it has died. You can 
even do 'b 8 t 4' after the program has exited. It does give an error 
for threads which never existed though.

Here is the patch again with the spaces fixed.

Andrew



[-- Attachment #2: thread-break-2.patch --]
[-- Type: text/plain, Size: 911 bytes --]

2005-11-17  Andrew Stubbs  <andrew.stubbs@st.com>

	* breakpoint.c (insert_breakpoints): Check that a thread exists
	before inserting thread specific breakpoints.

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2005-11-17 15:47:04.000000000 +0000
+++ src/gdb/breakpoint.c	2005-11-17 15:47:42.000000000 +0000
@@ -1142,6 +1142,12 @@ insert_breakpoints (void)
       if (!breakpoint_enabled (b->owner))
 	continue;
 
+      /* There is no point inserting thread-specific breakpoints if the
+	 thread no longer exists.  */
+      if (b->owner->thread != -1
+	  && !target_thread_alive (thread_id_to_pid (b->owner->thread)))
+	continue;
+
       /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
 	 hardware watchpoints are split into multiple loc breakpoints.  */
       if ((b->loc_type == bp_loc_hardware_watchpoint

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

Ping.

Since valid_thread_id did not work am I OK to commit this one. It has 
been on hold since mid Novemeber.

Thanks

Andrew

Andrew Stubbs wrote:
> Daniel Jacobowitz wrote:
> 
>> On Wed, Nov 16, 2005 at 02:52:17PM +0000, Andrew STUBBS wrote:
>>
>>> +      /* There is no point inserting thread-specific breakpoints if the
>>> +     thread no longer exists.  */
>>> +      if (b->owner->thread != -1
>>> +      && !target_thread_alive(thread_id_to_pid(b->owner->thread)))
>>> +    continue;
>>
>>
>>
>> You shouldn't need to use the target method here.  Does valid_thread_id
>> work?
>>
>> Also, please remember the space before opening parentheses.
> 
> 
> The thread still seems to have a valid ID after it has died. You can 
> even do 'b 8 t 4' after the program has exited. It does give an error 
> for threads which never existed though.
> 
> Here is the patch again with the spaces fixed.
> 
> Andrew
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 2005-11-17  Andrew Stubbs  <andrew.stubbs@st.com>
> 
> 	* breakpoint.c (insert_breakpoints): Check that a thread exists
> 	before inserting thread specific breakpoints.
> 
> Index: src/gdb/breakpoint.c
> ===================================================================
> --- src.orig/gdb/breakpoint.c	2005-11-17 15:47:04.000000000 +0000
> +++ src/gdb/breakpoint.c	2005-11-17 15:47:42.000000000 +0000
> @@ -1142,6 +1142,12 @@ insert_breakpoints (void)
>        if (!breakpoint_enabled (b->owner))
>  	continue;
>  
> +      /* There is no point inserting thread-specific breakpoints if the
> +	 thread no longer exists.  */
> +      if (b->owner->thread != -1
> +	  && !target_thread_alive (thread_id_to_pid (b->owner->thread)))
> +	continue;
> +
>        /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
>  	 hardware watchpoints are split into multiple loc breakpoints.  */
>        if ((b->loc_type == bp_loc_hardware_watchpoint

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Michael Snyder]

Daniel,

valid_thread_id wouldn't be safe, because thread ids might be re-used.

This implementation looks fine to me...

Michael

Andrew STUBBS wrote:
> Ping.
> 
> Since valid_thread_id did not work am I OK to commit this one. It has 
> been on hold since mid Novemeber.
> 
> Thanks
> 
> Andrew
> 
> Andrew Stubbs wrote:
> 
>> Daniel Jacobowitz wrote:
>>
>>> On Wed, Nov 16, 2005 at 02:52:17PM +0000, Andrew STUBBS wrote:
>>>
>>>> +      /* There is no point inserting thread-specific breakpoints if 
>>>> the
>>>> +     thread no longer exists.  */
>>>> +      if (b->owner->thread != -1
>>>> +      && !target_thread_alive(thread_id_to_pid(b->owner->thread)))
>>>> +    continue;
>>>
>>>
>>>
>>>
>>> You shouldn't need to use the target method here.  Does valid_thread_id
>>> work?
>>>
>>> Also, please remember the space before opening parentheses.
>>
>>
>>
>> The thread still seems to have a valid ID after it has died. You can 
>> even do 'b 8 t 4' after the program has exited. It does give an error 
>> for threads which never existed though.
>>
>> Here is the patch again with the spaces fixed.
>>
>> Andrew
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>> 2005-11-17  Andrew Stubbs  <andrew.stubbs@st.com>
>>
>>     * breakpoint.c (insert_breakpoints): Check that a thread exists
>>     before inserting thread specific breakpoints.
>>
>> Index: src/gdb/breakpoint.c
>> ===================================================================
>> --- src.orig/gdb/breakpoint.c    2005-11-17 15:47:04.000000000 +0000
>> +++ src/gdb/breakpoint.c    2005-11-17 15:47:42.000000000 +0000
>> @@ -1142,6 +1142,12 @@ insert_breakpoints (void)
>>        if (!breakpoint_enabled (b->owner))
>>      continue;
>>  
>> +      /* There is no point inserting thread-specific breakpoints if the
>> +     thread no longer exists.  */
>> +      if (b->owner->thread != -1
>> +      && !target_thread_alive (thread_id_to_pid (b->owner->thread)))
>> +    continue;
>> +
>>        /* FIXME drow/2003-10-07: This code should be pushed elsewhere 
>> when
>>       hardware watchpoints are split into multiple loc breakpoints.  */
>>        if ((b->loc_type == bp_loc_hardware_watchpoint
> 
> 
> 

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Thu, Jan 12, 2006 at 08:18:02PM -0800, Michael Snyder wrote:
> Daniel,
> 
> valid_thread_id wouldn't be safe, because thread ids might be re-used.
> 
> This implementation looks fine to me...

How is this any better in that case?  These thread IDs can't be reused
without all the rest of GDB having the same problem - this is GDB's
internal thread ID, not the inferior PTID (see the valid_thread_id
implementation).

From Andrew's message it sounds like a missing clear of the thread list
at inferior exit.  Which I thought someone had fixed just recently.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Thu, Nov 17, 2005 at 03:48:59PM +0000, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >On Wed, Nov 16, 2005 at 02:52:17PM +0000, Andrew STUBBS wrote:
> >
> >>+      /* There is no point inserting thread-specific breakpoints if the
> >>+	 thread no longer exists.  */
> >>+      if (b->owner->thread != -1
> >>+	  && !target_thread_alive(thread_id_to_pid(b->owner->thread)))
> >>+	continue;
> >
> >
> >You shouldn't need to use the target method here.  Does valid_thread_id
> >work?
> >
> >Also, please remember the space before opening parentheses.
> 
> The thread still seems to have a valid ID after it has died. You can 
> even do 'b 8 t 4' after the program has exited. It does give an error 
> for threads which never existed though.

Why does that happen?  It is presumably a bug.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
>>> You shouldn't need to use the target method here.  Does valid_thread_id
>>> work?
>>>
>>> Also, please remember the space before opening parentheses.
>> The thread still seems to have a valid ID after it has died. You can 
>> even do 'b 8 t 4' after the program has exited. It does give an error 
>> for threads which never existed though.
> 
> Why does that happen?  It is presumably a bug.
> 

I have looked into this. The problem is that the threads are only 
deleted from the table when 'info threads' is used. The target method 
works because that queries the target, not GDB's internal state, and 
always gets the right answer (at least in our target interface).

I am happy, therefore, that the attached patch, with valid_thread_id(), 
is correct, and will work once this other problem has been solved (or if 
the user types 'info threads').

OK to commit?

I'll have a look for real problem next week probably.

Andrew Stubbs

[-- Attachment #2: thread-break-3.patch --]
[-- Type: text/plain, Size: 888 bytes --]

2006-01-13  Andrew Stubbs  <andrew.stubbs@st.com>

	* breakpoint.c (insert_breakpoints): Check that a thread exists
	before inserting thread specific breakpoints.

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2006-01-03 18:23:49.000000000 +0000
+++ src/gdb/breakpoint.c	2006-01-13 16:34:50.000000000 +0000
@@ -1142,6 +1142,12 @@ insert_breakpoints (void)
       if (!breakpoint_enabled (b->owner))
 	continue;
 
+      /* There is no point inserting thread-specific breakpoints if the
+	 thread no longer exists.  */
+      if (b->owner->thread != -1
+	  && !valid_thread_id (b->owner->thread))
+	continue;
+
       /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
 	 hardware watchpoints are split into multiple loc breakpoints.  */
       if ((b->loc_type == bp_loc_hardware_watchpoint

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Mark Kettenis]

> Date: Fri, 13 Jan 2006 17:33:26 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> 
> Daniel Jacobowitz wrote:
> >>> You shouldn't need to use the target method here.  Does valid_thread_id
> >>> work?
> >>>
> >>> Also, please remember the space before opening parentheses.
> >> The thread still seems to have a valid ID after it has died. You can 
> >> even do 'b 8 t 4' after the program has exited. It does give an error 
> >> for threads which never existed though.
> > 
> > Why does that happen?  It is presumably a bug.
> > 
> 
> I have looked into this. The problem is that the threads are only 
> deleted from the table when 'info threads' is used. The target method 
> works because that queries the target, not GDB's internal state, and 
> always gets the right answer (at least in our target interface).
> 
> I am happy, therefore, that the attached patch, with valid_thread_id(), 
> is correct, and will work once this other problem has been solved (or if 
> the user types 'info threads').
> 
> OK to commit?

Sorry, but I don't think we should commit a patch that's just papering
over some other more serious problem, perhaps perhaps if there's some
pressing need to do so.

Mark

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Fri, Jan 13, 2006 at 09:11:35PM +0100, Mark Kettenis wrote:
> > Date: Fri, 13 Jan 2006 17:33:26 +0000
> > From: Andrew STUBBS <andrew.stubbs@st.com>
> > 
> > Daniel Jacobowitz wrote:
> > >>> You shouldn't need to use the target method here.  Does valid_thread_id
> > >>> work?
> > >>>
> > >>> Also, please remember the space before opening parentheses.
> > >> The thread still seems to have a valid ID after it has died. You can 
> > >> even do 'b 8 t 4' after the program has exited. It does give an error 
> > >> for threads which never existed though.
> > > 
> > > Why does that happen?  It is presumably a bug.
> > > 
> > 
> > I have looked into this. The problem is that the threads are only 
> > deleted from the table when 'info threads' is used. The target method 
> > works because that queries the target, not GDB's internal state, and 
> > always gets the right answer (at least in our target interface).

At a guess, we should be calling whatever the appropriate function is
to empty the threads list in the generic bits of GDB, around where we
call the target mourn_inferior hook.  Does that make sense?

> > I am happy, therefore, that the attached patch, with valid_thread_id(), 
> > is correct, and will work once this other problem has been solved (or if 
> > the user types 'info threads').
> > 
> > OK to commit?
> 
> Sorry, but I don't think we should commit a patch that's just papering
> over some other more serious problem, perhaps perhaps if there's some
> pressing need to do so.

Sorry about the confusion here - Mark, the patch Andrew posted is not a
workaround, but exactly the opposite.  It's a correct patch that ought
to work, but doesn't because of a different bug, that he has
volunteered to track down next week.

With that clarified, do you have any objection to the patch?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Mark Kettenis]

> Date: Sat, 14 Jan 2006 10:45:53 -0500
> From: Daniel Jacobowitz <drow@false.org>
>
> > > I am happy, therefore, that the attached patch, with valid_thread_id(), 
> > > is correct, and will work once this other problem has been solved (or if 
> > > the user types 'info threads').
> > > 
> > > OK to commit?
> > 
> > Sorry, but I don't think we should commit a patch that's just papering
> > over some other more serious problem, perhaps perhaps if there's some
> > pressing need to do so.
> 
> Sorry about the confusion here - Mark, the patch Andrew posted is not a
> workaround, but exactly the opposite.  It's a correct patch that ought
> to work, but doesn't because of a different bug, that he has
> volunteered to track down next week.
> 
> With that clarified, do you have any objection to the patch?

Of course not.

Mark

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Fri, Jan 13, 2006 at 05:33:26PM +0000, Andrew STUBBS wrote:
> 2006-01-13  Andrew Stubbs  <andrew.stubbs@st.com>
> 
> 	* breakpoint.c (insert_breakpoints): Check that a thread exists
> 	before inserting thread specific breakpoints.

This is OK now.  Thanks!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> On Fri, Jan 13, 2006 at 05:33:26PM +0000, Andrew STUBBS wrote:
>> 2006-01-13  Andrew Stubbs  <andrew.stubbs@st.com>
>>
>> 	* breakpoint.c (insert_breakpoints): Check that a thread exists
>> 	before inserting thread specific breakpoints.
> 
> This is OK now.  Thanks!
> 

Thanks, patch committed.

Now onto figuring out why it doesn't work....

Andrew

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

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

Andrew STUBBS wrote:
> Now onto figuring out why it doesn't work....

The smoking gun seems to be here:

[From linux-thread-db.c]

static void
detach_thread (ptid_t ptid, int verbose)
{
   struct thread_info *thread_info;

   if (verbose)
     printf_unfiltered (_("[%s exited]\n"), target_pid_to_str (ptid));

   /* Don't delete the thread now, because it still reports as active
      until it has executed a few instructions after the event
      breakpoint - if we deleted it now, "info threads" would cause us
      to re-attach to it.  Just mark it as having had a TD_DEATH
      event.  This means that we won't delete it from our thread list
      until we notice that it's dead (via prune_threads), or until
      something re-uses its thread ID.  */
   thread_info = find_thread_pid (ptid);
   gdb_assert (thread_info != NULL);
   thread_info->private->dying = 1;
}


The attached patch fixes the problem, but I don't know if it does it the 
best way.

What do you think?

Andrew Stubbs

[-- Attachment #2: prune_threads.patch --]
[-- Type: text/plain, Size: 1949 bytes --]

2006-01-16  Amdrew Stubbs  <andrew.stubbs@st.com>

	* gdbthread.h (prune_threads): Add prototype.
	* infrun.c (normal_stop): Call prune_threads().
	* thread.c (prune_threads): Remove 'static'.

Index: src/gdb/gdbthread.h
===================================================================
--- src.orig/gdb/gdbthread.h	2006-01-16 16:08:57.000000000 +0000
+++ src/gdb/gdbthread.h	2006-01-16 16:13:57.000000000 +0000
@@ -80,6 +80,9 @@ extern struct thread_info *add_thread (p
 /* Delete an existing thread list entry.  */
 extern void delete_thread (ptid_t);
 
+/* Delete all dead threads from the list.  */
+extern void prune_threads (void);
+
 /* Delete a step_resume_breakpoint from the thread database. */
 extern void delete_step_resume_breakpoint (void *);
 
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2006-01-16 16:08:57.000000000 +0000
+++ src/gdb/infrun.c	2006-01-16 16:13:57.000000000 +0000
@@ -3037,6 +3037,9 @@ Further execution is probably impossible
   if (stopped_by_random_signal)
     disable_current_display ();
 
+  /* Delete any threads which have died.  */
+  prune_threads ();
+
   /* Don't print a message if in the middle of doing a "step n"
      operation for n > 1 */
   if (step_multi && stop_step)
Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2006-01-16 16:08:57.000000000 +0000
+++ src/gdb/thread.c	2006-01-16 16:13:57.000000000 +0000
@@ -64,7 +64,6 @@ static void info_threads_command (char *
 static void thread_apply_command (char *, int);
 static void restore_current_thread (ptid_t);
 static void switch_to_thread (ptid_t ptid);
-static void prune_threads (void);
 
 void
 delete_step_resume_breakpoint (void *arg)
@@ -382,7 +381,7 @@ thread_alive (struct thread_info *tp)
   return 1;
 }
 
-static void
+void
 prune_threads (void)
 {
   struct thread_info *tp, *next;

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Andrew STUBBS]

Any opinions anybody?

Andrew Stubbs wrote:
> Andrew STUBBS wrote:
>> Now onto figuring out why it doesn't work....
> 
> The smoking gun seems to be here:
> 
> [From linux-thread-db.c]
> 
> static void
> detach_thread (ptid_t ptid, int verbose)
> {
>   struct thread_info *thread_info;
> 
>   if (verbose)
>     printf_unfiltered (_("[%s exited]\n"), target_pid_to_str (ptid));
> 
>   /* Don't delete the thread now, because it still reports as active
>      until it has executed a few instructions after the event
>      breakpoint - if we deleted it now, "info threads" would cause us
>      to re-attach to it.  Just mark it as having had a TD_DEATH
>      event.  This means that we won't delete it from our thread list
>      until we notice that it's dead (via prune_threads), or until
>      something re-uses its thread ID.  */
>   thread_info = find_thread_pid (ptid);
>   gdb_assert (thread_info != NULL);
>   thread_info->private->dying = 1;
> }
> 
> 
> The attached patch fixes the problem, but I don't know if it does it the 
> best way.
> 
> What do you think?
> 
> Andrew Stubbs
> 
> 
> ------------------------------------------------------------------------
> 
> 2006-01-16  Amdrew Stubbs  <andrew.stubbs@st.com>
> 
> 	* gdbthread.h (prune_threads): Add prototype.
> 	* infrun.c (normal_stop): Call prune_threads().
> 	* thread.c (prune_threads): Remove 'static'.
> 
> Index: src/gdb/gdbthread.h
> ===================================================================
> --- src.orig/gdb/gdbthread.h	2006-01-16 16:08:57.000000000 +0000
> +++ src/gdb/gdbthread.h	2006-01-16 16:13:57.000000000 +0000
> @@ -80,6 +80,9 @@ extern struct thread_info *add_thread (p
>  /* Delete an existing thread list entry.  */
>  extern void delete_thread (ptid_t);
>  
> +/* Delete all dead threads from the list.  */
> +extern void prune_threads (void);
> +
>  /* Delete a step_resume_breakpoint from the thread database. */
>  extern void delete_step_resume_breakpoint (void *);
>  
> Index: src/gdb/infrun.c
> ===================================================================
> --- src.orig/gdb/infrun.c	2006-01-16 16:08:57.000000000 +0000
> +++ src/gdb/infrun.c	2006-01-16 16:13:57.000000000 +0000
> @@ -3037,6 +3037,9 @@ Further execution is probably impossible
>    if (stopped_by_random_signal)
>      disable_current_display ();
>  
> +  /* Delete any threads which have died.  */
> +  prune_threads ();
> +
>    /* Don't print a message if in the middle of doing a "step n"
>       operation for n > 1 */
>    if (step_multi && stop_step)
> Index: src/gdb/thread.c
> ===================================================================
> --- src.orig/gdb/thread.c	2006-01-16 16:08:57.000000000 +0000
> +++ src/gdb/thread.c	2006-01-16 16:13:57.000000000 +0000
> @@ -64,7 +64,6 @@ static void info_threads_command (char *
>  static void thread_apply_command (char *, int);
>  static void restore_current_thread (ptid_t);
>  static void switch_to_thread (ptid_t ptid);
> -static void prune_threads (void);
>  
>  void
>  delete_step_resume_breakpoint (void *arg)
> @@ -382,7 +381,7 @@ thread_alive (struct thread_info *tp)
>    return 1;
>  }
>  
> -static void
> +void
>  prune_threads (void)
>  {
>    struct thread_info *tp, *next;

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Fri, Jan 20, 2006 at 02:53:34PM +0000, Andrew STUBBS wrote:
> Any opinions anybody?

I'll be doing a lot of patch review this afternoon.  I'll take another
look at it.  It doesn't feel like the right place - normally we don't
need new calls to prune_thread... maybe we can "push" the death event.

(prune_threads -> expensive).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Mon, Jan 16, 2006 at 04:16:43PM +0000, Andrew STUBBS wrote:
> Andrew STUBBS wrote:
> >Now onto figuring out why it doesn't work....
> 
> The smoking gun seems to be here:
> 
> [From linux-thread-db.c]
> 
> static void
> detach_thread (ptid_t ptid, int verbose)
> {
>   struct thread_info *thread_info;
> 
>   if (verbose)
>     printf_unfiltered (_("[%s exited]\n"), target_pid_to_str (ptid));
> 
>   /* Don't delete the thread now, because it still reports as active
>      until it has executed a few instructions after the event
>      breakpoint - if we deleted it now, "info threads" would cause us
>      to re-attach to it.  Just mark it as having had a TD_DEATH
>      event.  This means that we won't delete it from our thread list
>      until we notice that it's dead (via prune_threads), or until
>      something re-uses its thread ID.  */
>   thread_info = find_thread_pid (ptid);
>   gdb_assert (thread_info != NULL);
>   thread_info->private->dying = 1;
> }

Nice catch - although there's a second smoking gun besides this one,
which is that linux-nat.c won't delete threads that are the current
inferior_ptid.  With good reason.  GDB absolutely falls apart when
you do that.

I think the long term fix for the latter is going to involve some
serious surgery on the notion of a current global inferior_ptid.
But until that happens, here's a workaround confined to the Linux
native code.  The big advantage over calling prune_threads is we don't
need to make a lot of expensive syscalls.

Any comments?  Does this work for you?  It does seem to avoid inserting
the breakpoint unnecessarily, as I'd expect.

-- 
Daniel Jacobowitz
CodeSourcery

2006-01-20  Daniel Jacobowitz  <dan@codesourcery.com>

	* linux-nat.c (struct saved_ptids, threads_to_delete)
	(record_dead_thread, prune_lwps, find_thread_from_lwp)
	(exit_lwp): New.
	(linux_nat_resume): Call prune_lwps.
	(wait_lwp, linux_nat_wait): Call exit_lwp.

Index: linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.36
diff -u -p -r1.36 linux-nat.c
--- linux-nat.c	4 Jan 2006 19:34:58 -0000	1.36
+++ linux-nat.c	20 Jan 2006 22:36:46 -0000
@@ -868,6 +868,92 @@ iterate_over_lwps (int (*callback) (stru
   return NULL;
 }
 
+/* Record a PTID for later deletion.  */
+
+struct saved_ptids
+{
+  ptid_t ptid;
+  struct saved_ptids *next;
+};
+static struct saved_ptids *threads_to_delete;
+
+static void
+record_dead_thread (ptid_t ptid)
+{
+  struct saved_ptids *p = xmalloc (sizeof (struct saved_ptids));
+  p->ptid = ptid;
+  p->next = threads_to_delete;
+  threads_to_delete = p;
+}
+
+/* Delete any dead threads which are not the current thread.  */
+
+static void
+prune_lwps (void)
+{
+  struct saved_ptids **p = &threads_to_delete;
+
+  while (*p)
+    if (! ptid_equal ((*p)->ptid, inferior_ptid))
+      {
+	struct saved_ptids *tmp = *p;
+	delete_thread (tmp->ptid);
+	*p = tmp->next;
+	xfree (tmp);
+      }
+    else
+      p = &(*p)->next;
+}
+
+/* Callback for iterate_over_threads that finds a thread corresponding
+   to the given LWP.  */
+
+static int
+find_thread_from_lwp (struct thread_info *thr, void *dummy)
+{
+  ptid_t *ptid_p = dummy;
+
+  if (GET_LWP (thr->ptid) && GET_LWP (thr->ptid) == GET_LWP (*ptid_p))
+    return 1;
+  else
+    return 0;
+}
+
+/* Handle the exit of a single thread LP.  */
+
+static void
+exit_lwp (struct lwp_info *lp)
+{
+  if (in_thread_list (lp->ptid))
+    {
+      /* Core GDB cannot deal with us deleting the current thread.  */
+      if (!ptid_equal (lp->ptid, inferior_ptid))
+	delete_thread (lp->ptid);
+      else
+	record_dead_thread (lp->ptid);
+      printf_unfiltered (_("[%s exited]\n"),
+			 target_pid_to_str (lp->ptid));
+    }
+  else
+    {
+      /* Even if LP->PTID is not in the global GDB thread list, the
+	 LWP may be - with an additional thread ID.  We don't need
+	 to print anything in this case; thread_db is in use and
+	 already took care of that.  But it didn't delete the thread
+	 in order to handle zombies correctly.  */
+
+      struct thread_info *thr;
+
+      thr = iterate_over_threads (find_thread_from_lwp, &lp->ptid);
+      if (thr && !ptid_equal (thr->ptid, inferior_ptid))
+	delete_thread (thr->ptid);
+      else
+	record_dead_thread (thr->ptid);
+    }
+
+  delete_lwp (lp->ptid);
+}
+
 /* Attach to the LWP specified by PID.  If VERBOSE is non-zero, print
    a message telling the user that a new LWP has been added to the
    process.  */
@@ -1121,6 +1207,8 @@ linux_nat_resume (ptid_t ptid, int step,
 			signo ? strsignal (signo) : "0",
 			target_pid_to_str (inferior_ptid));
 
+  prune_lwps ();
+
   /* A specific PTID means `step only this process id'.  */
   resume_all = (PIDGET (ptid) == -1);
 
@@ -1321,16 +1409,7 @@ wait_lwp (struct lwp_info *lp)
 
   if (thread_dead)
     {
-      if (in_thread_list (lp->ptid))
-	{
-	  /* Core GDB cannot deal with us deleting the current thread.  */
-	  if (!ptid_equal (lp->ptid, inferior_ptid))
-	    delete_thread (lp->ptid);
-	  printf_unfiltered (_("[%s exited]\n"),
-			     target_pid_to_str (lp->ptid));
-	}
-
-      delete_lwp (lp->ptid);
+      exit_lwp (lp);
       return 0;
     }
 
@@ -2117,16 +2196,6 @@ retry:
 	  /* Check if the thread has exited.  */
 	  if ((WIFEXITED (status) || WIFSIGNALED (status)) && num_lwps > 1)
 	    {
-	      if (in_thread_list (lp->ptid))
-		{
-		  /* Core GDB cannot deal with us deleting the current
-		     thread.  */
-		  if (!ptid_equal (lp->ptid, inferior_ptid))
-		    delete_thread (lp->ptid);
-		  printf_unfiltered (_("[%s exited]\n"),
-				     target_pid_to_str (lp->ptid));
-		}
-
 	      /* If this is the main thread, we must stop all threads and
 	         verify if they are still alive.  This is because in the nptl
 	         thread model, there is no signal issued for exiting LWPs
@@ -2148,7 +2217,7 @@ retry:
 				    "LLW: %s exited.\n",
 				    target_pid_to_str (lp->ptid));
 
-	      delete_lwp (lp->ptid);
+	      exit_lwp (lp);
 
 	      /* If there is at least one more LWP, then the exit signal
 	         was not the end of the debugged application and should be
@@ -2170,21 +2239,12 @@ retry:
 	     has stopped.  A similar check is made in stop_wait_callback().  */
 	  if (num_lwps > 1 && !linux_nat_thread_alive (lp->ptid))
 	    {
-	      if (in_thread_list (lp->ptid))
-		{
-		  /* Core GDB cannot deal with us deleting the current
-		     thread.  */
-		  if (!ptid_equal (lp->ptid, inferior_ptid))
-		    delete_thread (lp->ptid);
-		  printf_unfiltered (_("[%s exited]\n"),
-				     target_pid_to_str (lp->ptid));
-		}
 	      if (debug_linux_nat)
 		fprintf_unfiltered (gdb_stdlog,
 				    "LLW: %s exited.\n",
 				    target_pid_to_str (lp->ptid));
 
-	      delete_lwp (lp->ptid);
+	      exit_lwp (lp);
 
 	      /* Make sure there is at least one thread running.  */
 	      gdb_assert (iterate_over_lwps (running_callback, NULL));

Re: [PATCH] Disable thread specific breakpoints when thread dies

[Daniel Jacobowitz]

On Fri, Jan 20, 2006 at 05:41:15PM -0500, Daniel Jacobowitz wrote:
> Any comments?  Does this work for you?  It does seem to avoid inserting
> the breakpoint unnecessarily, as I'd expect.

> 2006-01-20  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* linux-nat.c (struct saved_ptids, threads_to_delete)
> 	(record_dead_thread, prune_lwps, find_thread_from_lwp)
> 	(exit_lwp): New.
> 	(linux_nat_resume): Call prune_lwps.
> 	(wait_lwp, linux_nat_wait): Call exit_lwp.

There weren't any comments; I've checked this in.

-- 
Daniel Jacobowitz
CodeSourcery