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

[Pedro Alves <pedro at codesourcery dot com>]

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.

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

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

-- 
Pedro Alves