This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Checkpoint: wait the defunct process when delete it


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]