From b57bacecd5f3684cd9f58b0da0e2caccbb6a546d Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 2 Oct 2014 09:55:38 +0100 Subject: Fix non-stop regressions caused by "breakpoints always-inserted off" changes Commit a25a5a45 (Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto") regressed non-stop remote debugging. This was exposed by mi-nsintrall.exp intermittently failing with a spurious SIGTRAP. The problem is that when debugging with "target remote", new threads the target has spawned but have never reported a stop aren't visible to GDB until it explicitly resyncs its thread list with the target's. For example, in a program like this: int main (void) { pthread_t child_thread; pthread_create (&child_thread, NULL, child_function, NULL); return 0; <<<< set breakpoint here } If the user sets a breakpoint at the "return" statement, and runs the program, when that breakpoint hit is reported, GDB is only aware of the main thread. So if we base the decision to remove or insert breakpoints from the target based on whether all the threads we know about are stopped, we'll miss that child_thread is running, and thus we'll remove breakpoints from the target, even through they should still remain inserted, otherwise child_thread will miss them. The break-while-running.exp test actually should also be exposing this thread-list-out-of-synch problem. That test sets a breakpoint while the main thread is stopped, but other threads are running. Because other threads are running, the breakpoint is supposed to be inserted immediately. But, unless something forces a refetch of the thread list, like, e.g., "info threads", GDB won't be aware of the other threads that had been spawned by the main thread, and so won't insert new or old breakpoints in the target. And it turns out that the test is exactly doing an explicit "info threads", masking out the problem... This commit adjust the test to exercise the case of not issuing "info threads". The test then fails without the GDB fix. In the ni-nsintrall.exp case, what happens is that several threads hit the same breakpoint, and when the first thread reports the stop, because GDB wasn't aware other threads exist, all threads known to GDB are found stopped, so GDB removes the breakpoints from the target. The other threads follow up with SIGTRAPs too for that same breakpoint, which has already been removed. For the first few threads, the moribund breakpoints machinery suppresses the SIGTRAPs, but after a few events (precisely '3 * thread_count () + 1' at the time the breakpoint was removed, see update_global_location_list), the moribund breakpoint machinery is no longer aware of the removed breakpoint, and the SIGTRAP is reported as a spurious stop. The fix is naturally then to stop assuming that if no thread in the list is executing, then the target is fully stopped. We can't know that until we fully sync the thread list. Because updating the thread list on every stop would be too much RSP traffic, I chose instead to update it whenever we're about to present a stop to the user. Actually updating the thread list at that point happens to be an item I had added to the local/remote parity wiki page a while ago: Native GNU/Linux debugging adds new threads to the thread list as the program creates them "The [New Thread foo] messages". Remote debugging can't do that, and it's arguable whether we shouldn't even stop native debugging from doing that, as it hinders inferior performance. However, a related issue is that with remote targets (and gdbserver), even after the program stops, the user still needs to do "info threads" to pull an updated thread list. This, should most likely be addressed, so that GDB pulls the list itself, perhaps just before presenting a stop to the user. With that in place, the need to delay "Program received signal FOO" was actually caught by the manythreads.exp test. Without that bit, I was getting: [Thread 0x7ffff7f13700 (LWP 4499) exited] [New Thread 0x7ffff7f0b700 (LWP 4500)] ^C Program received signal SIGINT, Interrupt. [New Thread 0x7ffff7f03700 (LWP 4501)] <<< new output [Switching to Thread 0x7ffff7f0b700 (LWP 4500)] __GI___nptl_death_event () at events.c:31 31 { (gdb) FAIL: gdb.threads/manythreads.exp: stop threads 1 That is, I was now getting "New Thread" lines after the "Program received signal" line, and the test doesn't expect them. As the number of new threads discovered before and after the "Program received signal" output is unbounded, it's much nicer to defer "Program received signal" until after synching the thread list, thus close to the "switching to thread" output and "current frame/source" info: [Thread 0x7ffff7863700 (LWP 7647) exited] ^C[New Thread 0x7ffff786b700 (LWP 7648)] Program received signal SIGINT, Interrupt. [Switching to Thread 0x7ffff7fc4740 (LWP 6243)] __GI___nptl_create_event () at events.c:25 25 { (gdb) PASS: gdb.threads/manythreads.exp: stop threads 1 Tested on x86_64 Fedora 20, native and gdbserver. gdb/ 2014-10-02 Pedro Alves * breakpoint.c (breakpoints_should_be_inserted_now): Use threads_are_executing. * breakpoint.h (breakpoints_should_be_inserted_now): Add describing comment. * gdbthread.h (threads_are_executing): Declare. (handle_signal_stop) : Don't print about the signal here if stopping. (end_stepping_range): Don't notify observers here. (normal_stop): Update the thread list. If stopped by a random signal or a stepping range ended, notify observers. * thread.c (threads_executing): New global. (init_thread_list): Clear 'threads_executing'. (set_executing): Set or clear 'threads_executing'. (threads_are_executing): New function. (update_threads_executing): New function. (update_thread_list): Use it. gdb/testsuite/ 2014-10-02 Pedro Alves * gdb.threads/break-while-running.exp (test): Add new 'update_thread_list' argument. Skip "info threads" if false. (top level): Add new 'update_thread_list' axis. --- gdb/ChangeLog | 19 ++++++++ gdb/breakpoint.c | 11 ++--- gdb/breakpoint.h | 10 +++++ gdb/gdbthread.h | 3 ++ gdb/infrun.c | 54 +++++++++++++++-------- gdb/testsuite/ChangeLog | 6 +++ gdb/testsuite/gdb.threads/break-while-running.exp | 35 ++++++++++----- gdb/thread.c | 44 ++++++++++++++++++ 8 files changed, 144 insertions(+), 38 deletions(-) (limited to 'gdb') diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 18ebca44f87..748d61dc5db 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,22 @@ +2014-10-02 Pedro Alves + + * breakpoint.c (breakpoints_should_be_inserted_now): Use + threads_are_executing. + * breakpoint.h (breakpoints_should_be_inserted_now): Add + describing comment. + * gdbthread.h (threads_are_executing): Declare. + (handle_signal_stop) : Don't print about the + signal here if stopping. + (end_stepping_range): Don't notify observers here. + (normal_stop): Update the thread list. If stopped by a random + signal or a stepping range ended, notify observers. + * thread.c (threads_executing): New global. + (init_thread_list): Clear 'threads_executing'. + (set_executing): Set or clear 'threads_executing'. + (threads_are_executing): New function. + (update_threads_executing): New function. + (update_thread_list): Use it. + 2014-10-02 Pedro Alves PR breakpoints/17431 diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 684f74c228f..7da88b0a498 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -473,6 +473,8 @@ show_always_inserted_mode (struct ui_file *file, int from_tty, value); } +/* See breakpoint.h. */ + int breakpoints_should_be_inserted_now (void) { @@ -485,8 +487,6 @@ breakpoints_should_be_inserted_now (void) } else if (target_has_execution) { - struct thread_info *tp; - if (always_inserted_mode) { /* The user wants breakpoints inserted even if all threads @@ -494,11 +494,8 @@ breakpoints_should_be_inserted_now (void) return 1; } - ALL_NON_EXITED_THREADS (tp) - { - if (tp->executing) - return 1; - } + if (threads_are_executing ()) + return 1; } return 0; } diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index e191c10f9ce..d65405f8559 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -1491,6 +1491,16 @@ extern void breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf, const gdb_byte *writebuf_org, ULONGEST memaddr, LONGEST len); +/* Return true if breakpoints should be inserted now. That'll be the + case if either: + + - the target has global breakpoints. + + - "breakpoint always-inserted" is on, and the target has + execution. + + - threads are executing. +*/ extern int breakpoints_should_be_inserted_now (void); /* Called each time new event from target is processed. diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 676849195db..26ca9259ede 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -381,6 +381,9 @@ extern void set_executing (ptid_t ptid, int executing); /* Reports if thread PTID is executing. */ extern int is_executing (ptid_t ptid); +/* True if any (known or unknown) thread is or may be executing. */ +extern int threads_are_executing (void); + /* Merge the executing property of thread PTID over to its thread state property (frontend running/stopped view). diff --git a/gdb/infrun.c b/gdb/infrun.c index 728c160bc7f..8137eb38d34 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4436,13 +4436,6 @@ handle_signal_stop (struct execution_control_state *ecs) stopped_by_random_signal = 1; - if (signal_print[ecs->event_thread->suspend.stop_signal]) - { - /* The signal table tells us to print about this signal. */ - printed = 1; - target_terminal_ours_for_output (); - observer_notify_signal_received (ecs->event_thread->suspend.stop_signal); - } /* Always stop on signals if we're either just gaining control of the program, or the user explicitly requested this thread to remain stopped. */ @@ -4454,10 +4447,17 @@ handle_signal_stop (struct execution_control_state *ecs) stop_waiting (ecs); return; } - /* If not going to stop, give terminal back - if we took it away. */ - else if (printed) - target_terminal_inferior (); + + /* Notify observers the signal has "handle print" set. Note we + returned early above if stopping; normal_stop handles the + printing in that case. */ + if (signal_print[ecs->event_thread->suspend.stop_signal]) + { + /* The signal table tells us to print about this signal. */ + target_terminal_ours_for_output (); + observer_notify_signal_received (ecs->event_thread->suspend.stop_signal); + target_terminal_inferior (); + } /* Clear the signal if it should not be passed. */ if (signal_program[ecs->event_thread->suspend.stop_signal] == 0) @@ -6092,15 +6092,12 @@ prepare_to_wait (struct execution_control_state *ecs) } /* We are done with the step range of a step/next/si/ni command. - Called once for each n of a "step n" operation. Notify observers - if not in the middle of doing a "step N" operation for N > 1. */ + Called once for each n of a "step n" operation. */ static void end_stepping_range (struct execution_control_state *ecs) { ecs->event_thread->control.stop_step = 1; - if (!ecs->event_thread->step_multi) - observer_notify_end_stepping_range (); stop_waiting (ecs); } @@ -6311,6 +6308,19 @@ normal_stop (void) && last.kind != TARGET_WAITKIND_NO_RESUMED) make_cleanup (finish_thread_state_cleanup, &inferior_ptid); + /* As we're presenting a stop, and potentially removing breakpoints, + update the thread list so we can tell whether there are threads + running on the target. With target remote, for example, we can + only learn about new threads when we explicitly update the thread + list. Do this before notifying the interpreters about signal + stops, end of stepping ranges, etc., so that the "new thread" + output is emitted before e.g., "Program received signal FOO", + instead of after. */ + update_thread_list (); + + if (last.kind == TARGET_WAITKIND_STOPPED && stopped_by_random_signal) + observer_notify_signal_received (inferior_thread ()->suspend.stop_signal); + /* As with the notification of thread events, we want to delay notifying the user that we've switched thread context until the inferior actually stops. @@ -6349,6 +6359,7 @@ normal_stop (void) printf_filtered (_("No unwaited-for children left.\n")); } + /* Note: this depends on the update_thread_list call above. */ if (!breakpoints_should_be_inserted_now () && target_has_execution) { if (remove_breakpoints ()) @@ -6366,14 +6377,19 @@ normal_stop (void) if (stopped_by_random_signal) disable_current_display (); - /* Don't print a message if in the middle of doing a "step n" - operation for n > 1 */ + /* Notify observers if we finished a "step"-like command, etc. */ if (target_has_execution && last.kind != TARGET_WAITKIND_SIGNALLED && last.kind != TARGET_WAITKIND_EXITED - && inferior_thread ()->step_multi && inferior_thread ()->control.stop_step) - goto done; + { + /* But not if if in the middle of doing a "step n" operation for + n > 1 */ + if (inferior_thread ()->step_multi) + goto done; + + observer_notify_end_stepping_range (); + } target_terminal_ours (); async_enable_stdin (); diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 87f3684e0ef..35796603b15 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2014-10-02 Pedro Alves + + * gdb.threads/break-while-running.exp (test): Add new + 'update_thread_list' argument. Skip "info threads" if false. + (top level): Add new 'update_thread_list' axis. + 2014-10-02 Pedro Alves PR breakpoints/17431 diff --git a/gdb/testsuite/gdb.threads/break-while-running.exp b/gdb/testsuite/gdb.threads/break-while-running.exp index 690d6ab9eb6..ea28bf00304 100644 --- a/gdb/testsuite/gdb.threads/break-while-running.exp +++ b/gdb/testsuite/gdb.threads/break-while-running.exp @@ -28,11 +28,13 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads} return -1 } -# The test proper. NON_STOP indicates whether we're testing in -# non-stop, or all-stop mode. ALWAYS_INSERTED indicates whether -# testing in "breakpoint always-inserted" mode. +# The test proper. UPDATE_THREAD_LIST indicates whether we should do +# an "info threads" to sync the thread list after the first stop. +# ALWAYS_INSERTED indicates whether testing in "breakpoint +# always-inserted" mode. NON_STOP indicates whether we're testing in +# non-stop, or all-stop mode. -proc test { always_inserted non_stop } { +proc test { update_thread_list always_inserted non_stop } { global srcfile binfile global gdb_prompt global decimal @@ -70,9 +72,15 @@ proc test { always_inserted non_stop } { gdb_test "thread 1" "Switching to .*" "switch back to main thread" } - gdb_test "info threads" \ - "\\\(running\\\).*\\\(running\\\).* main .*" \ - "only main stopped" + # Test with and without pulling the thread list explicitly with + # "info threads". GDB should be able to figure out itself whether + # the target is running and thus breakpoints should be inserted, + # without the user explicitly fetching the thread list. + if {$update_thread_list} { + gdb_test "info threads" \ + "\\\(running\\\).*\\\(running\\\).* main .*" \ + "only main stopped" + } # Don't use gdb_test as it's racy in this case -- gdb_test matches # the prompt with an end anchor. Sometimes expect will manage to @@ -141,11 +149,14 @@ proc test { always_inserted non_stop } { } } -foreach always_inserted { "off" "on" } { - foreach non_stop { "off" "on" } { - set stop_mode [expr ($non_stop=="off")?"all-stop":"non-stop"] - with_test_prefix "always-inserted $always_inserted: $stop_mode" { - test $always_inserted $non_stop +foreach update_thread_list { true false } { + foreach always_inserted { "off" "on" } { + foreach non_stop { "off" "on" } { + set stop_mode [expr ($non_stop=="off")?"all-stop":"non-stop"] + set update_list_mode [expr ($update_thread_list)?"w/ithr":"wo/ithr"] + with_test_prefix "$update_list_mode: always-inserted $always_inserted: $stop_mode" { + test $update_thread_list $always_inserted $non_stop + } } } } diff --git a/gdb/thread.c b/gdb/thread.c index bceaf49ab33..ac1d8a15e69 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -56,6 +56,13 @@ void _initialize_thread (void); struct thread_info *thread_list = NULL; static int highest_thread_num; +/* True if any thread is, or may be executing. We need to track this + separately because until we fully sync the thread list, we won't + know whether the target is fully stopped, even if we see stop + events for all known threads, because any of those threads may have + spawned new threads we haven't heard of yet. */ +static int threads_executing; + static void thread_command (char *tidstr, int from_tty); static void thread_apply_all_command (char *, int); static int thread_alive (struct thread_info *); @@ -167,6 +174,7 @@ init_thread_list (void) } thread_list = NULL; + threads_executing = 0; } /* Allocate a new thread with target id PTID and add it to the thread @@ -702,6 +710,22 @@ set_executing (ptid_t ptid, int executing) gdb_assert (tp); tp->executing = executing; } + + /* It only takes one running thread to spawn more threads.*/ + if (executing) + threads_executing = 1; + /* Only clear the flag if the caller is telling us everything is + stopped. */ + else if (ptid_equal (minus_one_ptid, ptid)) + threads_executing = 0; +} + +/* See gdbthread.h. */ + +int +threads_are_executing (void) +{ + return threads_executing; } void @@ -1501,11 +1525,31 @@ gdb_thread_select (struct ui_out *uiout, char *tidstr, char **error_message) return GDB_RC_OK; } +/* Update the 'threads_executing' global based on the threads we know + about right now. */ + +static void +update_threads_executing (void) +{ + struct thread_info *tp; + + threads_executing = 0; + ALL_NON_EXITED_THREADS (tp) + { + if (tp->executing) + { + threads_executing = 1; + break; + } + } +} + void update_thread_list (void) { prune_threads (); target_find_new_threads (); + update_threads_executing (); } /* Return a new value for the selected thread's id. Return a value of 0 if -- cgit v1.2.1