Honour SIGILL and SIGSEGV in cancel breakpoint

  Pedro Alves <palves@redhat.com> writes:

> Instead of duplicating the code and comments, please factor out
> the SIGTRAP+SIGILL+SIGSEGVs checks to a helper function.  On the GDB side,
> we have linux_nat_lp_status_is_event, and we see that it's also used
> on count_count_events_callback (which gdbserver also has), which makes
> sense, as it's counting threads that had breakpoint SIGTRAP-ish
> events (though I'm not sure why we only consider breakpoints when
> selecting the event lwp).

I take a look at linux_nat_lp_status_is_event and email discussions on
adding this function <https://sourceware.org/ml/gdb-patches/2010-07/msg00275.html>,
a new function lp_status_is_sigtrap_like_event is added.  I don't use
the same name because I feel linux_nat_lp_status_is_event isn't clear
enough.  Secondly, I don't use "waitstatus.kind == TARGET_WAITKIND_IGNORE"
condition check inside lp_status_is_sigtrap_like_event, because IMO it
was used in linux_nat_lp_status_is_event due to lack of lp->status_p
flag, as the comments described.  However, in GDBserver, we have
status_pending_p flag, so "waitstatus.kind == TARGET_WAITKIND_IGNORE" is
not needed.

count_events_callback and select_event_lwp_callback in GDBServer need to
honour SIGILL and SIGSEGV too.  I write a patch to call
lp_status_is_sigtrap_like_event in them, but regression test result
isn't changed, which is a surprise to me.  I thought some fails should
be fixed.  I'll look into it deeply.

I post the updated patch to fix the issue we've seen on canceling breakpoint.
  

On 09/18/2014 03:30 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Instead of duplicating the code and comments, please factor out
>> the SIGTRAP+SIGILL+SIGSEGVs checks to a helper function.  On the GDB side,
>> we have linux_nat_lp_status_is_event, and we see that it's also used
>> on count_count_events_callback (which gdbserver also has), which makes
>> sense, as it's counting threads that had breakpoint SIGTRAP-ish
>> events (though I'm not sure why we only consider breakpoints when
>> selecting the event lwp).
> 
> I take a look at linux_nat_lp_status_is_event and email discussions on
> adding this function <https://sourceware.org/ml/gdb-patches/2010-07/msg00275.html>,
> a new function lp_status_is_sigtrap_like_event is added.  

I think something with "breakpoint" in the name,
like lp_status_maybe_breakpoint would be even clearer.  The event is
SIGTRAP-like only in the sense that it may signal a breakpoint like
SIGTRAP does.  A SIGILL is not sigtrap-like for single-steps, for example.

> I don't use
> the same name because I feel linux_nat_lp_status_is_event isn't clear
> enough.  Secondly, I don't use "waitstatus.kind == TARGET_WAITKIND_IGNORE"
> condition check inside lp_status_is_sigtrap_like_event, because IMO it
> was used in linux_nat_lp_status_is_event due to lack of lp->status_p
> flag, as the comments described.  However, in GDBserver, we have
> status_pending_p flag, so "waitstatus.kind == TARGET_WAITKIND_IGNORE" is
> not needed.
> 
> count_events_callback and select_event_lwp_callback in GDBServer need to
> honour SIGILL and SIGSEGV too.  I write a patch to call
> lp_status_is_sigtrap_like_event in them, but regression test result
> isn't changed, which is a surprise to me.  I thought some fails should
> be fixed.  I'll look into it deeply.

Maybe you're getting lucky with scheduling.
pthreads.exp and schedlock.exp I think are the most sensitive to this.

See:
 https://www.sourceware.org/ml/gdb-patches/2001-06/msg00250.html

Thanks,
Pedro Alves