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

[Hui Zhu <teawater@gmail.com>]

Thanks Pedro.

On Fri, May 14, 2010 at 20:20, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 14 May 2010 07:39:17, Hui Zhu wrote:
>
>> > 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);
>
> You're now accessing the FI pointer after having just deleted it.  The
> delete_fork(ptid) line deletes FI.

Faint.  I will fix it.

>
>> +  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 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;
>
>    if (...)
>      {
>> +      old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
>> +    }
>> +
>> +  if (old_cleanup)
>> +    do_cleanups (old_cleanup);
>
> Please do not use this check-for-NULL-cleanup pattern anywhere.
> You should install a null_cleanup instead, and unconditionally run
> the cleanups.  You should _never_ rely on old_cleanup being
> NULL to mean you haven't installed a cleanup above.  The reason
> is that the
>
>   old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
>
> line may well return NULL, because NULL was the head of
> the cleanup chain when the first cleanup is registered, and that's
> what make_cleanup returns, the previous head of the chain.
>
> To convince yourself, put a breakpoint on make_my_cleanup
> and run gdb.  Step out and check what is the old_chain value
> that ends up stored in the caller.


Thanks for told it so clear.   I will fix it.

>
>> +  return ret;
>> +}
>
> I see you dropped extracting the return value of the waitpid
> call, in addition to the dead code.  Was that on purpose?  It's
> fine either way, just pointing it out in case it slipped.

In before I just want check the return value of call_function_by_hand
but didn't check the waitpid return value.  So I just removed the
check code.

But add check for waitpid is not a big work, so I will add code for it.

>
> --
> Pedro Alves
>
I make a patch for cvs-head.  Please help me review it.

Best regards,
Hui

2010-05-14  Hui Zhu  <teawater@gmail.com>

	* linux-fork.c (inferior_call_waitpid_cleanup): Add check
	for oldfp.
	(inferior_call_waitpid): Move make_cleanup out of check.
	Check the return of waitpid.
	(delete_checkpoint_command): Add pptid to save fi->parent_ptid.


---
 linux-fork.c |   35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

--- a/linux-fork.c
+++ b/linux-fork.c
@@ -417,10 +417,13 @@ 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 ();
+  if (oldfp)
+    {
+      /* Switch back to inferior_ptid. */
+      remove_breakpoints ();
+      fork_load_infrun_state (oldfp);
+      insert_breakpoints ();
+    }
 }

 static int
@@ -428,10 +431,10 @@ inferior_call_waitpid (ptid_t pptid, int
 {
   struct objfile *waitpid_objf;
   struct value *waitpid_fn = NULL;
-  struct value *argv[4];
+  struct value *argv[4], *retv;
   struct gdbarch *gdbarch = get_current_arch ();
   struct fork_info *oldfp = NULL, *newfp = NULL;
-  struct cleanup *old_cleanup = NULL;
+  struct cleanup *old_cleanup;
   int ret = -1;

   if (!ptid_equal (pptid, inferior_ptid))
@@ -445,10 +448,10 @@ inferior_call_waitpid (ptid_t pptid, int
       remove_breakpoints ();
       fork_load_infrun_state (newfp);
       insert_breakpoints ();
-
-      old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
     }

+  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);
@@ -463,13 +466,14 @@ inferior_call_waitpid (ptid_t pptid, int
   argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
   argv[3] = 0;

-  call_function_by_hand (waitpid_fn, 3, argv);
+  retv = call_function_by_hand (waitpid_fn, 3, argv);
+  if (value_as_long (retv) < 0)
+    goto out;

   ret = 0;

 out:
-  if (old_cleanup)
-    do_cleanups (old_cleanup);
+  do_cleanups (old_cleanup);
   return ret;
 }

@@ -478,7 +482,7 @@ out:
 static void
 delete_checkpoint_command (char *args, int from_tty)
 {
-  ptid_t ptid;
+  ptid_t ptid, pptid;
   struct fork_info *fi;

   if (!args || !*args)
@@ -497,6 +501,7 @@ Please switch to another checkpoint befo

   fi = find_fork_ptid (ptid);
   gdb_assert (fi);
+  pptid = fi->parent_ptid;

   if (from_tty)
     printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));
@@ -507,10 +512,10 @@ Please switch to another checkpoint befo
      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 ((!find_thread_ptid (pptid) && find_fork_ptid (pptid))
+      || (find_thread_ptid (pptid) && is_stopped (pptid)))
     {
-      if (inferior_call_waitpid (fi->parent_ptid, PIDGET (ptid)))
+      if (inferior_call_waitpid (pptid, PIDGET (ptid)))
         warning (_("Unable to wait pid %s"), target_pid_to_str (ptid));
     }
 }