From patchwork Mon Jun 29 15:56:11 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 7420 Received: (qmail 115416 invoked by alias); 29 Jun 2015 15:56:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 115400 invoked by uid 89); 29 Jun 2015 15:56:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 29 Jun 2015 15:56:14 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 4979D35B8D5 for ; Mon, 29 Jun 2015 15:56:13 +0000 (UTC) Received: from brno.lan (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5TFuB60001113 for ; Mon, 29 Jun 2015 11:56:12 -0400 From: Pedro Alves To: gdb-patches@sourceware.org Subject: [pushed] PR threads/18127 - threads spawned by infcall end up stuck in "running" state Date: Mon, 29 Jun 2015 16:56:11 +0100 Message-Id: <1435593371-13111-1-git-send-email-palves@redhat.com> Refs: https://sourceware.org/ml/gdb/2015-03/msg00024.html https://sourceware.org/ml/gdb/2015-06/msg00005.html On GNU/Linux, if an infcall spawns a thread, that thread ends up with stuck running state. This happens because: - when linux-nat.c detects a new thread, it marks them as running, and does not report anything to the core. - we skip finish_thread_state when the thread that is running the infcall stops. As result, that new thread ends up with stuck "running" state, even though it really is stopped. On Windows, _all_ threads end up stuck in running state, not just the one that was spawned. That happens because when a new thread is detected, unlike linux-nat.c, windows-nat.c reports TARGET_WAITKIND_SPURIOUS to infrun. It's the fact that that event does not cause a user-visible stop that triggers the problem. When the target is re-resumed, we call set_running with a wildcard ptid, which marks all thread as running. That set_running is not suppressed because the (leader) thread being resumed does not have in_infcall set. Later, when the infcall finally finishes successfully, nothing marks all threads back to stopped. We can trigger the same problem on all targets by having a thread other than the one that is running the infcall report a breakpoint hit to infrun, and then have that breakpoint not cause a stop. That's what the included test does. The fix is to stop GDB from suppressing the set_running calls while doing an infcall, and then set the threads back to stopped when the call finishes, iff they were originally stopped before the infcall started. (Note the MI *running/*stopped event suppression isn't affected.) Tested on x86_64 GNU/Linux. gdb/ChangeLog: 2015-06-29 Pedro Alves PR threads/18127 * infcall.c (run_inferior_call): On infcall success, if the thread was marked stopped before, reset it back to stopped. * infrun.c (resume): Don't suppress the set_running calls when doing an infcall. (normal_stop): Only discard the finish_thread_state cleanup if the infcall succeeded. gdb/testsuite/ChangeLog: 2015-06-29 Pedro Alves PR threads/18127 * gdb.threads/hand-call-new-thread.c: New file. * gdb.threads/hand-call-new-thread.c: New file. --- gdb/ChangeLog | 10 +++++ gdb/testsuite/ChangeLog | 6 +++ gdb/infcall.c | 21 +++++++++ gdb/infrun.c | 31 ++++++-------- gdb/testsuite/gdb.threads/hand-call-new-thread.c | 50 ++++++++++++++++++++++ gdb/testsuite/gdb.threads/hand-call-new-thread.exp | 48 +++++++++++++++++++++ 6 files changed, 148 insertions(+), 18 deletions(-) create mode 100644 gdb/testsuite/gdb.threads/hand-call-new-thread.c create mode 100644 gdb/testsuite/gdb.threads/hand-call-new-thread.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index b25dff2..a8aa0ba 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,13 @@ +2015-06-29 Pedro Alves + + PR threads/18127 + * infcall.c (run_inferior_call): On infcall success, if the thread + was marked stopped before, reset it back to stopped. + * infrun.c (resume): Don't suppress the set_running calls when + doing an infcall. + (normal_stop): Only discard the finish_thread_state cleanup if the + infcall succeeded. + 2015-06-29 Pierre Langlois * MAINTAINERS (Write After Approval): Update my email address. diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 89d8e32..d9ec546 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2015-06-29 Pedro Alves + + PR threads/18127 + * gdb.threads/hand-call-new-thread.c: New file. + * gdb.threads/hand-call-new-thread.c: New file. + 2015-06-26 Keith Seitz Doug Evans diff --git a/gdb/infcall.c b/gdb/infcall.c index f79afea..e3bd72a 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -387,6 +387,7 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) int saved_in_infcall = call_thread->control.in_infcall; ptid_t call_thread_ptid = call_thread->ptid; int saved_sync_execution = sync_execution; + int was_running = call_thread->state == THREAD_RUNNING; /* Infcalls run synchronously, in the foreground. */ if (target_can_async_p ()) @@ -433,6 +434,26 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) CALL_THREAD as it could be invalid if its thread has exited. */ call_thread = find_thread_ptid (call_thread_ptid); + /* If the infcall does NOT succeed, normal_stop will have already + finished the thread states. However, on success, normal_stop + defers here, so that we can set back the thread states to what + they were before the call. Note that we must also finish the + state of new threads that might have spawned while the call was + running. The main cases to handle are: + + - "(gdb) print foo ()", or any other command that evaluates an + expression at the prompt. (The thread was marked stopped before.) + + - "(gdb) break foo if return_false()" or similar cases where we + do an infcall while handling an event (while the thread is still + marked running). In this example, whether the condition + evaluates true and thus we'll present a user-visible stop is + decided elsewhere. */ + if (!was_running + && ptid_equal (call_thread_ptid, inferior_ptid) + && stop_stack_dummy == STOP_STACK_DUMMY) + finish_thread_state (user_visible_resume_ptid (0)); + enable_watchpoints_after_interactive_call_stop (); /* Call breakpoint_auto_delete on the current contents of the bpstat diff --git a/gdb/infrun.c b/gdb/infrun.c index 792f847..445a612 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2264,11 +2264,8 @@ resume (enum gdb_signal sig) requests finish. The thread is not executing at this point, and the call to set_executing will be made later. But we need to call set_running here, since from the - user/frontend's point of view, threads were set running. - Unless we're calling an inferior function, as in that - case we pretend the inferior doesn't run at all. */ - if (!tp->control.in_infcall) - set_running (user_visible_resume_ptid (user_step), 1); + user/frontend's point of view, threads were set running. */ + set_running (user_visible_resume_ptid (user_step), 1); discard_cleanups (old_cleanups); return; } @@ -2346,10 +2343,8 @@ resume (enum gdb_signal sig) /* Even if RESUME_PTID is a wildcard, and we end up resuming less (e.g., we might need to step over a breakpoint), from the user/frontend's point of view, all threads in RESUME_PTID are now - running. Unless we're calling an inferior function, as in that - case pretend we inferior doesn't run at all. */ - if (!tp->control.in_infcall) - set_running (resume_ptid, 1); + running. */ + set_running (resume_ptid, 1); /* Maybe resume a single thread after all. */ if ((step || thread_has_single_step_breakpoints_set (tp)) @@ -6664,15 +6659,15 @@ normal_stop (void) if (has_stack_frames () && !stop_stack_dummy) set_current_sal_from_frame (get_current_frame ()); - /* Let the user/frontend see the threads as stopped, but do nothing - if the thread was running an infcall. We may be e.g., evaluating - a breakpoint condition. In that case, the thread had state - THREAD_RUNNING before the infcall, and shall remain set to - running, all without informing the user/frontend about state - transition changes. If this is actually a call command, then the - thread was originally already stopped, so there's no state to - finish either. */ - if (target_has_execution && inferior_thread ()->control.in_infcall) + /* Let the user/frontend see the threads as stopped, but defer to + call_function_by_hand if the thread finished an infcall + successfully. We may be e.g., evaluating a breakpoint condition. + In that case, the thread had state THREAD_RUNNING before the + infcall, and shall remain marked running, all without informing + the user/frontend about state transition changes. */ + if (target_has_execution + && inferior_thread ()->control.in_infcall + && stop_stack_dummy == STOP_STACK_DUMMY) discard_cleanups (old_chain); else do_cleanups (old_chain); diff --git a/gdb/testsuite/gdb.threads/hand-call-new-thread.c b/gdb/testsuite/gdb.threads/hand-call-new-thread.c new file mode 100644 index 0000000..042be9b --- /dev/null +++ b/gdb/testsuite/gdb.threads/hand-call-new-thread.c @@ -0,0 +1,50 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2015 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 . */ + +#include +#include +#include +#include + +static int +foo (void) +{ + usleep (1); +} + +static void * +thread_function (void *arg) +{ + while (1) + foo (); +} + +void +new_thread (void) +{ + pthread_t thread; + int res; + + res = pthread_create (&thread, NULL, thread_function, NULL); + assert (res == 0); +} + +int +main (int argc, char **argv) +{ + return 0; +} diff --git a/gdb/testsuite/gdb.threads/hand-call-new-thread.exp b/gdb/testsuite/gdb.threads/hand-call-new-thread.exp new file mode 100644 index 0000000..a02d8ba --- /dev/null +++ b/gdb/testsuite/gdb.threads/hand-call-new-thread.exp @@ -0,0 +1,48 @@ +# Copyright (C) 2015 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 . + +# Ensure that new threads created while an infcall is ongoing are set +# to stopped state once the call finishes. + +standard_testfile + +if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] { + return -1 +} + +if ![runto_main] { + continue +} + +# Set a thread-specific breakpoint that the wrong thread trips on +# while running the infcall. Check that no thread ends up in stale +# "running" state once the call finishes. +gdb_test "b foo thread 1" "Breakpoint .*$srcfile.*" + +for {set i 0} {$i < 3} {incr i} { + with_test_prefix "iter $i" { + gdb_test "p new_thread ()" " = void" + + set message "no thread marked running" + gdb_test_multiple "info threads" $message { + -re "\\\(running\\\).*$gdb_prompt $" { + fail $message + } + -re "$gdb_prompt $" { + pass $message + } + } + } +}