Re: [PATCH] Honour SIGILL and SIGSEGV in cancel breakpoint

[Pedro Alves <palves@redhat.com>]

On 09/14/2014 01:06 PM, Yao Qi wrote:
> I see the following fail on are-none-linux-gnueabi testing,
> 
> (gdb) continue^M
> Continuing.^M
> ^M
> Program received signal SIGILL, Illegal instruction.^M
> [Switching to Thread 1003]^M
> handler (signo=10) at
> /scratch/yqi/arm-none-linux-gnueabi/src/gdb-trunk/gdb/testsuite/gdb.threads/sigstep-threads.c:33^M
> 33        tgkill (getpid (), gettid (), SIGUSR1);       /* step-2 */^M
> (gdb) FAIL: gdb.threads/sigstep-threads.exp: continue
> 
> the cause is that GDBserver doesn't cancel the breakpoint if the stop
> signal is SIGILL.  The kernel used here is a little old, 2.6.x, and
> doesn't translate SIGILL to SIGTRAP when program hits breakpoint
> instruction (which is an illegal instruction actually).  GDB and
> GDBserver can translate SIGILL to SIGTRAP under certain circumstance,
> so it is not a problem here.  See gdbserver/linux-low.c:linux_wait_1
> 
>   /* If this event was not handled before, and is not a SIGTRAP, we
>      report it.  SIGILL and SIGSEGV are also treated as traps in case
>      a breakpoint is inserted at the current PC.  If this target does
>      not support internal breakpoints at all, we also report the
>      SIGTRAP without further processing; it's of no concern to us.  */
>   maybe_internal_trap
>     = (supports_breakpoints ()
>        && (WSTOPSIG (w) == SIGTRAP
> 	   || ((WSTOPSIG (w) == SIGILL
> 		|| WSTOPSIG (w) == SIGSEGV)
> 	       && (*the_low_target.breakpoint_at) (event_child->stop_pc))));
> 
> However, SIGILL and SIGSEGV is not considered when cancelling
> breakpoint, which causes the fail above.  That is, when GDB is doing
> software single step on address ADDR, both thread A and thread B hits the
> software single step breakpoint, and get SIGILL.  GDB selects the event
> from thread A, removes the software single step breakpoint, and resume
> the program.  The event (SIGILL) from thread B is reported to GDB, but
> GDB doesn't regard this SIGILL as SIGTRAP, because the breakpoint on
> address ADDR was removed, so GDB reports "Program received signal
> SIGILL".
> 
> The patch is to allow calling cancel_breakpoint if the signal is
> SIGILL and SIGSEGV.  This patch fixes the fail above.
> 
> Regression tested on arm-none-linux-gnueabi and x86_64-linux.
> 
> gdb/gdbserver:
> 
> 2014-09-12  Yao Qi  <yao@codesourcery.com>
> 
> 	* linux-low.c (cancel_breakpoints_callback): Allow calling
> 	cancel_breakpoint if stop signal is SIGILL or SIGSEGV.
> 	(linux_low_filter_event): Likewise.
> ---
>  gdb/gdbserver/linux-low.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index ec3260e..e2c9814 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -1936,7 +1936,12 @@ 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
> +		  && (WSTOPSIG (wstat) == SIGTRAP
> +		      /* SIGILL and SIGSEGV are also treated as traps in
> +			 case a breakpoint is inserted at the current PC,
> +			 which is checked in cancel_breakpoints below.  */
> +		      || WSTOPSIG (wstat) == SIGILL
> +		      || WSTOPSIG (wstat) == SIGSEGV)
>  		  && cancel_breakpoint (child))
>  		{
>  		  /* Throw away the SIGTRAP.  */
> @@ -2273,7 +2278,12 @@ cancel_breakpoints_callback (struct inferior_list_entry *entry, void *data)
>        && thread->last_status.kind == TARGET_WAITKIND_IGNORE
>        && lp->status_pending_p
>        && WIFSTOPPED (lp->status_pending)
> -      && WSTOPSIG (lp->status_pending) == SIGTRAP
> +      && (WSTOPSIG (lp->status_pending) == SIGTRAP
> +	  /* SIGILL and SIGSEGV are also treated as traps in case a
> +	     breakpoint is inserted at the current PC, which is checked
> +	     in cancel_breakpoints below.  */
> +	  || WSTOPSIG (lp->status_pending) == SIGILL
> +	  || WSTOPSIG (lp->status_pending) == SIGSEGV)
>        && !lp->stepping
>        && !lp->stopped_by_watchpoint
>        && cancel_breakpoint (lp))
> 

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).

Thanks,
Pedro Alves