[RFA] Checkpoint: wait the defunct process when delete it
Hui Zhu
teawater@gmail.com
Fri May 14 15:08:00 GMT 2010
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));
}
}
More information about the Gdb-patches
mailing list