From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20553 invoked by alias); 14 May 2010 06:39:46 -0000 Received: (qmail 20540 invoked by uid 22791); 14 May 2010 06:39:44 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,TW_BJ,TW_DF,TW_EG,TW_TP X-Spam-Check-By: sourceware.org Received: from mail-pw0-f41.google.com (HELO mail-pw0-f41.google.com) (209.85.160.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 14 May 2010 06:39:39 +0000 Received: by pwi7 with SMTP id 7so1838809pwi.0 for ; Thu, 13 May 2010 23:39:37 -0700 (PDT) Received: by 10.142.122.7 with SMTP id u7mr403891wfc.212.1273819177496; Thu, 13 May 2010 23:39:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.143.4.9 with HTTP; Thu, 13 May 2010 23:39:17 -0700 (PDT) In-Reply-To: <201005131130.57597.pedro@codesourcery.com> References: <201005121937.44287.pedro@codesourcery.com> <201005131130.57597.pedro@codesourcery.com> From: Hui Zhu Date: Fri, 14 May 2010 11:05: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/msg00297.txt.bz2 On Thu, May 13, 2010 at 18:30, Pedro Alves wrote: > > On Thursday 13 May 2010 08:58:49, Hui Zhu wrote: > > >> 2. =A0before call "inferior_call_waitpid" to waitpid the ptid. > > >> Check if ppid is a simple thread. =A0ppid > 1 > > >> Check if ppid is the GDB. =A0If ppid is GDB, it will auto wait the p= tid. > > > > > > What do you mean, it will auto wait the ptid? =A0AFAICS, > > > > > > (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. =A0GDB will auto wait > > the zombie, so I just leave them there let GDB hanle it. > > You didn't answer the question. =A0Please point me at where is > this "gdb auto waiting". > > I actually don't understand that rationale. =A0The `init' process > will "auto wait" =A0the 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 sur= e if > > + =A0if ((!find_thread_ptid (fi->parent_ptid) && find_fork_ptid (fi->pa= rent_ptid)) > > + =A0 =A0 =A0|| (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->= parent_ptid))) > > This requires an explaning comment in the code. OK. I will add them. > > > >> + > > >> + =A0ret =3D call_function_by_hand (getppid_fn, 0, NULL); > > >> + =A0if (ret =3D=3D 0) > > >> + =A0 =A0return ppid; > > > > > > ??? can getppid really return 0 ? > > > > This 0 is not the return value of getppid. > > Oh, right. =A0:-) > > > This is how function "checkpoint_command" use this function. =A0Check it > > just for code safe. =A0:) > > =A0 ret =3D call_function_by_hand (fork_fn, 0, &ret); > > =A0 do_cleanups (old_chain); > > =A0 if (!ret) =A0 =A0 /* Probably can't happen. =A0*/ > > =A0 =A0 error (_("checkpoint: call_function_by_hand returned null.")); > > Well, the only return from call_function_by_hand does: > > =A0 =A0gdb_assert (retval); > =A0 =A0return retval; > > All other cases are handled by throwing an error. =A0 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. =A0You 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 Michael Snyder * 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 #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 =3D 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 =3D NULL; + struct value *argv[4]; + struct gdbarch *gdbarch =3D get_current_arch (); + struct fork_info *oldfp =3D NULL, *newfp =3D NULL; + struct cleanup *old_cleanup =3D NULL; + int ret =3D -1; + + if (!ptid_equal (pptid, inferior_ptid)) + { + /* Switch to pptid. */ + oldfp =3D find_fork_ptid (inferior_ptid); + gdb_assert (oldfp !=3D NULL); + newfp =3D find_fork_ptid (pptid); + gdb_assert (oldfp !=3D NULL); + fork_save_infrun_state (oldfp, 1); + remove_breakpoints (); + fork_load_infrun_state (newfp); + insert_breakpoints (); + + 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); + if (!waitpid_fn && lookup_minimal_symbol ("_waitpid", NULL, NULL) !=3D N= ULL) + waitpid_fn =3D find_function_in_inferior ("_waitpid", &waitpid_objf); + if (!waitpid_fn) + goto out; + + /* Get the argv. */ + argv[0] =3D value_from_longest (builtin_type (gdbarch)->builtin_int, pid= ); + argv[1] =3D value_from_pointer (builtin_type (gdbarch)->builtin_data_ptr= , 0); + argv[2] =3D value_from_longest (builtin_type (gdbarch)->builtin_int, 0); + argv[3] =3D 0; + + call_function_by_hand (waitpid_fn, 3, argv); + + ret =3D 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 =3D 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_p= tid)) + || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_pti= d))) + { + 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 =3D last_target_ptid; } static void