[rfc] Delay deletion of step-resume breakpoints

[rfc] Delay deletion of step-resume breakpoints

[Daniel Jacobowitz]

This patch addresses a problem I encountered a long time ago.
Unfortunately I can't find the testcase, and all my attempts to write
a new one have failed.  But, logically, I'm pretty sure the problem
still exists - it's just hard to trigger.

delete_thread calls free_thread, which calls delete_breakpoint.  But
delete_thread is called from a number of places during target_wait on
various targets - when the program might not be stopped, so deleting
the breakpoint will fail.  This patch changes the delete_breakpoint
call to set disp_del_at_next_stop instead.  The breakpoint will stick
around a little longer, but still be collected.

The problem with writing a test case is that you need a step resume
breakpoint in a non-current thread when that thread exits.  This is
timing sensitive and I haven't had any luck reproducing it today.

Does anyone else think this patch is right (or wrong)?

-- 
Daniel Jacobowitz
CodeSourcery

2007-08-13  Daniel Jacobowitz  <dan@codesourcery.com>

	* thread.c (free_thread): Do not delete the step resume breakpoint
	right away.

Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.53
diff -u -p -r1.53 thread.c
--- thread.c	10 Apr 2007 14:53:46 -0000	1.53
+++ thread.c	13 Aug 2007 20:42:58 -0000
@@ -87,9 +87,11 @@ static void
 free_thread (struct thread_info *tp)
 {
   /* NOTE: this will take care of any left-over step_resume breakpoints,
-     but not any user-specified thread-specific breakpoints. */
+     but not any user-specified thread-specific breakpoints.  We can not
+     delete the breakpoint straight-off, because the inferior might not
+     be stopped at the moment.  */
   if (tp->step_resume_breakpoint)
-    delete_breakpoint (tp->step_resume_breakpoint);
+    tp->step_resume_breakpoint->disposition = disp_del_at_next_stop;
 
   /* FIXME: do I ever need to call the back-end to give it a
      chance at this private data before deleting the thread?  */

Re: [rfc] Delay deletion of step-resume breakpoints

[Mark Kettenis]

> Date: Mon, 13 Aug 2007 17:03:26 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> The problem with writing a test case is that you need a step resume
> breakpoint in a non-current thread when that thread exits.  This is
> timing sensitive and I haven't had any luck reproducing it today.
> 
> Does anyone else think this patch is right (or wrong)?

The change you made to the comment:

>    /* NOTE: this will take care of any left-over step_resume breakpoints,
> -     but not any user-specified thread-specific breakpoints. */
> +     but not any user-specified thread-specific breakpoints.  We can not
> +     delete the breakpoint straight-off, because the inferior might not
> +     be stopped at the moment.  */

Makes me suspect this is just a workaround for another bug, the bug
being that the inferior isn't properly stopped when this code gets
called.

Mark

Re: [rfc] Delay deletion of step-resume breakpoints

[Daniel Jacobowitz]

On Mon, Aug 13, 2007 at 11:13:32PM +0200, Mark Kettenis wrote:
> The change you made to the comment:
> 
> >    /* NOTE: this will take care of any left-over step_resume breakpoints,
> > -     but not any user-specified thread-specific breakpoints. */
> > +     but not any user-specified thread-specific breakpoints.  We can not
> > +     delete the breakpoint straight-off, because the inferior might not
> > +     be stopped at the moment.  */
> 
> Makes me suspect this is just a workaround for another bug, the bug
> being that the inferior isn't properly stopped when this code gets
> called.

That's supposed to be not a bug - I think.  We've detected a thread
exit while waiting for an interesting event; since thread exit isn't
generally considered "interesting" in this context, we keep waiting.
If your OS stops all threads for you at every event (HP/UX ttrace
does, I think) then that doesn't matter.  If your OS doesn't (Solaris
PR_ASYNC, GNU/Linux) then you might detect that thread exit while the
other threads are not yet stopped.

Or something else might have already resumed the other threads; I am
working on remote protocol notifications for stopped threads, and I
don't want to leave the entire target stopped while waiting for GDB to
ack.  Jim's come up with a sensible design I can reuse for this,
we'll post it soon I hope.

So we can delay deleting the thread until the next time we need to
stop all threads, but that's awkward - see record_dead_thread in
linux-nat.c.  Or we can let delete_thread handle the problem.

So should delete_thread be callable when the target is running or
should any target that might call it when the target is not running
maintain its own list the way the Linux native layer does?  We don't
have any formal specifications on what can be called when :-(

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Delay deletion of step-resume breakpoints

[Daniel Jacobowitz]

On Mon, Aug 13, 2007 at 05:37:15PM -0400, Daniel Jacobowitz wrote:
> On Mon, Aug 13, 2007 at 11:13:32PM +0200, Mark Kettenis wrote:
> > The change you made to the comment:
> > 
> > >    /* NOTE: this will take care of any left-over step_resume breakpoints,
> > > -     but not any user-specified thread-specific breakpoints. */
> > > +     but not any user-specified thread-specific breakpoints.  We can not
> > > +     delete the breakpoint straight-off, because the inferior might not
> > > +     be stopped at the moment.  */
> > 
> > Makes me suspect this is just a workaround for another bug, the bug
> > being that the inferior isn't properly stopped when this code gets
> > called.
> 
> That's supposed to be not a bug - I think.

Hi Mark,

Did you have any comments on my response?  Was it sensible or garbage?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Delay deletion of step-resume breakpoints

[Eli Zaretskii]

> Date: Mon, 13 Aug 2007 23:13:32 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: gdb-patches@sourceware.org
> 
> >    /* NOTE: this will take care of any left-over step_resume breakpoints,
> > -     but not any user-specified thread-specific breakpoints. */
> > +     but not any user-specified thread-specific breakpoints.  We can not
> > +     delete the breakpoint straight-off, because the inferior might not
> > +     be stopped at the moment.  */
> 
> Makes me suspect this is just a workaround for another bug, the bug
> being that the inferior isn't properly stopped when this code gets
> called.

Actually, I think that's TRT to do in _all_ cases: we are scheduling a
breakpoint delete the next time GDB is in a predictable state to do
that.

That's exactly analogous to what a Posix filesystem does when you
delete a file: if some process has that file open, the file is
scheduled to be deleted when its last handle is closed.

So I think Daniel's patch makes good sense.

Re: [rfc] Delay deletion of step-resume breakpoints

[Daniel Jacobowitz]

On Mon, Aug 13, 2007 at 05:03:26PM -0400, Daniel Jacobowitz wrote:
> 2007-08-13  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* thread.c (free_thread): Do not delete the step resume breakpoint
> 	right away.

I've committed this.

-- 
Daniel Jacobowitz
CodeSourcery