Re: [PATCH v3 01/17] Fix and test "checkpoint" in non-stop mode

Doug Evans
2015-04-21T02:36:04Z

Pedro Alves writes:
> 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.

Hi.

What does "target may still have execution" mean here?
Is it that there can be another inferior running?

> 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.

I'm guessing this is less hacky than it sounds :-),
but it does make me want to understand why this only comes up
now since threads/inferiors have always been able to "go away"
while gdb is doing something.

> 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 over 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 sources checkpoint.exp, and sets a knob that checkpoint.exp
> reads to know it should test non-stop mode. No other test in the tree
> currently uses this mechanism, but I can't see a reason we shouldn't
> do this.

Re: checkpoint-ns.exp:
If we're going to have one "make check" involve both all-stop
and non-stop testing (as opposed to doing one "make check" with all-stop
and another one with non-stop), I'd rather see a more table-driven approach
(the details don't matter to me much other than I'd rather have one file
than a proliferation of foo-ns.exp files).
Parallelizing that may involve an extra step, but that's ok.

>
> gdb/ChangeLog:
> 2015-04-17 Pedro Alves <palves@redhat.com>
>
> * 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) <next>: 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-04-17 Pedro Alves <palves@redhat.com>
>
> * gdb.base/checkpoint-ns.exp: New file.
> * gdb.base/checkpoint.exp (checkpoint_non_stop): New input
> variable. If set, test in non-stop mode. Pass explicit
> "checkpoint.c" to standard_testfile.
>
> v3:
> No changes.
> ---
> gdb/infrun.c | 31 +++++++++++++++++++++-----
> gdb/testsuite/gdb.base/checkpoint-ns.exp | 27 +++++++++++++++++++++++
> gdb/testsuite/gdb.base/checkpoint.exp | 31 +++++++++++++++++++++-----
> gdb/thread.c | 38 ++++++++++++++++++++++++++++++++
> 4 files changed, 117 insertions(+), 10 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/checkpoint-ns.exp
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 7870f70..9c1fdc5 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3804,8 +3804,18 @@ handle_inferior_event (struct execution_control_state *ecs)
> any other process were left running. */
> 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)

pasto: s/&&/||/

> + {
> + ptid_t pid_ptid;
> +
> + /* Some targets still have execution when a process exits.
> + E.g., for "checkpoint", when when a fork exits and is

s/when when/when/

> + mourned, linux-fork.c switches to another fork. */
> + pid_ptid = pid_to_ptid (ptid_get_pid (ecs->ptid));
> + set_executing (pid_ptid, 0);

This is a bit confusing. I'm guessing ecs->ptid is for the inferior
that just exited/signalled, but we're not changing pids here.
I'm guessing we'll mourn the inferior later, but IWBN to elaborate
on the comment a little, e.g., to say we always need to call set_executing
here, even if the inferior exited/signalled (which the code didn't
previously do). But it's not clear why "switches to another fork" comes
into play here.

> + }
> + else
> set_executing (ecs->ptid, 0);
>
> switch (ecs->ws.kind)
> @@ -6550,6 +6560,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);
>
> @@ -6559,9 +6570,19 @@ 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)
> + {
> + /* Some targets still have execution when a process exits.
> + E.g., for "checkpoint", when when a fork exits and is

s/when when/when/

> + mourned, linux-fork.c switches to another fork. */
> + 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..03a1747
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/checkpoint-ns.exp
> @@ -0,0 +1,27 @@
> +# 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 <http://www.gnu.org/licenses/>. */
> +
> +# 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.
> +
> +# checkpoint.exp reads this variable.
> +set checkpoint_non_stop 1
> +
> +source $srcdir/$subdir/checkpoint.exp
> +
> +# Unset to avoid problems with non-parallel mode.
> +unset checkpoint_non_stop
> diff --git a/gdb/testsuite/gdb.base/checkpoint.exp b/gdb/testsuite/gdb.base/checkpoint.exp
> index 6d94ab6..c297b50 100644
> --- a/gdb/testsuite/gdb.base/checkpoint.exp
> +++ b/gdb/testsuite/gdb.base/checkpoint.exp
> @@ -13,6 +13,20 @@
> # You should have received a copy of the GNU General Public License
> # along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> +#
> +# This tests gdb checkpoint and restart.
> +#
> +
> +# Note this file is also sourced by checkpoint-ns.exp to run all the
> +# tests below in non-stop mode. That's driven by a separate file so
> +# testing both all-stop and non-stop can be done in parallel.
> +
> +# This is the variable checkpoint-ns.exp sets to request non-stop
> +# mode. When not set, default to all-stop mode.
> +if {![info exists checkpoint_non_stop]} {
> + set checkpoint_non_stop 0
> +}
> +
> if { [is_remote target] || ![isnative] } then {
> continue
> }
> @@ -24,8 +38,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]} {
> @@ -42,9 +58,9 @@ if {[prepare_for_testing ${testfile}.exp $testfile $srcfile \
>
> global gdb_prompt
>
> -#
> -# This tests gdb checkpoint and restart.
> -#
> +if {$checkpoint_non_stop} {
> + gdb_test_no_output "set non-stop on"
> +}
>
> runto_main
> set break1_loc [gdb_get_line_number "breakpoint 1"]
> @@ -327,6 +343,11 @@ gdb_start
> gdb_reinitialize_dir $srcdir/$subdir
> gdb_load ${binfile}
>
> +if {$checkpoint_non_stop} {
> + gdb_test_no_output "set non-stop on" \
> + "set non-stop on, for many checkpoints"
> +}
> +
> runto_main
> gdb_breakpoint $break1_loc
>
> 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);
> }
> --
> 1.9.3
>

--
/dje