diff mbox

Honour SIGILL and SIGSEGV in cancel breakpoint

Message ID 87mw9xzmlr.fsf@codesourcery.com
State New
Headers show

Commit Message

Yao Qi Sept. 18, 2014, 2:30 a.m. UTC
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.

Comments

Pedro Alves Sept. 19, 2014, 5:04 p.m. UTC | #1
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
diff mbox

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index ec3260e..9c9a303 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -1739,6 +1739,20 @@  cancel_breakpoint (struct lwp_info *lwp)
   return 0;
 }
 
+/* Check for SIGTRAP-like events in LP.  */
+
+static int
+lp_status_is_sigtrap_like_event (struct lwp_info *lp)
+{
+  return (lp->status_pending_p
+	  && WIFSTOPPED (lp->status_pending)
+	  && (WSTOPSIG (lp->status_pending) == SIGTRAP
+	      /* SIGILL and SIGSEGV are also treated as traps in case a
+		 breakpoint is inserted at the current PC.  */
+	      || WSTOPSIG (lp->status_pending) == SIGILL
+	      || WSTOPSIG (lp->status_pending) == SIGSEGV));
+}
+
 /* Do low-level handling of the event, and check if we should go on
    and pass it to caller code.  Return the affected lwp if we are, or
    NULL otherwise.  */
@@ -1936,7 +1950,7 @@  linux_low_filter_event (ptid_t filter_ptid, int lwpid, int wstat)
 		 the core before this one is handled.  All-stop always
 		 cancels breakpoint hits in all threads.  */
 	      if (non_stop
-		  && WSTOPSIG (wstat) == SIGTRAP
+		  && lp_status_is_sigtrap_like_event (child)
 		  && cancel_breakpoint (child))
 		{
 		  /* Throw away the SIGTRAP.  */
@@ -2271,9 +2285,7 @@  cancel_breakpoints_callback (struct inferior_list_entry *entry, void *data)
 
   if (thread->last_resume_kind != resume_stop
       && thread->last_status.kind == TARGET_WAITKIND_IGNORE
-      && lp->status_pending_p
-      && WIFSTOPPED (lp->status_pending)
-      && WSTOPSIG (lp->status_pending) == SIGTRAP
+      && lp_status_is_sigtrap_like_event (lp)
       && !lp->stepping
       && !lp->stopped_by_watchpoint
       && cancel_breakpoint (lp))