From 483805cf9ea5a6dace41415d8830e93fccc49c43 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 22 Apr 2014 15:00:56 +0100 Subject: [PATCH] Consecutive step-overs trigger internal error. If a thread trips on a breakpoint that needs stepping over just after finishing a step over, GDB currently fails an assertion. This is a regression caused by the "Handle multiple step-overs." patch (99619beac6252113fed212fdb9e1ab97bface423) at https://sourceware.org/ml/gdb-patches/2014-02/msg00765.html. (gdb) x /4i $pc => 0x400540 : movl $0x0,0x2003da(%rip) # 0x600924 0x40054a : movl $0x1,0x2003d0(%rip) # 0x600924 0x400554 : movl $0x2,0x2003c6(%rip) # 0x600924 0x40055e : movl $0x3,0x2003bc(%rip) # 0x600924 (gdb) PASS: gdb.base/consecutive-step-over.exp: get breakpoint addresses break *0x40054a Breakpoint 2 at 0x40054a: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 23. (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 1: set breakpoint condition $bpnum condition (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 1: set condition break *0x400554 Breakpoint 3 at 0x400554: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 24. (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 2: set breakpoint condition $bpnum condition (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 2: set condition break *0x40055e Breakpoint 4 at 0x40055e: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 25. (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 3: set breakpoint condition $bpnum condition (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 3: set condition break 27 Breakpoint 5 at 0x400568: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 27. (gdb) continue Continuing. ../../src/gdb/infrun.c:5200: internal-error: switch_back_to_stepped_thread: Assertion `!tp->control.trap_expected' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. FAIL: gdb.base/consecutive-step-over.exp: continue to breakpoint: break here (GDB internal error) The assertion fails, because the code is not expecting that the event thread itself might need another step over. IOW, not expecting that TP in: tp = find_thread_needs_step_over (stepping_thread != NULL, stepping_thread); could be the event thread. A small fix for this would be to clear the event thread's trap_expected earlier, before asserting. But looking deeper, although currently_stepping_or_nexting_callback's intention is finding the thread that is doing a step/next, it also returns the thread that is doing a step-over dance, with trap_expected set. If there ever was a reason for that (it was I who added currently_stepping_or_nexting_callback , but I can't recall why I put trap_expected there in the first place), the only remaining reason nowadays is to aid in implementing switch_back_to_stepped_thread's assertion that is now triggering, by piggybacking on the walk over all threads, thus avoiding a separate walk. This is quite obscure, and I think we can do even better, by merging the walks that look for the stepping thread, and the walk that looks for some thread that might need a step over. Tested on x86_64 Fedora 17, native and gdbserver, and also native on top of my "software single-step on x86_64" series. gdb/ 2014-04-22 Pedro Alves * infrun.c (schedlock_applies): New function, factored out from find_thread_needs_step_over. (find_thread_needs_step_over): Use it. (switch_back_to_stepped_thread): Always clear trap_expected if the step over is finished. Return early if scheduler locking applies. Look for the stepping thread and a potential step-over thread with a single loop. (currently_stepping_or_nexting_callback): Delete. 2014-04-22 Pedro Alves * gdb.base/consecutive-step-over.c: New file. * gdb.base/consecutive-step-over.exp: New file. --- gdb/ChangeLog | 11 ++ gdb/infrun.c | 125 ++++++++++++------ gdb/testsuite/ChangeLog | 5 + .../gdb.base/consecutive-step-over.c | 28 ++++ .../gdb.base/consecutive-step-over.exp | 70 ++++++++++ 5 files changed, 195 insertions(+), 44 deletions(-) create mode 100644 gdb/testsuite/gdb.base/consecutive-step-over.c create mode 100644 gdb/testsuite/gdb.base/consecutive-step-over.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 120b70b620d..6de9f69445e 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,14 @@ +2014-04-22 Pedro Alves + + * infrun.c (schedlock_applies): New function, factored out from + find_thread_needs_step_over. + (find_thread_needs_step_over): Use it. + (switch_back_to_stepped_thread): Always clear trap_expected if the + step over is finished. Return early if scheduler locking applies. + Look for the stepping thread and a potential step-over thread with + a single loop. + (currently_stepping_or_nexting_callback): Delete. + 2014-04-22 Nick Clifton * NEWS: Mention that ARM sim now supports tracing. diff --git a/gdb/infrun.c b/gdb/infrun.c index 31bb1327210..ab39b6e7bd3 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -85,9 +85,6 @@ static void set_schedlock_func (char *args, int from_tty, static int currently_stepping (struct thread_info *tp); -static int currently_stepping_or_nexting_callback (struct thread_info *tp, - void *data); - static void xdb_handle_command (char *args, int from_tty); static void print_exited_reason (int exitstatus); @@ -2107,6 +2104,17 @@ thread_still_needs_step_over (struct thread_info *tp) return 0; } +/* Returns true if scheduler locking applies. STEP indicates whether + we're about to do a step/next-like command to a thread. */ + +static int +schedlock_applies (int step) +{ + return (scheduler_mode == schedlock_on + || (scheduler_mode == schedlock_step + && step)); +} + /* Look a thread other than EXCEPT that has previously reported a breakpoint event, and thus needs a step-over in order to make progress. Returns NULL is none is found. STEP indicates whether @@ -2116,21 +2124,16 @@ thread_still_needs_step_over (struct thread_info *tp) static struct thread_info * find_thread_needs_step_over (int step, struct thread_info *except) { - int schedlock_enabled; struct thread_info *tp, *current; /* With non-stop mode on, threads are always handled individually. */ gdb_assert (! non_stop); - schedlock_enabled = (scheduler_mode == schedlock_on - || (scheduler_mode == schedlock_step - && step)); - current = inferior_thread (); /* If scheduler locking applies, we can avoid iterating over all threads. */ - if (schedlock_enabled) + if (schedlock_applies (step)) { if (except != current && thread_still_needs_step_over (current)) @@ -5137,6 +5140,7 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs) { struct thread_info *tp; struct thread_info *stepping_thread; + struct thread_info *step_over; /* If any thread is blocked on some internal breakpoint, and we simply need to step over that breakpoint to get it going @@ -5179,17 +5183,72 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs) return 1; } - stepping_thread - = iterate_over_threads (currently_stepping_or_nexting_callback, - ecs->event_thread); + /* Otherwise, we no longer expect a trap in the current thread. + Clear the trap_expected flag before switching back -- this is + what keep_going does as well, if we call it. */ + ecs->event_thread->control.trap_expected = 0; + + /* If scheduler locking applies even if not stepping, there's no + need to walk over threads. Above we've checked whether the + current thread is stepping. If some other thread not the + event thread is stepping, then it must be that scheduler + locking is not in effect. */ + if (schedlock_applies (0)) + return 0; + + /* Look for the stepping/nexting thread, and check if any other + thread other than the stepping thread needs to start a + step-over. Do all step-overs before actually proceeding with + step/next/etc. */ + stepping_thread = NULL; + step_over = NULL; + ALL_THREADS (tp) + { + /* Ignore threads of processes we're not resuming. */ + if (!sched_multi + && ptid_get_pid (tp->ptid) != ptid_get_pid (inferior_ptid)) + continue; + + /* When stepping over a breakpoint, we lock all threads + except the one that needs to move past the breakpoint. + If a non-event thread has this set, the "incomplete + step-over" check above should have caught it earlier. */ + gdb_assert (!tp->control.trap_expected); + + /* Did we find the stepping thread? */ + if (tp->control.step_range_end) + { + /* Yep. There should only one though. */ + gdb_assert (stepping_thread == NULL); + + /* The event thread is handled at the top, before we + enter this loop. */ + gdb_assert (tp != ecs->event_thread); + + /* If some thread other than the event thread is + stepping, then scheduler locking can't be in effect, + otherwise we wouldn't have resumed the current event + thread in the first place. */ + gdb_assert (!schedlock_applies (1)); + + stepping_thread = tp; + } + else if (thread_still_needs_step_over (tp)) + { + step_over = tp; + + /* At the top we've returned early if the event thread + is stepping. If some other thread not the event + thread is stepping, then scheduler locking can't be + in effect, and we can resume this thread. No need to + keep looking for the stepping thread then. */ + break; + } + } - /* Check if any other thread except the stepping thread that - needs to start a step-over. Do that before actually - proceeding with step/next/etc. */ - tp = find_thread_needs_step_over (stepping_thread != NULL, - stepping_thread); - if (tp != NULL) + if (step_over != NULL) { + tp = step_over; if (debug_infrun) { fprintf_unfiltered (gdb_stdlog, @@ -5197,14 +5256,9 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs) target_pid_to_str (tp->ptid)); } - gdb_assert (!tp->control.trap_expected); + /* Only the stepping thread should have this set. */ gdb_assert (tp->control.step_range_end == 0); - /* We no longer expect a trap in the current thread. Clear - the trap_expected flag before switching. This is what - keep_going would do as well, if we called it. */ - ecs->event_thread->control.trap_expected = 0; - ecs->ptid = tp->ptid; ecs->event_thread = tp; switch_to_thread (ecs->ptid); @@ -5212,12 +5266,13 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs) return 1; } - tp = stepping_thread; - if (tp != NULL) + if (stepping_thread != NULL) { struct frame_info *frame; struct gdbarch *gdbarch; + tp = stepping_thread; + /* If the stepping thread exited, then don't try to switch back and resume it, which could fail in several different ways depending on the target. Instead, just keep going. @@ -5250,11 +5305,6 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs) return 1; } - /* Otherwise, we no longer expect a trap in the current thread. - Clear the trap_expected flag before switching back -- this is - what keep_going would do as well, if we called it. */ - ecs->event_thread->control.trap_expected = 0; - if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: switching back to stepped thread\n"); @@ -5325,19 +5375,6 @@ currently_stepping (struct thread_info *tp) || bpstat_should_step ()); } -/* Returns true if any thread *but* the one passed in "data" is in the - middle of stepping or of handling a "next". */ - -static int -currently_stepping_or_nexting_callback (struct thread_info *tp, void *data) -{ - if (tp == data) - return 0; - - return (tp->control.step_range_end - || tp->control.trap_expected); -} - /* Inferior has stepped into a subroutine call with source code that we should not step over. Do step to the first line of code in it. */ diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 1de6dc0864d..2f42ad12fc1 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2014-04-22 Pedro Alves + + * gdb.base/consecutive-step-over.c: New file. + * gdb.base/consecutive-step-over.exp: New file. + 2014-04-22 Pedro Alves * lib/gdb.exp (gdb_continue_to_breakpoint): Use gdb_test_multiple diff --git a/gdb/testsuite/gdb.base/consecutive-step-over.c b/gdb/testsuite/gdb.base/consecutive-step-over.c new file mode 100644 index 00000000000..e7c8e9d80ca --- /dev/null +++ b/gdb/testsuite/gdb.base/consecutive-step-over.c @@ -0,0 +1,28 @@ +/* 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 . */ + +volatile int i; +volatile int condition; + +int +main (void) +{ + i = 0; + i = 1; + i = 2; + i = 3; + + return 0; /* break here */ +} diff --git a/gdb/testsuite/gdb.base/consecutive-step-over.exp b/gdb/testsuite/gdb.base/consecutive-step-over.exp new file mode 100644 index 00000000000..3f78042da03 --- /dev/null +++ b/gdb/testsuite/gdb.base/consecutive-step-over.exp @@ -0,0 +1,70 @@ +# 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 . + +# +# Regression test for a bug where GDB would internal error if a thread +# runs into a breakpoint that needs stepping over, just after stepping +# over another breakpoint, without a user visible stop in between. +# +standard_testfile + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { + return -1 +} + +if ![runto_main] then { + fail "Can't run to main" + return 0 +} + +# Make sure the target doesn't hide the breakpoint hits (that don't +# cause a user visible stop) from GDB. +gdb_test_no_output "set breakpoint condition-evaluation host" + +set up_to_nl "\[^\r\n\]+\[\r\n\]+" + +# Number of consecutive breakpoints in a row to try. +set n_insns 3 + +# Extract addresses of a few consecutive instructions. +set test "get breakpoint addresses" +if { [gdb_test_multiple "x /[expr $n_insns + 1]i \$pc" $test { + -re "=> $hex${up_to_nl} ($hex)${up_to_nl} ($hex)${up_to_nl} ($hex)${up_to_nl}$gdb_prompt $" { + for {set i 1} {$i <= $n_insns} {incr i} { + set bp_addrs($i) $expect_out($i,string) + } + pass $test + } +}] != 0 } { + # No use proceeding if bp_addrs wasn't set. + return +} + +for {set i 1} {$i <= $n_insns} {incr i} { + with_test_prefix "insn $i" { + gdb_test "break \*$bp_addrs($i)" \ + "Breakpoint $decimal at $bp_addrs($i): file .*" \ + "set breakpoint" + + # Give the breakpoint a condition that always fails, so that + # the thread is immediately re-resumed. + gdb_test_no_output "condition \$bpnum condition" \ + "set condition" + } +} + +set lineno [gdb_get_line_number "break here"] +gdb_breakpoint $lineno +gdb_continue_to_breakpoint "break here" ".*break here.*" -- 2.30.2