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

[Pedro Alves <palves@redhat.com>]

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:
>> The attached patch fixes a gdb_assert caused by the combination of catch
>> signal and fork:
>>   break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed.
>>
>> The problem is that the signal_catch_counts is decremented by detach_breakpoints.
>> The fix consists in not detaching breakpoint locations of type bp_loc_other.
>> The patch introduces a new test.
>>
>> Ok to commit ?
>>
>> Index: gdb/ChangeLog
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/ChangeLog,v
>> retrieving revision 1.15539
>> diff -u -p -r1.15539 ChangeLog
>> --- gdb/ChangeLog       9 May 2013 18:03:27 -0000       1.15539
>> +++ gdb/ChangeLog       9 May 2013 21:46:32 -0000
>> @@ -1,3 +1,8 @@
>> +2013-05-09  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>> +
>> +       * breakpoints.c (detach_breakpoints): Do not
>> +       detach breakpoints locations with loc_type bp_loc_other.
>> +
>>  2013-05-09  Doug Evans  <dje@google.com>
>>
>>         * symfile.c (syms_from_objfile_1): Delete args offsets, num_offsets.
>> 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 ...]
> 
> 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.

-- 
Pedro Alves