From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31842 invoked by alias); 13 May 2010 07:59:20 -0000 Received: (qmail 31827 invoked by uid 22791); 13 May 2010 07:59:18 -0000 X-SWARE-Spam-Status: No, hits=-1.7 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-pv0-f169.google.com (HELO mail-pv0-f169.google.com) (74.125.83.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 13 May 2010 07:59:11 +0000 Received: by pva4 with SMTP id 4so608882pva.0 for ; Thu, 13 May 2010 00:59:09 -0700 (PDT) Received: by 10.142.201.15 with SMTP id y15mr6110442wff.247.1273737549168; Thu, 13 May 2010 00:59:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.143.4.9 with HTTP; Thu, 13 May 2010 00:58:49 -0700 (PDT) In-Reply-To: <201005121937.44287.pedro@codesourcery.com> References: <4BE9F5EF.2050405@vmware.com> <201005121937.44287.pedro@codesourcery.com> From: Hui Zhu Date: Thu, 13 May 2010 09:11: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/msg00269.txt.bz2 On Thu, May 13, 2010 at 02:37, Pedro Alves wrote: > On Wednesday 12 May 2010 05:19:25, Hui Zhu wrote: >> On Wed, May 12, 2010 at 08:27, Michael Snyder wrote: >> > >> > Pedro Alves wrote: >> >> >> >> On Tuesday 11 May 2010 23:43:04, Michael Snyder wrote: >> >> >> >> >> >>> =A0 =A0 =A0old_cleanup =3D save_inferior_ptid (); >> >>> =A0 =A0 =A0inferior_ptid =3D fd->parent_ptid; >> >>> >> >>> Something like this? =A0Then the original inferior_ptid will be >> >>> restored when you do >> >>> >> >>>> + =A0if (call_function_by_hand (waitpid_fn, 3, argv) =3D=3D 0) >> >>>> + =A0 =A0return -1; >> >>> >> >>> =A0 =A0 =A0do_cleanups(); >> >>> >> >>>> + =A0return 0; >> >>>> +} >> >> >> >> That won't work. =A0You will hit an assertion somewhere: either becau= se >> >> inferior_ptid is not found in the linux-nat.c lwp list, or because >> >> inferior_ptid is not found in gdb's thread list. =A0I believe you'll >> >> need to do a full linux_nat_switch_fork and back. >> >> >> > >> > There you go. =A0 ;-) >> > >> >> Hello guys, >> >> I am sorry that I didn't complete the new patch yesterday. =A0Thanks for >> your help. >> >> According to your comments. I did following change: >> >> 1. =A0Before kill the ptid, GDB switch to ptid and call >> "inferior_call_getppid" to get the ppid of this inferior. =A0And save it >> to ppid. >> >> 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 ptid. > > 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. GDB will auto wait the zombie, so I just leave them there let GDB hanle it. > >> =A0ppid !=3D getpid () >> Check if ppid is stop. =A0is_stopped (pptid) >> >> 3. =A0In function inferior_call_waitpid, before call waitpid, swith to p= pid. >> >> 4. =A0If inferior_call_waitpid, just give a warning to user. >> >> Please help me review it. >> >> Best regards, >> Hui >> >> 2010-05-12 =A0Hui Zhu =A0 >> >> =A0 =A0 =A0 * linux-fork.c (gdbthread.h): New include. >> =A0 =A0 =A0 (inferior_call_getppid, inferior_call_waitpid): New function. >> =A0 =A0 =A0 (delete_checkpoint_command): Call inferior_call_getppid >> =A0 =A0 =A0 and inferior_call_waitpid. >> >> >> --- >> =A0linux-fork.c | =A0105 +++++++++++++++++++++++++++++++++++++++++++++++= +++++++++++- >> =A01 file changed, 104 insertions(+), 1 deletion(-) >> >> --- a/linux-fork.c >> +++ b/linux-fork.c >> @@ -29,6 +29,7 @@ >> =A0#include "gdb_string.h" >> =A0#include "linux-fork.h" >> =A0#include "linux-nat.h" >> +#include "gdbthread.h" >> >> =A0#include >> =A0#include "gdb_wait.h" >> @@ -410,12 +411,105 @@ linux_fork_detach (char *args, int from_ >> =A0 =A0 =A0delete_fork (inferior_ptid); >> =A0} >> >> +static int >> +inferior_call_getppid (ptid_t ptid) >> +{ >> + =A0struct objfile *getppid_objf; >> + =A0struct value *getppid_fn =3D NULL, *ret; >> + =A0struct value *argv[4]; >> + =A0struct gdbarch *gdbarch =3D get_current_arch (); >> + =A0struct fork_info *oldfp, *newfp; >> + =A0int ppid =3D 1; >> + >> + =A0/* Switch to ptid. =A0*/ >> + =A0oldfp =3D find_fork_ptid (inferior_ptid); >> + =A0gdb_assert (oldfp !=3D NULL); >> + =A0newfp =3D find_fork_ptid (ptid); >> + =A0gdb_assert (oldfp !=3D NULL); >> + =A0fork_save_infrun_state (oldfp, 1); >> + =A0remove_breakpoints (); >> + =A0fork_load_infrun_state (newfp); >> + =A0insert_breakpoints (); >> + >> + =A0/* Get the getppid_fn. =A0*/ >> + =A0if (lookup_minimal_symbol ("getppid", NULL, NULL) !=3D NULL) >> + =A0 =A0getppid_fn =3D find_function_in_inferior ("getppid", &getppid_o= bjf); >> + =A0if (!getppid_fn && lookup_minimal_symbol ("_getppid", NULL, NULL) != =3D NULL) >> + =A0 =A0getppid_fn =3D find_function_in_inferior ("_getppid", &getppid_= objf); >> + =A0if (!getppid_fn) >> + =A0 =A0return 1; >> + >> + =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. This is how function "checkpoint_command" use this function. Check it just for code safe. :) ret =3D call_function_by_hand (fork_fn, 0, &ret); do_cleanups (old_chain); if (!ret) /* Probably can't happen. */ error (_("checkpoint: call_function_by_hand returned null.")); > >> + =A0ppid =3D value_as_long (ret); >> + >> + =A0/* Switch back to inferior_ptid. */ >> + =A0remove_breakpoints (); >> + =A0fork_load_infrun_state (oldfp); >> + =A0insert_breakpoints (); >> + >> + =A0return ppid; >> +} > > I don't think calling getppid is a great idea. =A0You may not > even be debugging the parent process of the process you are > about to kill! =A0E.g., say you attach to a process, create a checkpoint, > restart the checkpoint, and delete checkpoint 0. =A0getppid will return > a pid of a process you are _not_ debugging. =A0You should at least > check for that. =A0But, why didn't storing the parent pid > at fork time (like was suggested before), work? > I agree with you. getppid is too cumbersome. I will update the patch. >> + >> +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; >> + =A0int ret =3D 0; >> + >> + =A0if (!ptid_equal (pptid, inferior_ptid)) >> + =A0 =A0{ >> + =A0 =A0 =A0/* Switch to pptid. =A0*/ >> + =A0 =A0 =A0oldfp =3D find_fork_ptid (inferior_ptid); >> + =A0 =A0 =A0gdb_assert (oldfp !=3D NULL); >> + =A0 =A0 =A0newfp =3D find_fork_ptid (pptid); >> + =A0 =A0 =A0gdb_assert (oldfp !=3D NULL); >> + =A0 =A0 =A0fork_save_infrun_state (oldfp, 1); >> + =A0 =A0 =A0remove_breakpoints (); >> + =A0 =A0 =A0fork_load_infrun_state (newfp); >> + =A0 =A0 =A0insert_breakpoints (); >> + =A0 =A0} >> + >> + =A0/* Get the waitpid_fn. =A0*/ >> + =A0if (lookup_minimal_symbol ("waitpid", NULL, NULL) !=3D NULL) >> + =A0 =A0waitpid_fn =3D find_function_in_inferior ("waitpid", &waitpid_o= bjf); >> + =A0if (!waitpid_fn && lookup_minimal_symbol ("_waitpid", NULL, NULL) != =3D NULL) >> + =A0 =A0waitpid_fn =3D find_function_in_inferior ("_waitpid", &waitpid_= objf); >> + =A0if (!waitpid_fn) >> + =A0 =A0return -1; > > This early return doesn't switch back to oldfp... =A0I'd prefer to > have this switching in and out done by the caller of this function. > Can you do that change please? OK. I will fix it. > >> + >> + =A0/* Get the argv. =A0*/ >> + =A0argv[0] =3D value_from_longest (builtin_type (gdbarch)->builtin_int= , pid); >> + =A0argv[1] =3D value_from_longest (builtin_type (gdbarch)->builtin_dat= a_ptr, 0); > > value_from_pointer > >> + =A0argv[2] =3D value_from_longest (builtin_type (gdbarch)->builtin_int= , 0); >> + =A0argv[3] =3D 0; >> + >> + =A0if (call_function_by_hand (waitpid_fn, 3, argv) =3D=3D 0) >> + =A0 =A0ret =3D -1; >> + >> + =A0if (oldfp) >> + =A0 =A0{ >> + =A0 =A0 =A0/* Switch back to inferior_ptid. */ >> + =A0 =A0 =A0remove_breakpoints (); >> + =A0 =A0 =A0fork_load_infrun_state (oldfp); >> + =A0 =A0 =A0insert_breakpoints (); >> + =A0 =A0} >> + >> + =A0return ret; >> +} >> + >> =A0/* Fork list <-> user interface. =A0*/ >> >> =A0static void >> =A0delete_checkpoint_command (char *args, int from_tty) >> =A0{ >> - =A0ptid_t ptid; >> + =A0ptid_t ptid, pptid; >> + =A0int ppid; >> >> =A0 =A0if (!args || !*args) >> =A0 =A0 =A0error (_("Requires argument (checkpoint id to delete)")); >> @@ -428,9 +522,18 @@ delete_checkpoint_command (char *args, i >> =A0 =A0 =A0error (_("\ >> =A0Please switch to another checkpoint before deleting the current one")= ); >> >> + =A0ppid =3D inferior_call_getppid (ptid); >> + =A0pptid =3D ptid_build (ppid, ppid, 0); >> + >> =A0 =A0if (ptrace (PTRACE_KILL, PIDGET (ptid), 0, 0)) >> =A0 =A0 =A0error (_("Unable to kill pid %s"), target_pid_to_str (ptid)); >> >> + =A0if (ppid > 1 && ppid !=3D getpid () && is_stopped (pptid)) >> + =A0 =A0{ >> + =A0 =A0 =A0if (inferior_call_waitpid (pptid, PIDGET (ptid))) >> + =A0 =A0 =A0 =A0warning (_("Unable to wait pid %s"), target_pid_to_str = (ptid)); >> + =A0 =A0} >> + >> =A0 =A0if (from_tty) >> =A0 =A0 =A0printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid)); >> > I make a new patch according to your mail. Please help me review it. Thanks, Hui 2010-05-13 Hui Zhu Michael Snyder * linux-fork.c (gdbthread.h): New include. (fork_info): Add parent_ptid. (inferior_call_waitpid): New function. (delete_checkpoint_command): Call inferior_call_getppid and inferior_call_waitpid. (checkpoint_command): Set parent_ptid. --- linux-fork.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++= +++++ 1 file changed, 67 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,66 @@ linux_fork_detach (char *args, int from_ delete_fork (inferior_ptid); } +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; + 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 (); + } + + /* 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; + + if (call_function_by_hand (waitpid_fn, 3, argv) =3D=3D 0) + goto out; + + ret =3D 0; + +out: + if (oldfp) + { + /* Switch back to inferior_ptid. */ + remove_breakpoints (); + fork_load_infrun_state (oldfp); + insert_breakpoints (); + } + 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,6 +487,16 @@ 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 ((!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)); + } + if (from_tty) printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid)); @@ -596,6 +662,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