Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Fri, 2013-05-10 at 18:18 +0100, Pedro Alves wrote:
> On 05/10/2013 05:39 PM, Doug Evans wrote:
> > On Thu, May 9, 2013 at 2:56 PM, Philippe Waroquiers
> > <philippe.waroquiers@skynet.be> wrote:
> >> Index: gdb/breakpoint.c
> >> ===================================================================
> >> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> >> retrieving revision 1.761
> >> diff -u -p -r1.761 breakpoint.c
> >> --- gdb/breakpoint.c    7 May 2013 17:04:28 -0000       1.761
> >> +++ gdb/breakpoint.c    9 May 2013 21:46:33 -0000
> >> @@ -3537,6 +3537,9 @@ detach_breakpoints (ptid_t ptid)
> >>      if (bl->pspace != inf->pspace)
> >>        continue;
> >>
> >> +    if (bl->loc_type == bp_loc_other)
> >> +      continue;
> >> +
> >>      if (bl->inserted)
> >>        val |= remove_breakpoint_1 (bl, mark_inserted);
> >>    }
> > 
> > I think a comment is required here explaining *why* we continue for
> > bp_loc_other.
> > [Assuming the patch is correct ...]
Yes, adding a comment is a good idea.

> > 
> > However, there's nothing in "bp_loc_other" that says we should always
> > continue there.
> > Other breakpoint kinds are marked bp_loc_other too.
> 
> The other breakpoint kinds (software watchpoints, catchpoints,
> tracepoints) don't require detaching.  The state of bp_loc_other
> breakpoints, at least at present, is always on the GDB side.
> Detaching is required for those breakpoints that is assumed
> get auto-cloned by the target/kernel to forked children.
> 
> > Plus avoiding calling remove_breakpoint_1 feels like working around the problem.
> > This doesn't feel like the right fix.
> 
> GDB doesn't have an inferior or any other state corresponding
> to the process whose breakpoints are being detached.
> 
> An alternative I imagine would be something like adding
> "detach breakpoint" target methods (and bl->owner->ops->detach_location,
> etc.) and call that instead of remove_breakpoint_1, though it
> seems like we'd get the same result (with the present state).  But
> I won't object to trying that.
I do not master much of breakpoint.h/.c but it looks to
me that this implies to add quite some code which will
at the end either do nothing (for bp_loc_other) or
call remove_breakpoint_1 (for others).

What would be the advantages of the detach_breakpoint
and detach_location target methods ?
As long as there is no need (yet) for a different "detach"
behaviour depending on specialised bp_location, it looks
to me that the single "if" is simpler and corresponds to
the description of detach_breakpoints in breakpoint.h.
(maybe we just have to add 'software_breakpoint' and
'single_step_breakpoint' in the description in breakpoint.h ?)

Or do I miss something about the interest/need for detach_* methods ?

Philippe