From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13560 invoked by alias); 14 May 2010 14:43:52 -0000 Received: (qmail 13544 invoked by uid 22791); 14 May 2010 14:43:50 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,TW_BJ X-Spam-Check-By: sourceware.org Received: from mail-pv0-f169.google.com (HELO mail-pv0-f169.google.com) (74.125.83.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 14 May 2010 14:43:44 +0000 Received: by pvg13 with SMTP id 13so171967pvg.0 for ; Fri, 14 May 2010 07:43:43 -0700 (PDT) Received: by 10.142.152.29 with SMTP id z29mr832152wfd.30.1273848223201; Fri, 14 May 2010 07:43:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.143.39.6 with HTTP; Fri, 14 May 2010 07:43:23 -0700 (PDT) In-Reply-To: <201005141320.09678.pedro@codesourcery.com> References: <201005131130.57597.pedro@codesourcery.com> <201005141320.09678.pedro@codesourcery.com> From: Hui Zhu Date: Fri, 14 May 2010 15:08:00 -0000 Message-ID: Subject: Re: [RFA] Checkpoint: wait the defunct process when delete it To: Pedro Alves Cc: Michael Snyder , "gdb-patches@sourceware.org" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-05/txt/msg00300.txt.bz2 Thanks Pedro. On Fri, May 14, 2010 at 20:20, Pedro Alves wrote: > On Friday 14 May 2010 07:39:17, Hui Zhu wrote: > >> > BTW, you patch is not error/exception safe. =A0You didn't consider >> > the case of an `error' being thrown between switching the >> > fork and back. >> >> Thanks for alarm me about it. =A0I have add a >> inferior_call_waitpid_cleanup to clean up And move >> inferior_call_waitpid to the end of function >> delete_checkpoint_command. =A0Then it will not affect inferior_ptid and >> delete_fork (ptid); > > You're now accessing the FI pointer after having just deleted it. =A0The > delete_fork(ptid) line deletes FI. Faint. I will fix it. > >> + =A0fi =3D find_fork_ptid (ptid); >> + =A0gdb_assert (fi); >> + >> =A0 =A0if (from_tty) >> =A0 =A0 =A0printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid)); >> >> =A0 =A0delete_fork (ptid); >> + >> + =A0/* If fi->parent_ptid is not a part of lwp but it's a part of check= point >> + =A0 =A0 list, waitpid the ptid. >> + =A0 =A0 If fi->parent_ptid is a part of lwp and it is stoped, waitpid = the >> + =A0 =A0 ptid. =A0*/ >> + =A0if ((!find_thread_ptid (fi->parent_ptid) && find_fork_ptid (fi->par= ent_ptid)) >> + =A0 =A0 =A0|| (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->p= arent_ptid))) >> + =A0 =A0{ >> + =A0 =A0 =A0if (inferior_call_waitpid (fi->parent_ptid, PIDGET (ptid))) >> + =A0 =A0 =A0 =A0warning (_("Unable to wait pid %s"), target_pid_to_str = (ptid)); >> + =A0 =A0} >> =A0} > > >> +static int >> +inferior_call_waitpid (ptid_t pptid, int pid) >> +{ >> + =A0struct objfile *waitpid_objf; >> + =A0struct value *waitpid_fn =3D NULL; >> + =A0struct value *argv[4]; >> + =A0struct gdbarch *gdbarch =3D get_current_arch (); >> + =A0struct fork_info *oldfp =3D NULL, *newfp =3D NULL; >> + =A0struct cleanup *old_cleanup =3D NULL; > > =A0 =A0if (...) > =A0 =A0 =A0{ >> + =A0 =A0 =A0old_cleanup =3D make_cleanup (inferior_call_waitpid_cleanup= , oldfp); >> + =A0 =A0} >> + >> + =A0if (old_cleanup) >> + =A0 =A0do_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. =A0You should _never_ rely on old_cleanup being > NULL to mean you haven't installed a cleanup above. =A0The reason > is that the > > =A0 old_cleanup =3D 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. =A0Step 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. > >> + =A0return ret; >> +} > > I see you dropped extracting the return value of the waitpid > call, in addition to the dead code. =A0Was that on purpose? =A0It'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 * 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 =3D 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 =3D NULL; - struct value *argv[4]; + struct value *argv[4], *retv; struct gdbarch *gdbarch =3D get_current_arch (); struct fork_info *oldfp =3D NULL, *newfp =3D NULL; - struct cleanup *old_cleanup =3D NULL; + struct cleanup *old_cleanup; int ret =3D -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 =3D make_cleanup (inferior_call_waitpid_cleanup, oldfp); } + old_cleanup =3D make_cleanup (inferior_call_waitpid_cleanup, oldfp); + /* Get the waitpid_fn. */ if (lookup_minimal_symbol ("waitpid", NULL, NULL) !=3D NULL) waitpid_fn =3D find_function_in_inferior ("waitpid", &waitpid_objf); @@ -463,13 +466,14 @@ inferior_call_waitpid (ptid_t pptid, int argv[2] =3D value_from_longest (builtin_type (gdbarch)->builtin_int, 0); argv[3] =3D 0; - call_function_by_hand (waitpid_fn, 3, argv); + retv =3D call_function_by_hand (waitpid_fn, 3, argv); + if (value_as_long (retv) < 0) + goto out; ret =3D 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 =3D find_fork_ptid (ptid); gdb_assert (fi); + pptid =3D 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_p= tid)) - || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_pti= d))) + 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)); } }