From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15362 invoked by alias); 12 May 2010 18:39:45 -0000 Received: (qmail 15349 invoked by uid 22791); 12 May 2010 18:39:44 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,TW_BJ,TW_DF,TW_TP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 12 May 2010 18:39:38 +0000 Received: (qmail 15264 invoked from network); 12 May 2010 18:39:36 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 12 May 2010 18:39:36 -0000 From: Pedro Alves To: Hui Zhu Subject: Re: [RFA] Checkpoint: wait the defunct process when delete it Date: Wed, 12 May 2010 18:39:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: Michael Snyder , "gdb-patches@sourceware.org" References: <4BE9F5EF.2050405@vmware.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201005121937.44287.pedro@codesourcery.com> 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/msg00261.txt.bz2 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: > >> > >> > >>> old_cleanup = save_inferior_ptid (); > >>> inferior_ptid = fd->parent_ptid; > >>> > >>> Something like this? Then the original inferior_ptid will be > >>> restored when you do > >>> > >>>> + if (call_function_by_hand (waitpid_fn, 3, argv) == 0) > >>>> + return -1; > >>> > >>> do_cleanups(); > >>> > >>>> + return 0; > >>>> +} > >> > >> That won't work. You will hit an assertion somewhere: either because > >> inferior_ptid is not found in the linux-nat.c lwp list, or because > >> inferior_ptid is not found in gdb's thread list. I believe you'll > >> need to do a full linux_nat_switch_fork and back. > >> > > > > There you go. ;-) > > > > Hello guys, > > I am sorry that I didn't complete the new patch yesterday. Thanks for > your help. > > According to your comments. I did following change: > > 1. Before kill the ptid, GDB switch to ptid and call > "inferior_call_getppid" to get the ppid of this inferior. And save it > to ppid. > > 2. before call "inferior_call_waitpid" to waitpid the ptid. > Check if ppid is a simple thread. ppid > 1 > Check if ppid is the GDB. If ppid is GDB, it will auto wait the ptid. What do you mean, it will auto wait the ptid? AFAICS, (gdb) checkpoint (gdb) checkpoint (gdb) checkpoint (gdb) checkpoint (gdb) checkpoint (gdb) restart 5 (gdb) delete checkpoint 0 will still leave checkpoint 0 zombie? > ppid != getpid () > Check if ppid is stop. is_stopped (pptid) > > 3. In function inferior_call_waitpid, before call waitpid, swith to ppid. > > 4. If inferior_call_waitpid, just give a warning to user. > > Please help me review it. > > Best regards, > Hui > > 2010-05-12 Hui Zhu > > * linux-fork.c (gdbthread.h): New include. > (inferior_call_getppid, inferior_call_waitpid): New function. > (delete_checkpoint_command): Call inferior_call_getppid > and inferior_call_waitpid. > > > --- > linux-fork.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 104 insertions(+), 1 deletion(-) > > --- 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" > @@ -410,12 +411,105 @@ linux_fork_detach (char *args, int from_ > delete_fork (inferior_ptid); > } > > +static int > +inferior_call_getppid (ptid_t ptid) > +{ > + struct objfile *getppid_objf; > + struct value *getppid_fn = NULL, *ret; > + struct value *argv[4]; > + struct gdbarch *gdbarch = get_current_arch (); > + struct fork_info *oldfp, *newfp; > + int ppid = 1; > + > + /* Switch to ptid. */ > + oldfp = find_fork_ptid (inferior_ptid); > + gdb_assert (oldfp != NULL); > + newfp = find_fork_ptid (ptid); > + gdb_assert (oldfp != NULL); > + fork_save_infrun_state (oldfp, 1); > + remove_breakpoints (); > + fork_load_infrun_state (newfp); > + insert_breakpoints (); > + > + /* Get the getppid_fn. */ > + if (lookup_minimal_symbol ("getppid", NULL, NULL) != NULL) > + getppid_fn = find_function_in_inferior ("getppid", &getppid_objf); > + if (!getppid_fn && lookup_minimal_symbol ("_getppid", NULL, NULL) != NULL) > + getppid_fn = find_function_in_inferior ("_getppid", &getppid_objf); > + if (!getppid_fn) > + return 1; > + > + ret = call_function_by_hand (getppid_fn, 0, NULL); > + if (ret == 0) > + return ppid; ??? can getppid really return 0 ? > + ppid = value_as_long (ret); > + > + /* Switch back to inferior_ptid. */ > + remove_breakpoints (); > + fork_load_infrun_state (oldfp); > + insert_breakpoints (); > + > + return ppid; > +} I don't think calling getppid is a great idea. You may not even be debugging the parent process of the process you are about to kill! E.g., say you attach to a process, create a checkpoint, restart the checkpoint, and delete checkpoint 0. getppid will return a pid of a process you are _not_ debugging. You should at least check for that. But, why didn't storing the parent pid at fork time (like was suggested before), work? > + > +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; > + int ret = 0; > + > + if (!ptid_equal (pptid, inferior_ptid)) > + { > + /* Switch to pptid. */ > + oldfp = find_fork_ptid (inferior_ptid); > + gdb_assert (oldfp != NULL); > + newfp = find_fork_ptid (pptid); > + gdb_assert (oldfp != 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) != NULL) > + waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf); > + if (!waitpid_fn && lookup_minimal_symbol ("_waitpid", NULL, NULL) != NULL) > + waitpid_fn = find_function_in_inferior ("_waitpid", &waitpid_objf); > + if (!waitpid_fn) > + return -1; This early return doesn't switch back to oldfp... I'd prefer to have this switching in and out done by the caller of this function. Can you do that change please? > + > + /* Get the argv. */ > + argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int, pid); > + argv[1] = value_from_longest (builtin_type (gdbarch)->builtin_data_ptr, 0); value_from_pointer > + argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0); > + argv[3] = 0; > + > + if (call_function_by_hand (waitpid_fn, 3, argv) == 0) > + ret = -1; > + > + 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; > + ptid_t ptid, pptid; > + int ppid; > > if (!args || !*args) > error (_("Requires argument (checkpoint id to delete)")); > @@ -428,9 +522,18 @@ delete_checkpoint_command (char *args, i > error (_("\ > Please switch to another checkpoint before deleting the current one")); > > + ppid = inferior_call_getppid (ptid); > + pptid = ptid_build (ppid, ppid, 0); > + > if (ptrace (PTRACE_KILL, PIDGET (ptid), 0, 0)) > error (_("Unable to kill pid %s"), target_pid_to_str (ptid)); > > + if (ppid > 1 && ppid != getpid () && is_stopped (pptid)) > + { > + if (inferior_call_waitpid (pptid, 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)); > -- Pedro Alves From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18015 invoked by alias); 12 May 2010 18:42:55 -0000 Received: (qmail 18002 invoked by uid 22791); 12 May 2010 18:42:54 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,TW_BJ,TW_DF,TW_TP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 12 May 2010 18:42:50 +0000 Received: (qmail 17683 invoked from network); 12 May 2010 18:42:49 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 12 May 2010 18:42:49 -0000 From: Pedro Alves To: Hui Zhu Subject: Re: [RFA] Checkpoint: wait the defunct process when delete it Date: Wed, 12 May 2010 18:57:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: Michael Snyder , "gdb-patches@sourceware.org" References: <4BE9F5EF.2050405@vmware.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-ID: <201005121937.44287.pedro@codesourcery.com> 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/msg00262.txt.bz2 Message-ID: <20100512185700.Z6SNiq8myBNaP4UEDJGmYmoatJOShhSrDMhi_AlW-BA@z> 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: > >> > >> > >>> old_cleanup = save_inferior_ptid (); > >>> inferior_ptid = fd->parent_ptid; > >>> > >>> Something like this? Then the original inferior_ptid will be > >>> restored when you do > >>> > >>>> + if (call_function_by_hand (waitpid_fn, 3, argv) == 0) > >>>> + return -1; > >>> > >>> do_cleanups(); > >>> > >>>> + return 0; > >>>> +} > >> > >> That won't work. You will hit an assertion somewhere: either because > >> inferior_ptid is not found in the linux-nat.c lwp list, or because > >> inferior_ptid is not found in gdb's thread list. I believe you'll > >> need to do a full linux_nat_switch_fork and back. > >> > > > > There you go. ;-) > > > > Hello guys, > > I am sorry that I didn't complete the new patch yesterday. Thanks for > your help. > > According to your comments. I did following change: > > 1. Before kill the ptid, GDB switch to ptid and call > "inferior_call_getppid" to get the ppid of this inferior. And save it > to ppid. > > 2. before call "inferior_call_waitpid" to waitpid the ptid. > Check if ppid is a simple thread. ppid > 1 > Check if ppid is the GDB. If ppid is GDB, it will auto wait the ptid. What do you mean, it will auto wait the ptid? AFAICS, (gdb) checkpoint (gdb) checkpoint (gdb) checkpoint (gdb) checkpoint (gdb) checkpoint (gdb) restart 5 (gdb) delete checkpoint 0 will still leave checkpoint 0 zombie? > ppid != getpid () > Check if ppid is stop. is_stopped (pptid) > > 3. In function inferior_call_waitpid, before call waitpid, swith to ppid. > > 4. If inferior_call_waitpid, just give a warning to user. > > Please help me review it. > > Best regards, > Hui > > 2010-05-12 Hui Zhu > > * linux-fork.c (gdbthread.h): New include. > (inferior_call_getppid, inferior_call_waitpid): New function. > (delete_checkpoint_command): Call inferior_call_getppid > and inferior_call_waitpid. > > > --- > linux-fork.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 104 insertions(+), 1 deletion(-) > > --- 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" > @@ -410,12 +411,105 @@ linux_fork_detach (char *args, int from_ > delete_fork (inferior_ptid); > } > > +static int > +inferior_call_getppid (ptid_t ptid) > +{ > + struct objfile *getppid_objf; > + struct value *getppid_fn = NULL, *ret; > + struct value *argv[4]; > + struct gdbarch *gdbarch = get_current_arch (); > + struct fork_info *oldfp, *newfp; > + int ppid = 1; > + > + /* Switch to ptid. */ > + oldfp = find_fork_ptid (inferior_ptid); > + gdb_assert (oldfp != NULL); > + newfp = find_fork_ptid (ptid); > + gdb_assert (oldfp != NULL); > + fork_save_infrun_state (oldfp, 1); > + remove_breakpoints (); > + fork_load_infrun_state (newfp); > + insert_breakpoints (); > + > + /* Get the getppid_fn. */ > + if (lookup_minimal_symbol ("getppid", NULL, NULL) != NULL) > + getppid_fn = find_function_in_inferior ("getppid", &getppid_objf); > + if (!getppid_fn && lookup_minimal_symbol ("_getppid", NULL, NULL) != NULL) > + getppid_fn = find_function_in_inferior ("_getppid", &getppid_objf); > + if (!getppid_fn) > + return 1; > + > + ret = call_function_by_hand (getppid_fn, 0, NULL); > + if (ret == 0) > + return ppid; ??? can getppid really return 0 ? > + ppid = value_as_long (ret); > + > + /* Switch back to inferior_ptid. */ > + remove_breakpoints (); > + fork_load_infrun_state (oldfp); > + insert_breakpoints (); > + > + return ppid; > +} I don't think calling getppid is a great idea. You may not even be debugging the parent process of the process you are about to kill! E.g., say you attach to a process, create a checkpoint, restart the checkpoint, and delete checkpoint 0. getppid will return a pid of a process you are _not_ debugging. You should at least check for that. But, why didn't storing the parent pid at fork time (like was suggested before), work? > + > +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; > + int ret = 0; > + > + if (!ptid_equal (pptid, inferior_ptid)) > + { > + /* Switch to pptid. */ > + oldfp = find_fork_ptid (inferior_ptid); > + gdb_assert (oldfp != NULL); > + newfp = find_fork_ptid (pptid); > + gdb_assert (oldfp != 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) != NULL) > + waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf); > + if (!waitpid_fn && lookup_minimal_symbol ("_waitpid", NULL, NULL) != NULL) > + waitpid_fn = find_function_in_inferior ("_waitpid", &waitpid_objf); > + if (!waitpid_fn) > + return -1; This early return doesn't switch back to oldfp... I'd prefer to have this switching in and out done by the caller of this function. Can you do that change please? > + > + /* Get the argv. */ > + argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int, pid); > + argv[1] = value_from_longest (builtin_type (gdbarch)->builtin_data_ptr, 0); value_from_pointer > + argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0); > + argv[3] = 0; > + > + if (call_function_by_hand (waitpid_fn, 3, argv) == 0) > + ret = -1; > + > + 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; > + ptid_t ptid, pptid; > + int ppid; > > if (!args || !*args) > error (_("Requires argument (checkpoint id to delete)")); > @@ -428,9 +522,18 @@ delete_checkpoint_command (char *args, i > error (_("\ > Please switch to another checkpoint before deleting the current one")); > > + ppid = inferior_call_getppid (ptid); > + pptid = ptid_build (ppid, ppid, 0); > + > if (ptrace (PTRACE_KILL, PIDGET (ptid), 0, 0)) > error (_("Unable to kill pid %s"), target_pid_to_str (ptid)); > > + if (ppid > 1 && ppid != getpid () && is_stopped (pptid)) > + { > + if (inferior_call_waitpid (pptid, 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)); > -- Pedro Alves