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

[Hui Zhu <teawater@gmail.com>]

On Thu, May 13, 2010 at 18:30, Pedro Alves <pedro@codesourcery.com> wrote:
>
> On Thursday 13 May 2010 08:58:49, Hui Zhu wrote:
> > >> 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?
> >
> > This is because the parent of checkpoint is GDB.  GDB will auto wait
> > the zombie, so I just leave them there let GDB hanle it.
>
> You didn't answer the question.  Please point me at where is
> this "gdb auto waiting".
>
> I actually don't understand that rationale.  The `init' process
> will "auto wait"  the checkpoint forks as well, so why bother in
> the first place then?
>

my_waitpid with -1 will handle them.

And looks if we don't call getppid, we will not have a good way to make sure if

> > +  if ((!find_thread_ptid (fi->parent_ptid) && find_fork_ptid (fi->parent_ptid))
> > +      || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid)))
>
> This requires an explaning comment in the code.

OK.  I will add them.

>
> > >> +
> > >> +  ret = call_function_by_hand (getppid_fn, 0, NULL);
> > >> +  if (ret == 0)
> > >> +    return ppid;
> > >
> > > ??? can getppid really return 0 ?
> >
> > This 0 is not the return value of getppid.
>
> Oh, right.  :-)
>
> > This is how function "checkpoint_command" use this function.  Check it
> > just for code safe.  :)
> >   ret = call_function_by_hand (fork_fn, 0, &ret);
> >   do_cleanups (old_chain);
> >   if (!ret)     /* Probably can't happen.  */
> >     error (_("checkpoint: call_function_by_hand returned null."));
>
> Well, the only return from call_function_by_hand does:
>
>    gdb_assert (retval);
>    return retval;
>
> All other cases are handled by throwing an error.   So
> "definitely can't happen"; please remove the new dead code
> you're adding.

Thanks.  I will fix it.

>
> BTW, you patch is not error/exception safe.  You didn't consider
> the case of an `error' being thrown between switching the
> fork and back.

Thanks for alarm me about it.  I have add a
inferior_call_waitpid_cleanup to clean up And move
inferior_call_waitpid to the end of function
delete_checkpoint_command.  Then it will not affect inferior_ptid and
delete_fork (ptid);

>
> Otherwise, the patch looks good.


Thanks.  I fixed them all and checked in.

Best regards,
Hui

2010-05-14  Hui Zhu  <teawater@gmail.com>
            Michael Snyder  <msnyder@vmware.com>

	* linux-fork.c (gdbthread.h): New include.
	(fork_info): Add parent_ptid.
	(inferior_call_waitpid_cleanup, inferior_call_waitpid): New
	functions.
	(delete_checkpoint_command): Call inferior_call_waitpid.
	(checkpoint_command): Set parent_ptid.



---
 linux-fork.c |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

--- 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"
@@ -47,6 +48,7 @@ struct fork_info
 {
   struct fork_info *next;
   ptid_t ptid;
+  ptid_t parent_ptid;
   int num;			/* Convenient handle (GDB fork id) */
   struct regcache *savedregs;	/* Convenient for info fork, saves
 				   having to actually switch contexts.  */
@@ -410,12 +412,74 @@ linux_fork_detach (char *args, int from_
     delete_fork (inferior_ptid);
 }

+static void
+inferior_call_waitpid_cleanup (void *fp)
+{
+  struct fork_info *oldfp = fp;
+
+  /* Switch back to inferior_ptid. */
+  remove_breakpoints ();
+  fork_load_infrun_state (oldfp);
+  insert_breakpoints ();
+}
+
+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;
+  struct cleanup *old_cleanup = NULL;
+  int ret = -1;
+
+  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 ();
+
+      old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
+    }
+
+  /* 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)
+    goto out;
+
+  /* Get the argv.  */
+  argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int, pid);
+  argv[1] = value_from_pointer (builtin_type (gdbarch)->builtin_data_ptr, 0);
+  argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
+  argv[3] = 0;
+
+  call_function_by_hand (waitpid_fn, 3, argv);
+
+  ret = 0;
+
+out:
+  if (old_cleanup)
+    do_cleanups (old_cleanup);
+  return ret;
+}
+
 /* Fork list <-> user interface.  */

 static void
 delete_checkpoint_command (char *args, int from_tty)
 {
   ptid_t ptid;
+  struct fork_info *fi;

   if (!args || !*args)
     error (_("Requires argument (checkpoint id to delete)"));
@@ -431,10 +495,24 @@ Please switch to another checkpoint befo
   if (ptrace (PTRACE_KILL, PIDGET (ptid), 0, 0))
     error (_("Unable to kill pid %s"), target_pid_to_str (ptid));

+  fi = find_fork_ptid (ptid);
+  gdb_assert (fi);
+
   if (from_tty)
     printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));

   delete_fork (ptid);
+
+  /* If fi->parent_ptid is not a part of lwp but it's a part of checkpoint
+     list, waitpid the ptid.
+     If fi->parent_ptid is a part of lwp and it is stoped, waitpid the
+     ptid.  */
+  if ((!find_thread_ptid (fi->parent_ptid) && find_fork_ptid (fi->parent_ptid))
+      || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid)))
+    {
+      if (inferior_call_waitpid (fi->parent_ptid, PIDGET (ptid)))
+        warning (_("Unable to wait pid %s"), target_pid_to_str (ptid));
+    }
 }

 static void
@@ -596,6 +674,7 @@ checkpoint_command (char *args, int from
   if (!fp)
     error (_("Failed to find new fork"));
   fork_save_infrun_state (fp, 1);
+  fp->parent_ptid = last_target_ptid;
 }

 static void