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

Pedro Alves
2014-09-16T12:13:01Z

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