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

Doug Evans dje@google.com
Tue Apr 21 02:36:00 GMT 2015


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



More information about the Gdb-patches mailing list