From: Pedro Alves Date: Tue, 28 Oct 2014 13:42:11 +0000 (+0000) Subject: PR gdb/12623: non-stop crashes inferior, PC adjustment and 1-byte insns X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=7f5ef60532b466ec7a83a943f36e93e32e30eafe;p=binutils-gdb.git 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. --- diff --git a/gdb/ChangeLog b/gdb/ChangeLog index b358dd71805..9901d164ba7 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,14 @@ +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. + 2014-10-27 Pedro Alves * remote.c (remote_thread_alive): New, factored out from ... diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index fb47baebda7..4fd5f69283f 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -201,6 +201,11 @@ struct thread_info SIGTRAP from a breakpoint SIGTRAP. */ CORE_ADDR prev_pc; + /* Did we set the thread stepping a breakpoint instruction? This is + used in conjunction with PREV_PC to decide whether to adjust the + PC. */ + int stepped_breakpoint; + /* Should we step over breakpoint next time keep_going is called? */ int stepping_over_breakpoint; diff --git a/gdb/infrun.c b/gdb/infrun.c index df053e26ffe..7af5e0f58c3 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2213,12 +2213,35 @@ a command like `return' or `jump' to continue execution.")); resume_ptid = inferior_ptid; } - if (gdbarch_cannot_step_breakpoint (gdbarch)) - { + if (execution_direction != EXEC_REVERSE + && step && breakpoint_inserted_here_p (aspace, pc)) + { + /* The only case we currently need to step a breakpoint + instruction is when we have a signal to deliver. See + handle_signal_stop where we handle random signals that could + take out us out of the stepping range. Normally, in that + case we end up continuing (instead of stepping) over the + signal handler with a breakpoint at PC, but there are cases + where we should _always_ single-step, even if we have a + step-resume breakpoint, like when a software watchpoint is + set. Assuming single-stepping and delivering a signal at the + same time would takes us to the signal handler, then we could + have removed the breakpoint at PC to step over it. However, + some hardware step targets (like e.g., Mac OS) can't step + into signal handlers, and for those, we need to leave the + breakpoint at PC inserted, as otherwise if the handler + recurses and executes PC again, it'll miss the breakpoint. + So we leave the breakpoint inserted anyway, but we need to + record that we tried to step a breakpoint instruction, so + that adjust_pc_after_break doesn't end up confused. */ + gdb_assert (sig != GDB_SIGNAL_0); + + tp->stepped_breakpoint = 1; + /* Most targets can step a breakpoint instruction, thus executing it normally. But if this one cannot, just continue and we will hit it anyway. */ - if (step && breakpoint_inserted_here_p (aspace, pc)) + if (gdbarch_cannot_step_breakpoint (gdbarch)) step = 0; } @@ -3221,6 +3244,7 @@ set_step_info (struct frame_info *frame, struct symtab_and_line sal) void init_thread_stepping_state (struct thread_info *tss) { + tss->stepped_breakpoint = 0; tss->stepping_over_breakpoint = 0; tss->stepping_over_watchpoint = 0; tss->step_after_step_resume_breakpoint = 0; @@ -3385,7 +3409,8 @@ adjust_pc_after_break (struct execution_control_state *ecs) if (thread_has_single_step_breakpoints_set (ecs->event_thread) || !ptid_equal (ecs->ptid, inferior_ptid) || !currently_stepping (ecs->event_thread) - || ecs->event_thread->prev_pc == breakpoint_pc) + || (ecs->event_thread->stepped_breakpoint + && ecs->event_thread->prev_pc == breakpoint_pc)) regcache_write_pc (regcache, breakpoint_pc); do_cleanups (old_cleanups); @@ -4241,6 +4266,7 @@ handle_signal_stop (struct execution_control_state *ecs) return; } + ecs->event_thread->stepped_breakpoint = 0; ecs->event_thread->stepping_over_breakpoint = 0; ecs->event_thread->stepping_over_watchpoint = 0; bpstat_clear (&ecs->event_thread->control.stop_bpstat); diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index cada90d15ea..5d2d265393e 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,17 @@ +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. + 2014-10-28 Pedro Alves PR gdb/17511 diff --git a/gdb/testsuite/gdb.base/sigstep.c b/gdb/testsuite/gdb.base/sigstep.c index 38dd1fb1c2e..bc0f67b307f 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); diff --git a/gdb/testsuite/gdb.base/sigstep.exp b/gdb/testsuite/gdb.base/sigstep.exp index 08ad828f121..c4580a7a672 100644 --- a/gdb/testsuite/gdb.base/sigstep.exp +++ b/gdb/testsuite/gdb.base/sigstep.exp @@ -476,18 +476,37 @@ foreach cmd {"step" "next" "continue"} { # Try stepping when there's a signal pending, and a pre-existing # breakpoint at the current instruction, and no breakpoint in the -# handler. Should advance to the next line/instruction. - -proc breakpoint_over_handler { cmd } { +# handler. Should advance to the next line/instruction. If SW_WATCH +# is true, set a software watchpoint, which exercises stepping the +# breakpoint instruction while delivering a signal at the same time. +# If NO_HANDLER, arrange for the signal's handler be SIG_IGN, thus +# when the software watchpoint is also set, testing stepping a +# breakpoint instruction and immediately triggering the breakpoint +# (exercises adjust_pc_after_break logic). + +proc breakpoint_over_handler { cmd with_sw_watch no_handler } { global infinite_loop global clear_done - with_test_prefix "$cmd on breakpoint, skip handler" { + set prefix "$cmd on breakpoint, skip handler" + if { $with_sw_watch } { + append prefix ", with sw-watchpoint" + } + if { $no_handler } { + append prefix ", no handler" + } + + with_test_prefix "$prefix" { restart # Use the real-time itimer, as otherwize the process never gets # enough time to expire the timer. gdb_test_no_output "set itimer = itimer_real" + if {$no_handler} { + gdb_test "print no_handler = 1" " = 1" \ + "set no_handler" + } + gdb_test "break $infinite_loop" ".*" "break infinite loop" gdb_test "break $clear_done" ".*" "break clear done" @@ -498,10 +517,26 @@ proc breakpoint_over_handler { cmd } { # Make the signal pending sleep 1 + if { $with_sw_watch } { + # A watchpoint on a convenience variable is always a + # software watchpoint. + gdb_test "watch \$convenience" "Watchpoint .*: \\\$convenience" + } + + if {$no_handler} { + # With no handler, we need to set the global ourselves + # manually. + gdb_test "print done = 1" " = 1" "set done" + } + test_skip_handler $cmd } } foreach cmd {"stepi" "nexti" "step" "next" "continue"} { - breakpoint_over_handler $cmd + foreach with_sw_watch {0 1} { + foreach no_handler {0 1} { + breakpoint_over_handler $cmd $with_sw_watch $no_handler + } + } } diff --git a/gdb/testsuite/gdb.base/step-sw-breakpoint-adjust-pc.c b/gdb/testsuite/gdb.base/step-sw-breakpoint-adjust-pc.c new file mode 100644 index 00000000000..f7b6e70a697 --- /dev/null +++ b/gdb/testsuite/gdb.base/step-sw-breakpoint-adjust-pc.c @@ -0,0 +1,50 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2014 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +/* An instruction with the same length as decr_pc_after_break. This + is 1-byte on x86. */ +#define INSN asm ("nop") + +void +test_user_bp (void) +{ + INSN; /* break for user-bp test here */ + INSN; /* insn1 */ + INSN; /* insn2 */ + INSN; /* insn3 */ +} + +void +foo (void) +{ +} + +void +test_step_resume (void) +{ + foo (); /* break for step-resume test here */ + INSN; /* insn1 */ + INSN; /* insn2 */ +} + +int +main (void) +{ + test_user_bp (); + test_step_resume (); + return 0; +} diff --git a/gdb/testsuite/gdb.base/step-sw-breakpoint-adjust-pc.exp b/gdb/testsuite/gdb.base/step-sw-breakpoint-adjust-pc.exp new file mode 100644 index 00000000000..4d08a4418ed --- /dev/null +++ b/gdb/testsuite/gdb.base/step-sw-breakpoint-adjust-pc.exp @@ -0,0 +1,94 @@ +# This testcase is part of GDB, the GNU debugger. + +# Copyright 2014 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# Test tstepping an instruction just as long as decr_pc_after_break +# after removing a breakpoint at PC. GDB used to get confused with +# this in non-stop mode, and adjust the PC incorrectly. PR gdb/12623. + +standard_testfile + +if [build_executable "failed to build" ${testfile} ${srcfile} {debug}] { + return -1 +} + +set linenum_for_user_bp [gdb_get_line_number "break for user-bp test here"] +set linenum_for_step_resume [gdb_get_line_number "break for step-resume test here"] + +proc test {non_stop displaced always_inserted} { + global binfile + global linenum_for_user_bp + global linenum_for_step_resume + + clean_restart $binfile + + gdb_test_no_output "set non-stop $non_stop" + gdb_test_no_output "set displaced-stepping $displaced" + gdb_test_no_output "set breakpoint always-inserted $always_inserted" + + if ![runto_main] { + return -1 + } + + with_test_prefix "user bp" { + delete_breakpoints + + gdb_breakpoint $linenum_for_user_bp + gdb_continue_to_breakpoint "continue to breakpoint" + + # If breakpoint always-inserted is on, this makes the location + # moribund. + delete_breakpoints + + gdb_test "si" "INSN.*insn1.*" "si advances" + } + + with_test_prefix "step-resume" { + delete_breakpoints + + gdb_breakpoint $linenum_for_step_resume + gdb_continue_to_breakpoint "continue to breakpoint" + + gdb_test "next" "insn1.*" + + # We're now stopped where the step-resume breakpoint for the + # previous "next" was. That breakpoint was removed and is now + # on the moribund locations list. + gdb_test "si" "INSN.*insn2.*" "si advances" + + delete_breakpoints + } +} + +# Wrapper for foreach that calls with_test_prefix on each iteration, +# including the iterator's current value in the prefix. + +proc foreach_with_prefix {var list body} { + upvar 1 $var myvar + foreach myvar $list { + with_test_prefix "$var=$myvar" { + uplevel 1 $body + } + } +} + +foreach_with_prefix non_stop { "off" "on" } { + foreach_with_prefix displaced_step { "off" "on" } { + foreach_with_prefix always_inserted { "off" "on" } { + test $non_stop $displaced_step $always_inserted + } + } +}