Re: [RFA] Checkpoint: wait the defunct process when delete it

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 12 May 2010 05:19:25, Hui Zhu wrote:
> On Wed, May 12, 2010 at 08:27, Michael Snyder <msnyder@vmware.com> wrote:
> >
> > Pedro Alves wrote:
> >>
> >> On Tuesday 11 May 2010 23:43:04, Michael Snyder wrote:
> >>
> >>
> >>>      old_cleanup = save_inferior_ptid ();
> >>>      inferior_ptid = fd->parent_ptid;
> >>>
> >>> Something like this?  Then the original inferior_ptid will be
> >>> restored when you do
> >>>
> >>>> +  if (call_function_by_hand (waitpid_fn, 3, argv) == 0)
> >>>> +    return -1;
> >>>
> >>>      do_cleanups();
> >>>
> >>>> +  return 0;
> >>>> +}
> >>
> >> That won't work.  You will hit an assertion somewhere: either because
> >> inferior_ptid is not found in the linux-nat.c lwp list, or because
> >> inferior_ptid is not found in gdb's thread list.  I believe you'll
> >> need to do a full linux_nat_switch_fork and back.
> >>
> >
> > There you go.   ;-)
> >
> 
> Hello guys,
> 
> I am sorry that I didn't complete the new patch yesterday.  Thanks for
> your help.
> 
> According to your comments. I did following change:
> 
> 1.  Before kill the ptid, GDB switch to ptid and call
> "inferior_call_getppid" to get the ppid of this inferior.  And save it
> to ppid.
> 
> 2.  before call "inferior_call_waitpid" to waitpid the ptid.
> Check if ppid is a simple thread.  ppid > 1
> Check if ppid is the GDB.  If ppid is GDB, it will auto wait the ptid.

What do you mean, it will auto wait the ptid?  AFAICS,

(gdb) checkpoint
(gdb) checkpoint
(gdb) checkpoint
(gdb) checkpoint
(gdb) checkpoint
(gdb) restart 5
(gdb) delete checkpoint 0

will still leave checkpoint 0 zombie?

>  ppid != getpid ()
> Check if ppid is stop.  is_stopped (pptid)
> 
> 3.  In function inferior_call_waitpid, before call waitpid, swith to ppid.
> 
> 4.  If inferior_call_waitpid, just give a warning to user.
> 
> Please help me review it.
> 
> Best regards,
> Hui
> 
> 2010-05-12  Hui Zhu  <teawater@gmail.com>
> 
> 	* linux-fork.c (gdbthread.h): New include.
> 	(inferior_call_getppid, inferior_call_waitpid): New function.
> 	(delete_checkpoint_command): Call inferior_call_getppid
> 	and inferior_call_waitpid.
> 
> 
> ---
>  linux-fork.c |  105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 104 insertions(+), 1 deletion(-)
> 
> --- a/linux-fork.c
> +++ b/linux-fork.c
> @@ -29,6 +29,7 @@
>  #include "gdb_string.h"
>  #include "linux-fork.h"
>  #include "linux-nat.h"
> +#include "gdbthread.h"
> 
>  #include <sys/ptrace.h>
>  #include "gdb_wait.h"
> @@ -410,12 +411,105 @@ linux_fork_detach (char *args, int from_
>      delete_fork (inferior_ptid);
>  }
> 
> +static int
> +inferior_call_getppid (ptid_t ptid)
> +{
> +  struct objfile *getppid_objf;
> +  struct value *getppid_fn = NULL, *ret;
> +  struct value *argv[4];
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  struct fork_info *oldfp, *newfp;
> +  int ppid = 1;
> +
> +  /* Switch to ptid.  */
> +  oldfp = find_fork_ptid (inferior_ptid);
> +  gdb_assert (oldfp != NULL);
> +  newfp = find_fork_ptid (ptid);
> +  gdb_assert (oldfp != NULL);
> +  fork_save_infrun_state (oldfp, 1);
> +  remove_breakpoints ();
> +  fork_load_infrun_state (newfp);
> +  insert_breakpoints ();
> +
> +  /* Get the getppid_fn.  */
> +  if (lookup_minimal_symbol ("getppid", NULL, NULL) != NULL)
> +    getppid_fn = find_function_in_inferior ("getppid", &getppid_objf);
> +  if (!getppid_fn && lookup_minimal_symbol ("_getppid", NULL, NULL) != NULL)
> +    getppid_fn = find_function_in_inferior ("_getppid", &getppid_objf);
> +  if (!getppid_fn)
> +    return 1;
> +
> +  ret = call_function_by_hand (getppid_fn, 0, NULL);
> +  if (ret == 0)
> +    return ppid;

??? can getppid really return 0 ?

> +  ppid = value_as_long (ret);
> +
> +  /* Switch back to inferior_ptid. */
> +  remove_breakpoints ();
> +  fork_load_infrun_state (oldfp);
> +  insert_breakpoints ();
> +
> +  return ppid;
> +}

