[RFA] Checkpoint: wait the defunct process when delete it
Hui Zhu
teawater@gmail.com
Thu May 13 09:11:00 GMT 2010
On Thu, May 13, 2010 at 02:37, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 12 May 2010 05:19:25, Hui Zhu wrote:
>> On Wed, May 12, 2010 at 08:27, Michael Snyder <msnyder@vmware.com> 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?
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.
>
>> 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 <teawater@gmail.com>
>>
>> * 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 <sys/ptrace.h>
>> #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 ?
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 = 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."));
>
>> + 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?
>
I agree with you. getppid is too cumbersome. I will update the patch.
>> +
>> +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?
OK. I will fix it.
>
>> +
>> + /* 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));
>>
>
I make a new patch according to your mail.
Please help me review it.
Thanks,
Hui
2010-05-13 Hui Zhu <teawater@gmail.com>
Michael Snyder <msnyder@vmware.com>
* 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 <sys/ptrace.h>
#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 = NULL;
+ struct value *argv[4];
+ struct gdbarch *gdbarch = get_current_arch ();
+ struct fork_info *oldfp = NULL, *newfp = NULL;
+ int ret = -1;
+
+ 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)
+ goto out;
+
+ /* Get the argv. */
+ argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int, pid);
+ argv[1] = value_from_pointer (builtin_type (gdbarch)->builtin_data_ptr, 0);
+ argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
+ argv[3] = 0;
+
+ if (call_function_by_hand (waitpid_fn, 3, argv) == 0)
+ goto out;
+
+ ret = 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 = find_fork_ptid (ptid);
+ gdb_assert (fi);
+
+ 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));
+ }
+
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 = last_target_ptid;
}
static void
More information about the Gdb-patches
mailing list