From 7f5ef60532b466ec7a83a943f36e93e32e30eafe Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 28 Oct 2014 13:42:11 +0000 Subject: PR gdb/12623: non-stop crashes inferior, PC adjustment and 1-byte insns TL;DR - if we step an instruction that is as long as decr_pc_after_break (1-byte on x86) right after removing the breakpoint at PC, in non-stop mode, adjust_pc_after_break adjusts the PC, but it shouldn't. In non-stop mode, when a breakpoint is removed, it is moved to the "moribund locations" list. This is because other threads that are running may have tripped on that breakpoint as well, and we haven't heard about it. When a trap is reported, we check if perhaps it was such a deleted breakpoint that caused the trap. If so, we also need to adjust the PC (decr_pc_after_break). Now, say that, on x86: - a breakpoint was placed at an address where we have an instruction of the same length as decr_pc_after_break on this arch (1 on x86). - the breakpoint is removed, and thus put on the moribund locations list. - the thread is single-stepped. As there's no breakpoint inserted at PC anymore, the single-step actually executes the 1-byte instruction normally. GDB should _not_ adjust the PC for the resulting SIGTRAP. But, adjust_pc_after_break confuses the step SIGTRAP reported for this single-step as being a SIGTRAP for the moribund location of the breakpoint that used to be at the previous PC, and so infrun applies the decr_pc_after_break adjustment incorrectly. The confusion comes from the special case mentioned in the comment: static void adjust_pc_after_break (struct execution_control_state *ecs) { ... As a special case, we could have hardware single-stepped a software breakpoint. In this case (prev_pc == breakpoint_pc), we also need to back up to the breakpoint address. */ if (thread_has_single_step_breakpoints_set (ecs->event_thread) || !ptid_equal (ecs->ptid, inferior_ptid) || !currently_stepping (ecs->event_thread) || (ecs->event_thread->stepped_breakpoint && ecs->event_thread->prev_pc == breakpoint_pc)) regcache_write_pc (regcache, breakpoint_pc); The condition that incorrectly triggers is the "ecs->event_thread->prev_pc == breakpoint_pc" one. Afterwards, the next resume resume re-executes an instruction that had already executed, which if you're lucky, results in the inferior crashing. If you're unlucky, you'll get silent bad behavior... The fix is to remember that we stepped a breakpoint. Turns out the only case we step a breakpoint instruction today isn't covered by the testsuite. It's the case of a 'handle nostop" signal arriving while a step is in progress _and_ we have a software watchpoint, which forces always single-stepping. This commit extends sigstep.exp to cover that, and adds a new test for the adjust_pc_after_break issue. Tested on x86_64 Fedora 20, native and gdbserver. gdb/ 2014-10-28 Pedro Alves PR gdb/12623 * gdbthread.h (struct thread_info) : New field. * infrun.c (resume) : Set the thread's stepped_breakpoint field. Skip if reverse debugging. Add comment. (init_thread_stepping_state, handle_signal_stop): Clear the thread's stepped_breakpoint field. gdb/testsuite/ 2014-10-28 Pedro Alves PR gdb/12623 * gdb.base/sigstep.c (no_handler): New global. (main): If 'no_handler is true, set the signal handlers to SIG_IGN. * gdb.base/sigstep.exp (breakpoint_over_handler): Add with_sw_watch and no_handler parameters. Handle them. (top level) : Add a test axis for testing with a software watchpoint, and another for testing with the signal handler set to SIG_IGN. * gdb.base/step-sw-breakpoint-adjust-pc.c: New file. * gdb.base/step-sw-breakpoint-adjust-pc.exp: New file. --- gdb/testsuite/gdb.base/sigstep.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'gdb/testsuite/gdb.base/sigstep.c') diff --git a/gdb/testsuite/gdb.base/sigstep.c b/gdb/testsuite/gdb.base/sigstep.c index 38dd1fb..bc0f67b 100644 --- a/gdb/testsuite/gdb.base/sigstep.c +++ b/gdb/testsuite/gdb.base/sigstep.c @@ -25,6 +25,7 @@ static volatile int done; static volatile int dummy; +static volatile int no_handler; static void handler (int sig) @@ -49,11 +50,11 @@ enum { int main () { - int res; + /* Set up the signal handler. */ memset (&action, 0, sizeof (action)); - action.sa_handler = handler; + action.sa_handler = no_handler ? SIG_IGN : handler; sigaction (SIGVTALRM, &action, NULL); sigaction (SIGALRM, &action, NULL); -- cgit v1.1