I don't think calling getppid is a great idea.  You may not
even be debugging the parent process of the process you are
about to kill!  E.g., say you attach to a process, create a checkpoint,
restart the checkpoint, and delete checkpoint 0.  getppid will return
a pid of a process you are _not_ debugging.  You should at least
check for that.  But, why didn't storing the parent pid
at fork time (like was suggested before), work?

> +
> +static int
> +inferior_call_waitpid (ptid_t pptid, int pid)
> +{
> +  struct objfile *waitpid_objf;
> +  struct value *waitpid_fn = NULL;
> +  struct value *argv[4];
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  struct fork_info *oldfp = NULL, *newfp = NULL;
> +  int ret = 0;
> +
> +  if (!ptid_equal (pptid, inferior_ptid))
> +    {
> +      /* Switch to pptid.  */
> +      oldfp = find_fork_ptid (inferior_ptid);
> +      gdb_assert (oldfp != NULL);
> +      newfp = find_fork_ptid (pptid);
> +      gdb_assert (oldfp != NULL);
> +      fork_save_infrun_state (oldfp, 1);
> +      remove_breakpoints ();
> +      fork_load_infrun_state (newfp);
> +      insert_breakpoints ();
> +    }
> +
> +  /* Get the waitpid_fn.  */
> +  if (lookup_minimal_symbol ("waitpid", NULL, NULL) != NULL)
> +    waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
> +  if (!waitpid_fn && lookup_minimal_symbol ("_waitpid", NULL, NULL) != NULL)
> +    waitpid_fn = find_function_in_inferior ("_waitpid", &waitpid_objf);
> +  if (!waitpid_fn)
> +    return -1;

This early return doesn't switch back to oldfp...  I'd prefer to
have this switching in and out done by the caller of this function.
Can you do that change please?

> +
> +  /* Get the argv.  */
> +  argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int, pid);
> +  argv[1] = value_from_longest (builtin_type (gdbarch)->builtin_data_ptr, 0);

value_from_pointer

> +  argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
> +  argv[3] = 0;
> +
> +  if (call_function_by_hand (waitpid_fn, 3, argv) == 0)
> +    ret = -1;
> +
> +  if (oldfp)
> +    {
> +      /* Switch back to inferior_ptid. */
> +      remove_breakpoints ();
> +      fork_load_infrun_state (oldfp);
> +      insert_breakpoints ();
> +    }
> +
> +  return ret;
> +}
> +
>  /* Fork list <-> user interface.  */
> 
>  static void
>  delete_checkpoint_command (char *args, int from_tty)
>  {
> -  ptid_t ptid;
> +  ptid_t ptid, pptid;
> +  int ppid;
> 
>    if (!args || !*args)
>      error (_("Requires argument (checkpoint id to delete)"));
> @@ -428,9 +522,18 @@ delete_checkpoint_command (char *args, i
>      error (_("\
>  Please switch to another checkpoint before deleting the current one"));
> 
> +  ppid = inferior_call_getppid (ptid);
> +  pptid = ptid_build (ppid, ppid, 0);
> +
>    if (ptrace (PTRACE_KILL, PIDGET (ptid), 0, 0))
>      error (_("Unable to kill pid %s"), target_pid_to_str (ptid));
> 
> +  if (ppid > 1 && ppid != getpid () && is_stopped (pptid))
> +    {
> +      if (inferior_call_waitpid (pptid, PIDGET (ptid)))
> +        warning (_("Unable to wait pid %s"), target_pid_to_str (ptid));
> +    }
> +
>    if (from_tty)
>      printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));
> 


-- 
Pedro Alves