From patchwork Thu May 21 23:18:57 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 6863 Received: (qmail 64200 invoked by alias); 21 May 2015 23:19:22 -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 64103 invoked by uid 89); 21 May 2015 23:19:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.1 required=5.0 tests=AWL, BAYES_50, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD 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; Thu, 21 May 2015 23:19:18 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 5D0F129320E for ; Thu, 21 May 2015 23:19:17 +0000 (UTC) Received: from brno.lan (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4LNJEIx027592 for ; Thu, 21 May 2015 19:19:16 -0400 From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 01/18] Fix and test "checkpoint" in non-stop mode Date: Fri, 22 May 2015 00:18:57 +0100 Message-Id: <1432250354-2721-2-git-send-email-palves@redhat.com> In-Reply-To: <1432250354-2721-1-git-send-email-palves@redhat.com> References: <1432250354-2721-1-git-send-email-palves@redhat.com> Letting a "checkpoint" run to exit with "set non-stop on" behaves differently compared to the default all-stop mode ("set non-stop off"). Currently, in non-stop mode: (gdb) start Temporary breakpoint 1 at 0x40086b: file src/gdb/testsuite/gdb.base/checkpoint.c, line 28. Starting program: build/gdb/testsuite/gdb.base/checkpoint Temporary breakpoint 1, main () at src/gdb/testsuite/gdb.base/checkpoint.c:28 28 char *tmp = &linebuf[0]; (gdb) checkpoint checkpoint 1: fork returned pid 24948. (gdb) c Continuing. Copy complete. Deleting copy. [Inferior 1 (process 24944) exited normally] [Switching to process 24948] (gdb) info threads Id Target Id Frame 1 process 24948 "checkpoint" (running) No selected thread. See `help thread'. (gdb) c The program is not being run. (gdb) Two issues above: 1. Thread 1 got stuck in "(running)" state (it isn't really running) 2. While checkpoints try to preserve the illusion that the thread is still the same when the process exits, GDB switched to "No thread selected." instead of staying with thread 1 selected. Problem #1 is caused by handle_inferior_event and normal_stop not considering that when a TARGET_WAITKIND_SIGNALLED/TARGET_WAITKIND_EXITED event is reported, and the inferior is mourned, the target may still have execution. Problem #2 is caused by the make_cleanup_restore_current_thread cleanup installed by fetch_inferior_event not being able to find the original thread 1's ptid in the thread list, thus not being able to restore thread 1 as selected thread. The fix is to make the cleanup installed by make_cleanup_restore_current_thread aware of thread ptid changes, by installing a thread_ptid_changed observer that adjusts the cleanup's data. After the patch, we get the same in all-stop and non-stop modes: (gdb) c Continuing. Copy complete. Deleting copy. [Inferior 1 (process 25109) exited normally] [Switching to process 25113] (gdb) info threads Id Target Id Frame * 1 process 25113 "checkpoint" main () at src/gdb/testsuite/gdb.base/checkpoint.c:28 (gdb) Turns out the whole checkpoints.exp file can run in non-stop mode unmodified. I thought of moving most of the test file's contents to a procedure that can be called twice, once in non-stop mode and another in all-stop mode. But then, the test already takes close to 30 seconds to run on my machine, so I thought it'd be nicer to run all-stop and non-stop mode in parallel. Thus I added a new checkpoint-ns.exp file that just appends "set non-stop on" to GDBFLAGS and sources checkpoint.exp. gdb/ChangeLog: 2015-05-21 Pedro Alves * infrun.c (handle_inferior_event): If we get TARGET_WAITKIND_SIGNALLED or TARGET_WAITKIND_EXITED in non-stop mode, mark all threads of the exiting process as not-executing. (normal_stop): If we get TARGET_WAITKIND_SIGNALLED or TARGET_WAITKIND_EXITED in non-stop mode, finish all threads of the exiting process, if inferior_ptid still points at a process. * thread.c (struct current_thread_cleanup) : New field. (current_thread_cleanup_chain): New global. (restore_current_thread_ptid_changed): New function. (restore_current_thread_cleanup_dtor): Remove the cleanup from the current_thread_cleanup_chain list. (make_cleanup_restore_current_thread): Add the cleanup data to the current_thread_cleanup_chain list. (_initialize_thread): Install restore_current_thread_ptid_changed as thread_ptid_changed observer. gdb/testsuite/ChangeLog: 2015-05-21 Pedro Alves * gdb.base/checkpoint-ns.exp: New file. * gdb.base/checkpoint.exp: Pass explicit "checkpoint.c" to standard_testfile. --- gdb/ChangeLog | 18 ++++++++++++ gdb/testsuite/ChangeLog | 6 ++++ gdb/infrun.c | 48 ++++++++++++++++++++++++++------ gdb/testsuite/gdb.base/checkpoint-ns.exp | 26 +++++++++++++++++ gdb/testsuite/gdb.base/checkpoint.exp | 6 ++-- gdb/thread.c | 38 +++++++++++++++++++++++++ 6 files changed, 131 insertions(+), 11 deletions(-) create mode 100644 gdb/testsuite/gdb.base/checkpoint-ns.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 42ef67d..aae10e5 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,21 @@ +2015-05-19 Pedro Alves + + * infrun.c (handle_inferior_event): If we get + TARGET_WAITKIND_SIGNALLED or TARGET_WAITKIND_EXITED in non-stop + mode, mark all threads of the exiting process as not-executing. + (normal_stop): If we get TARGET_WAITKIND_SIGNALLED or + TARGET_WAITKIND_EXITED in non-stop mode, finish all threads of the + exiting process, if inferior_ptid still points at a process. + * thread.c (struct current_thread_cleanup) : New field. + (current_thread_cleanup_chain): New global. + (restore_current_thread_ptid_changed): New function. + (restore_current_thread_cleanup_dtor): Remove the cleanup from the + current_thread_cleanup_chain list. + (make_cleanup_restore_current_thread): Add the cleanup data to the + current_thread_cleanup_chain list. + (_initialize_thread): Install restore_current_thread_ptid_changed + as thread_ptid_changed observer. + 2015-05-21 Andrew Burgess * tui/tui-regs.c (tui_reg_next_command): Use NULL not 0. diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 49f56f4..01af98a 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2015-05-19 Pedro Alves + + * gdb.base/checkpoint-ns.exp: New file. + * gdb.base/checkpoint.exp: Pass explicit "checkpoint.c" to + standard_testfile. + 2015-05-21 Andrew Burgess * gdb.base/completion.exp: Add test for completion of layout diff --git a/gdb/infrun.c b/gdb/infrun.c index 2f6bc41..537de36 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3784,14 +3784,31 @@ handle_inferior_event_1 (struct execution_control_state *ecs) /* Mark the non-executing threads accordingly. In all-stop, all threads of all processes are stopped when we get any event - reported. In non-stop mode, only the event thread stops. If - we're handling a process exit in non-stop mode, there's nothing - to do, as threads of the dead process are gone, and threads of - any other process were left running. */ + reported. In non-stop mode, only the event thread stops. */ if (!non_stop) set_executing (minus_one_ptid, 0); - else if (ecs->ws.kind != TARGET_WAITKIND_SIGNALLED - && ecs->ws.kind != TARGET_WAITKIND_EXITED) + else if (ecs->ws.kind == TARGET_WAITKIND_SIGNALLED + || ecs->ws.kind == TARGET_WAITKIND_EXITED) + { + ptid_t pid_ptid; + + /* If we're handling a process exit in non-stop mode, even + though threads haven't been deleted yet, one would think that + there is nothing to do, as threads of the dead process will + be soon deleted, and threads of any other process were left + running. However, on some targets, threads survive a process + exit event. E.g., for the "checkpoint" command, when the + current checkpoint/fork exits, linux-fork.c automatically + switches to another fork from within target_mourn_inferior, + by associating the same inferior/thread to another fork. We + haven't mourned yet at this point, but we must mark any + threads left in the process as not-executing so that + finish_thread_state marks them stopped (in the user's + perspective) if/when we present the stop to the user. */ + pid_ptid = pid_to_ptid (ptid_get_pid (ecs->ptid)); + set_executing (pid_ptid, 0); + } + else set_executing (ecs->ptid, 0); switch (ecs->ws.kind) @@ -6552,6 +6569,7 @@ normal_stop (void) struct target_waitstatus last; ptid_t last_ptid; struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); + ptid_t pid_ptid; get_last_target_status (&last_ptid, &last); @@ -6561,9 +6579,21 @@ normal_stop (void) here, so do this before any filtered output. */ if (!non_stop) make_cleanup (finish_thread_state_cleanup, &minus_one_ptid); - else if (last.kind != TARGET_WAITKIND_SIGNALLED - && last.kind != TARGET_WAITKIND_EXITED - && last.kind != TARGET_WAITKIND_NO_RESUMED) + else if (last.kind == TARGET_WAITKIND_SIGNALLED + || last.kind == TARGET_WAITKIND_EXITED) + { + /* On some targets, we may still have live threads in the + inferior when we get a process exit event. E.g., for + "checkpoint", when the current checkpoint/fork exits, + linux-fork.c automatically switches to another fork from + within target_mourn_inferior. */ + if (!ptid_equal (inferior_ptid, null_ptid)) + { + pid_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid)); + make_cleanup (finish_thread_state_cleanup, &pid_ptid); + } + } + else if (last.kind != TARGET_WAITKIND_NO_RESUMED) make_cleanup (finish_thread_state_cleanup, &inferior_ptid); /* As we're presenting a stop, and potentially removing breakpoints, diff --git a/gdb/testsuite/gdb.base/checkpoint-ns.exp b/gdb/testsuite/gdb.base/checkpoint-ns.exp new file mode 100644 index 0000000..d3698ba --- /dev/null +++ b/gdb/testsuite/gdb.base/checkpoint-ns.exp @@ -0,0 +1,26 @@ +# 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 . */ + +# Test gdb checkpoint and restart in non-stop mode. + +# We drive non-stop mode from a separate file because the whole test +# takes a while to run. This way, we can test both modes in parallel. + +set saved_gdbflags $GDBFLAGS +append GDBFLAGS " -ex \"set non-stop on\"" + +source $srcdir/$subdir/checkpoint.exp + +set GDBFLAGS $saved_gdbflags diff --git a/gdb/testsuite/gdb.base/checkpoint.exp b/gdb/testsuite/gdb.base/checkpoint.exp index 6d94ab6..4a476be 100644 --- a/gdb/testsuite/gdb.base/checkpoint.exp +++ b/gdb/testsuite/gdb.base/checkpoint.exp @@ -24,8 +24,10 @@ if {![istarget "*-*-linux*"]} then { continue } - -standard_testfile .c +# Must name the source file explicitly, otherwise when driven by +# checkpoints-ns.exp, we'd try compiling checkpoints-ns.c, which +# doesn't exist. +standard_testfile checkpoint.c set pi_txt [gdb_remote_download host ${srcdir}/${subdir}/pi.txt] if {[is_remote host]} { diff --git a/gdb/thread.c b/gdb/thread.c index 23dfcc9..46b5947 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1279,8 +1279,16 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level) } } +/* Data used by the cleanup installed by + 'make_cleanup_restore_current_thread'. */ + struct current_thread_cleanup { + /* Next in list of currently installed 'struct + current_thread_cleanup' cleanups. See + 'current_thread_cleanup_chain' below. */ + struct current_thread_cleanup *next; + ptid_t inferior_ptid; struct frame_id selected_frame_id; int selected_frame_level; @@ -1289,6 +1297,29 @@ struct current_thread_cleanup int was_removable; }; +/* A chain of currently installed 'struct current_thread_cleanup' + cleanups. Restoring the previously selected thread looks up the + old thread in the thread list by ptid. If the thread changes ptid, + we need to update the cleanup's thread structure so the look up + succeeds. */ +static struct current_thread_cleanup *current_thread_cleanup_chain; + +/* A thread_ptid_changed observer. Update all currently installed + current_thread_cleanup cleanups that want to switch back to + OLD_PTID to switch back to NEW_PTID instead. */ + +static void +restore_current_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) +{ + struct current_thread_cleanup *it; + + for (it = current_thread_cleanup_chain; it != NULL; it = it->next) + { + if (ptid_equal (it->inferior_ptid, old_ptid)) + it->inferior_ptid = new_ptid; + } +} + static void do_restore_current_thread_cleanup (void *arg) { @@ -1329,6 +1360,8 @@ restore_current_thread_cleanup_dtor (void *arg) struct thread_info *tp; struct inferior *inf; + current_thread_cleanup_chain = current_thread_cleanup_chain->next; + tp = find_thread_ptid (old->inferior_ptid); if (tp) tp->refcount--; @@ -1362,6 +1395,9 @@ make_cleanup_restore_current_thread (void) old->inf_id = current_inferior ()->num; old->was_removable = current_inferior ()->removable; + old->next = current_thread_cleanup_chain; + current_thread_cleanup_chain = old; + if (!ptid_equal (inferior_ptid, null_ptid)) { old->was_stopped = is_stopped (inferior_ptid); @@ -1815,4 +1851,6 @@ Show printing of thread events (such as thread start and exit)."), NULL, &setprintlist, &showprintlist); create_internalvar_type_lazy ("_thread", &thread_funcs, NULL); + + observer_attach_thread_ptid_changed (restore_current_thread_ptid_changed); }