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

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

[Hui Zhu]

Hi,

I found that when we delete the checkpoint process, it keep defunct.
This is because the parent process is still running and didn't wait
it.
So I add a wait_ptid function after ptrace kill.

Please help me review it.

Thanks,
Hui

2010-05-09  Hui Zhu  <teawater@gmail.com>

	* linux-fork.c (wait_ptid): New function.
	(delete_checkpoint_command): Call wait_ptid.


---
 linux-fork.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

--- a/linux-fork.c
+++ b/linux-fork.c
@@ -410,6 +410,35 @@ linux_fork_detach (char *args, int from_
     delete_fork (inferior_ptid);
 }

+static int
+wait_ptid (ptid_t ptid)
+{
+  struct objfile *waitpid_objf;
+  struct value *waitpid_fn = NULL;
+  struct value *argv[4];
+  struct gdbarch *gdbarch = get_current_arch ();
+
+  /* Get the waitpid_fn.  */
+  if (lookup_minimal_symbol ("waitpid", NULL, NULL) != NULL)
+    waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
+  if (!waitpid_fn)
+    if (lookup_minimal_symbol ("_waitpid", NULL, NULL) != NULL)
+      waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
+  if (!waitpid_fn)
+    return -1;
+
+  /* Get the argv.  */
+  argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int,
PIDGET (ptid));
+  argv[1] = value_from_longest (builtin_type (gdbarch)->builtin_int, 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)
+    return -1;
+
+  return 0;
+}
+
 /* Fork list <-> user interface.  */

 static void
@@ -431,6 +460,9 @@ 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));

+  if (wait_ptid (ptid))
+    error (_("Unable to wait pid %s"), target_pid_to_str (ptid));
+
   if (from_tty)
     printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));

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

[Michael Snyder]

Hui Zhu wrote:
> Hi,
> 
> I found that when we delete the checkpoint process, it keep defunct.
> This is because the parent process is still running and didn't wait
> it.
> So I add a wait_ptid function after ptrace kill.
> 
> Please help me review it.

I think it should be called something more informative than "wait_ptid".
The name should reflect the fact that it is actually the inferior that
is calling wait, not gdb.

Something like "inferior_call_wait_ptid", perhaps.

Other than that it seems like a good idea...


> 2010-05-09  Hui Zhu  <teawater@gmail.com>
> 
> 	* linux-fork.c (wait_ptid): New function.
> 	(delete_checkpoint_command): Call wait_ptid.
> 
> 
> ---
>  linux-fork.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> --- a/linux-fork.c
> +++ b/linux-fork.c
> @@ -410,6 +410,35 @@ linux_fork_detach (char *args, int from_
>      delete_fork (inferior_ptid);
>  }
> 
> +static int
> +wait_ptid (ptid_t ptid)
> +{
> +  struct objfile *waitpid_objf;
> +  struct value *waitpid_fn = NULL;
> +  struct value *argv[4];
> +  struct gdbarch *gdbarch = get_current_arch ();
> +
> +  /* Get the waitpid_fn.  */
> +  if (lookup_minimal_symbol ("waitpid", NULL, NULL) != NULL)
> +    waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
> +  if (!waitpid_fn)
> +    if (lookup_minimal_symbol ("_waitpid", NULL, NULL) != NULL)
> +      waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
> +  if (!waitpid_fn)
> +    return -1;
> +
> +  /* Get the argv.  */
> +  argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int,
> PIDGET (ptid));
> +  argv[1] = value_from_longest (builtin_type (gdbarch)->builtin_int, 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)
> +    return -1;
> +
> +  return 0;
> +}
> +
>  /* Fork list <-> user interface.  */
> 
>  static void
> @@ -431,6 +460,9 @@ 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));
> 
> +  if (wait_ptid (ptid))
> +    error (_("Unable to wait pid %s"), target_pid_to_str (ptid));
> +
>    if (from_tty)
>      printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));

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

[Pedro Alves]

On Sunday 09 May 2010 07:23:15, Hui Zhu wrote:

> I found that when we delete the checkpoint process, it keep defunct.
> This is because the parent process is still running and didn't wait
> it.
> So I add a wait_ptid function after ptrace kill.

You're assuming inferior_ptid is the parent process
of the checkpoint fork, but I don't believe that is always
true.  E.g., if you do

(gdb) checkpoint
(gdb) checkpoint
(gdb) checkpoint
(gdb) info checkpoints
  3 process 15353 at 0x457d43, file gdb.c, line 28
  2 process 15352 at 0x457d43, file gdb.c, line 28
  1 process 15351 at 0x457d43, file gdb.c, line 28
* 0 Thread 0x7ffff7fcc6f0 (LWP 15348) (main process) at 0x457d43, file gdb.c, line 28
(gdb) restart 1
...
(gdb) delete checkpoint 2

At this point, inferior_ptid will be process 15351, but that
is not the parent of 15352, the process you're killing.  15348
is.

> +static int
> +wait_ptid (ptid_t ptid)
> +{

I'd rename this to call_waitpid, similarly to the call_lseek function
already present in the file.  You may want to reimplement your
function similarly to call_lseek is implemented too.  Your call.

> +  struct objfile *waitpid_objf;
> +  struct value *waitpid_fn = NULL;
> +  struct value *argv[4];
> +  struct gdbarch *gdbarch = get_current_arch ();
> +
> +  /* Get the waitpid_fn.  */
> +  if (lookup_minimal_symbol ("waitpid", NULL, NULL) != NULL)
> +    waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
> +  if (!waitpid_fn)
> +    if (lookup_minimal_symbol ("_waitpid", NULL, NULL) != NULL)

"_waitpid" here,

> +      waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);

but "waitpid" here?

You could also put those two 'if's on a single line, like:
 
 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);

> +  /* Get the argv.  */
> +  argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int,
> PIDGET (ptid));
> +  argv[1] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
> +  argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
> +  argv[3] = 0;

The second argument of waitpid is a pointer, not an integer.  From
`man waitpid':

 pid_t waitpid(pid_t pid, int *status, int options);

> +  if (call_function_by_hand (waitpid_fn, 3, argv) == 0)
> +    return -1;

This doesn't work in non-stop/async modes if the parent
is presently running.  Maybe just take the easy route for now, and
add an is_stopped check, bailing out if not stopped?

> +  if (wait_ptid (ptid))
> +    error (_("Unable to wait pid %s"), target_pid_to_str (ptid));

-- 
Pedro Alves

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

[Michael Snyder]

Pedro Alves wrote:
> On Sunday 09 May 2010 07:23:15, Hui Zhu wrote:
> 
>> I found that when we delete the checkpoint process, it keep defunct.
>> This is because the parent process is still running and didn't wait
>> it.
>> So I add a wait_ptid function after ptrace kill.
> 
> You're assuming inferior_ptid is the parent process
> of the checkpoint fork, but I don't believe that is always
> true.  

Correct.  Maybe we should add a "parent ID" field to the
internal checkpoint table?

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

Michael Snyder wrote:
> Pedro Alves wrote:
>> On Sunday 09 May 2010 07:23:15, Hui Zhu wrote:
>>
>>> I found that when we delete the checkpoint process, it keep defunct.
>>> This is because the parent process is still running and didn't wait
>>> it.
>>> So I add a wait_ptid function after ptrace kill.
>> You're assuming inferior_ptid is the parent process
>> of the checkpoint fork, but I don't believe that is always
>> true.  
> 
> Correct.  Maybe we should add a "parent ID" field to the
> internal checkpoint table?


Hui,

Here's a small change that saves the parent PTID.

Can you combine this with your change, to make sure that
waitpid is called by the correct process?

Good luck,
Michael




[-- Attachment #2: fork.txt --]
[-- Type: text/plain, Size: 756 bytes --]

Index: linux-fork.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-fork.c,v
retrieving revision 1.32
diff -u -p -r1.32 linux-fork.c
--- linux-fork.c	5 May 2010 20:37:23 -0000	1.32
+++ linux-fork.c	11 May 2010 22:26:08 -0000
@@ -47,6 +47,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.  */
@@ -596,6 +597,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

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

[Pedro Alves]

On Tuesday 11 May 2010 22:50:26, Michael Snyder wrote:
> Pedro Alves wrote:
> > On Sunday 09 May 2010 07:23:15, Hui Zhu wrote:
> > 
> >> I found that when we delete the checkpoint process, it keep defunct.
> >> This is because the parent process is still running and didn't wait
> >> it.
> >> So I add a wait_ptid function after ptrace kill.
> > 
> > You're assuming inferior_ptid is the parent process
> > of the checkpoint fork, but I don't believe that is always
> > true.  
> 
> Correct.  Maybe we should add a "parent ID" field to the
> internal checkpoint table?

Sure.

-- 
Pedro Alves

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

[Michael Snyder]

Follow-up to my last reply...


Hui Zhu wrote:
> Hi,
> 
> I found that when we delete the checkpoint process, it keep defunct.
> This is because the parent process is still running and didn't wait
> it.
> So I add a wait_ptid function after ptrace kill.
> 
> Please help me review it.
> 
> Thanks,
> Hui
> 
> 2010-05-09  Hui Zhu  <teawater@gmail.com>
> 
> 	* linux-fork.c (wait_ptid): New function.
> 	(delete_checkpoint_command): Call wait_ptid.
> 
> 
> ---
>  linux-fork.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> --- a/linux-fork.c
> +++ b/linux-fork.c
> @@ -410,6 +410,35 @@ linux_fork_detach (char *args, int from_
>      delete_fork (inferior_ptid);
>  }
> 
> +static int
> +wait_ptid (ptid_t ptid)
> +{
> +  struct objfile *waitpid_objf;
> +  struct value *waitpid_fn = NULL;
> +  struct value *argv[4];
> +  struct gdbarch *gdbarch = get_current_arch ();
> +
> +  /* Get the waitpid_fn.  */
> +  if (lookup_minimal_symbol ("waitpid", NULL, NULL) != NULL)
> +    waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
> +  if (!waitpid_fn)
> +    if (lookup_minimal_symbol ("_waitpid", NULL, NULL) != NULL)
> +      waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
> +  if (!waitpid_fn)
> +    return -1;
> +
> +  /* Get the argv.  */
> +  argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int,
> PIDGET (ptid));
> +  argv[1] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
> +  argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
> +  argv[3] = 0;

      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;
> +}
> +
>  /* Fork list <-> user interface.  */
> 
>  static void
> @@ -431,6 +460,9 @@ 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));
> 
> +  if (wait_ptid (ptid))
> +    error (_("Unable to wait pid %s"), target_pid_to_str (ptid));
> +
>    if (from_tty)
>      printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));

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

[Pedro Alves]

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.

-- 
Pedro Alves

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

[Michael Snyder]

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.   ;-)

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

[Hui Zhu]

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.
 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;
+  ppid = value_as_long (ret);
+
+  /* Switch back to inferior_ptid. */
+  remove_breakpoints ();
+  fork_load_infrun_state (oldfp);
+  insert_breakpoints ();
+
+  return ppid;
+}
+
+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;
+
+  /* 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);
+  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));

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

[Pedro Alves]

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?

>  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 ?

> +  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

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

[Pedro Alves]

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?

>  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 ?

> +  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

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

[Hui Zhu]

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

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

[Pedro Alves]

On Thursday 13 May 2010 08:58:49, Hui Zhu wrote:
> >> 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.

You didn't answer the question.  Please point me at where is
this "gdb auto waiting".

I actually don't understand that rationale.  The `init' process
will "auto wait"  the checkpoint forks as well, so why bother in
the first place then?

> +  if ((!find_thread_ptid (fi->parent_ptid) && find_fork_ptid (fi->parent_ptid))
> +      || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid)))

This requires an explaning comment in the code.

> >> +
> >> +  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.

Oh, right.  :-)

> 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."));

Well, the only return from call_function_by_hand does:

    gdb_assert (retval);
    return retval;

All other cases are handled by throwing an error.   So
"definitely can't happen"; please remove the new dead code
you're adding.  

BTW, you patch is not error/exception safe.  You didn't consider
the case of an `error' being thrown between switching the
fork and back.

Otherwise, the patch looks good.

-- 
Pedro Alves

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

[Hui Zhu]

On Thu, May 13, 2010 at 18:30, Pedro Alves <pedro@codesourcery.com> wrote:
>
> On Thursday 13 May 2010 08:58:49, Hui Zhu wrote:
> > >> 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.
>
> You didn't answer the question.  Please point me at where is
> this "gdb auto waiting".
>
> I actually don't understand that rationale.  The `init' process
> will "auto wait"  the 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 sure if

> > +  if ((!find_thread_ptid (fi->parent_ptid) && find_fork_ptid (fi->parent_ptid))
> > +      || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid)))
>
> This requires an explaning comment in the code.

OK.  I will add them.

>
> > >> +
> > >> +  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.
>
> Oh, right.  :-)
>
> > 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."));
>
> Well, the only return from call_function_by_hand does:
>
>    gdb_assert (retval);
>    return retval;
>
> All other cases are handled by throwing an error.   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.  You 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  <teawater@gmail.com>
            Michael Snyder  <msnyder@vmware.com>

	* 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 <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,74 @@ linux_fork_detach (char *args, int from_
     delete_fork (inferior_ptid);
 }

+static void
+inferior_call_waitpid_cleanup (void *fp)
+{
+  struct fork_info *oldfp = 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 = NULL;
+  struct value *argv[4];
+  struct gdbarch *gdbarch = get_current_arch ();
+  struct fork_info *oldfp = NULL, *newfp = NULL;
+  struct cleanup *old_cleanup = 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 ();
+
+      old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
+    }
+
+  /* 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;
+
+  call_function_by_hand (waitpid_fn, 3, argv);
+
+  ret = 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 = 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_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));
+    }
 }

 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 = last_target_ptid;
 }

 static void

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

[Pedro Alves]

On Friday 14 May 2010 07:39:17, Hui Zhu wrote:

> > BTW, you patch is not error/exception safe.  You 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);

You're now accessing the FI pointer after having just deleted it.  The
delete_fork(ptid) line deletes FI.

> +  fi = 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_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));
> +    }
>  }


> +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;
> +  struct cleanup *old_cleanup = NULL;

    if (...)
      {
> +      old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
> +    }
> +
> +  if (old_cleanup)
> +    do_cleanups (old_cleanup);

Please do not use this check-for-NULL-cleanup pattern anywhere.
You should install a null_cleanup instead, and unconditionally run
the cleanups.  You should _never_ rely on old_cleanup being
NULL to mean you haven't installed a cleanup above.  The reason
is that the

   old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);

line may well return NULL, because NULL was the head of
the cleanup chain when the first cleanup is registered, and that's
what make_cleanup returns, the previous head of the chain.

To convince yourself, put a breakpoint on make_my_cleanup
and run gdb.  Step out and check what is the old_chain value
that ends up stored in the caller.

> +  return ret;
> +}

I see you dropped extracting the return value of the waitpid
call, in addition to the dead code.  Was that on purpose?  It's
fine either way, just pointing it out in case it slipped.

-- 
Pedro Alves

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

[Hui Zhu]

Thanks Pedro.

On Fri, May 14, 2010 at 20:20, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 14 May 2010 07:39:17, Hui Zhu wrote:
>
>> > BTW, you patch is not error/exception safe.  You 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);
>
> You're now accessing the FI pointer after having just deleted it.  The
> delete_fork(ptid) line deletes FI.

Faint.  I will fix it.

>
>> +  fi = 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_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));
>> +    }
>>  }
>
>
>> +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;
>> +  struct cleanup *old_cleanup = NULL;
>
>    if (...)
>      {
>> +      old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
>> +    }
>> +
>> +  if (old_cleanup)
>> +    do_cleanups (old_cleanup);
>
> Please do not use this check-for-NULL-cleanup pattern anywhere.
> You should install a null_cleanup instead, and unconditionally run
> the cleanups.  You should _never_ rely on old_cleanup being
> NULL to mean you haven't installed a cleanup above.  The reason
> is that the
>
>   old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
>
> line may well return NULL, because NULL was the head of
> the cleanup chain when the first cleanup is registered, and that's
> what make_cleanup returns, the previous head of the chain.
>
> To convince yourself, put a breakpoint on make_my_cleanup
> and run gdb.  Step out and check what is the old_chain value
> that ends up stored in the caller.


Thanks for told it so clear.   I will fix it.

>
>> +  return ret;
>> +}
>
> I see you dropped extracting the return value of the waitpid
> call, in addition to the dead code.  Was that on purpose?  It's
> fine either way, just pointing it out in case it slipped.

In before I just want check the return value of call_function_by_hand
but didn't check the waitpid return value.  So I just removed the
check code.

But add check for waitpid is not a big work, so I will add code for it.

>
> --
> Pedro Alves
>
I make a patch for cvs-head.  Please help me review it.

Best regards,
Hui

2010-05-14  Hui Zhu  <teawater@gmail.com>

	* linux-fork.c (inferior_call_waitpid_cleanup): Add check
	for oldfp.
	(inferior_call_waitpid): Move make_cleanup out of check.
	Check the return of waitpid.
	(delete_checkpoint_command): Add pptid to save fi->parent_ptid.


---
 linux-fork.c |   35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

--- a/linux-fork.c
+++ b/linux-fork.c
@@ -417,10 +417,13 @@ inferior_call_waitpid_cleanup (void *fp)
 {
   struct fork_info *oldfp = fp;

-  /* Switch back to inferior_ptid. */
-  remove_breakpoints ();
-  fork_load_infrun_state (oldfp);
-  insert_breakpoints ();
+  if (oldfp)
+    {
+      /* Switch back to inferior_ptid. */
+      remove_breakpoints ();
+      fork_load_infrun_state (oldfp);
+      insert_breakpoints ();
+    }
 }

 static int
@@ -428,10 +431,10 @@ inferior_call_waitpid (ptid_t pptid, int
 {
   struct objfile *waitpid_objf;
   struct value *waitpid_fn = NULL;
-  struct value *argv[4];
+  struct value *argv[4], *retv;
   struct gdbarch *gdbarch = get_current_arch ();
   struct fork_info *oldfp = NULL, *newfp = NULL;
-  struct cleanup *old_cleanup = NULL;
+  struct cleanup *old_cleanup;
   int ret = -1;

   if (!ptid_equal (pptid, inferior_ptid))
@@ -445,10 +448,10 @@ inferior_call_waitpid (ptid_t pptid, int
       remove_breakpoints ();
       fork_load_infrun_state (newfp);
       insert_breakpoints ();
-
-      old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
     }

+  old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
+
   /* Get the waitpid_fn.  */
   if (lookup_minimal_symbol ("waitpid", NULL, NULL) != NULL)
     waitpid_fn = find_function_in_inferior ("waitpid", &waitpid_objf);
@@ -463,13 +466,14 @@ inferior_call_waitpid (ptid_t pptid, int
   argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
   argv[3] = 0;

-  call_function_by_hand (waitpid_fn, 3, argv);
+  retv = call_function_by_hand (waitpid_fn, 3, argv);
+  if (value_as_long (retv) < 0)
+    goto out;

   ret = 0;

 out:
-  if (old_cleanup)
-    do_cleanups (old_cleanup);
+  do_cleanups (old_cleanup);
   return ret;
 }

@@ -478,7 +482,7 @@ out:
 static void
 delete_checkpoint_command (char *args, int from_tty)
 {
-  ptid_t ptid;
+  ptid_t ptid, pptid;
   struct fork_info *fi;

   if (!args || !*args)
@@ -497,6 +501,7 @@ Please switch to another checkpoint befo

   fi = find_fork_ptid (ptid);
   gdb_assert (fi);
+  pptid = fi->parent_ptid;

   if (from_tty)
     printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));
@@ -507,10 +512,10 @@ Please switch to another checkpoint befo
      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_ptid))
-      || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid)))
+  if ((!find_thread_ptid (pptid) && find_fork_ptid (pptid))
+      || (find_thread_ptid (pptid) && is_stopped (pptid)))
     {
-      if (inferior_call_waitpid (fi->parent_ptid, PIDGET (ptid)))
+      if (inferior_call_waitpid (pptid, PIDGET (ptid)))
         warning (_("Unable to wait pid %s"), target_pid_to_str (ptid));
     }
 }

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

[Pedro Alves]

Thanks Hui,

On Friday 14 May 2010 15:43:23, Hui Zhu wrote:
> +  pptid = fi->parent_ptid;
> 
>    if (from_tty)
>      printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));
> @@ -507,10 +512,10 @@ Please switch to another checkpoint befo
>       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_ptid))
> -      || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid)))
> +  if ((!find_thread_ptid (pptid) && find_fork_ptid (pptid))

Sorry, but is still not correct.  When you end up with only one fork
in the checkpoint list, delete_fork will delete it as well.  For
example:

(gdb) checkpoint

at this point, you have two checkpoints, checkpoint 1 (the forked child),
and checkpoint 0, the inferior you were already debugging.  When you
delete the new checkpoint 1, with:

(gdb) delete checkpoint 1

delete_fork will also delete checkpoint 0, the parent of checkpoint 1.
(see the "Special case:" comment in delete_fork), so your
find_fork_ptid(pptid) here will be too late.

-- 
Pedro Alves

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

[Hui Zhu]

On Fri, May 14, 2010 at 23:08, Pedro Alves <pedro@codesourcery.com> wrote:
> Thanks Hui,
>
> On Friday 14 May 2010 15:43:23, Hui Zhu wrote:
>> +  pptid = fi->parent_ptid;
>>
>>    if (from_tty)
>>      printf_filtered (_("Killed %s\n"), target_pid_to_str (ptid));
>> @@ -507,10 +512,10 @@ Please switch to another checkpoint befo
>>       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_ptid))
>> -      || (find_thread_ptid (fi->parent_ptid) && is_stopped (fi->parent_ptid)))
>> +  if ((!find_thread_ptid (pptid) && find_fork_ptid (pptid))
>
> Sorry, but is still not correct.  When you end up with only one fork
> in the checkpoint list, delete_fork will delete it as well.  For
> example:
>
> (gdb) checkpoint
>
> at this point, you have two checkpoints, checkpoint 1 (the forked child),
> and checkpoint 0, the inferior you were already debugging.  When you
> delete the new checkpoint 1, with:
>
> (gdb) delete checkpoint 1
>
> delete_fork will also delete checkpoint 0, the parent of checkpoint 1.
> (see the "Special case:" comment in delete_fork), so your
> find_fork_ptid(pptid) here will be too late.
>

At this time, "find_thread_ptid (pptid) && is_stopped (pptid)" this
line will handle it.

Thanks,
Hui

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

[Pedro Alves]

On Monday 17 May 2010 07:37:01, Hui Zhu wrote:

> At this time, "find_thread_ptid (pptid) && is_stopped (pptid)" this
> line will handle it.

I see.  I'd prefer to not have these subtle assumptions, but
the patch is okay as is then.  Thanks.

-- 
Pedro Alves

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

[Hui Zhu]

On Mon, May 17, 2010 at 18:33, Pedro Alves <pedro@codesourcery.com> wrote:
> On Monday 17 May 2010 07:37:01, Hui Zhu wrote:
>
>> At this time, "find_thread_ptid (pptid) && is_stopped (pptid)" this
>> line will handle it.
>
> I see.  I'd prefer to not have these subtle assumptions, but
> the patch is okay as is then.  Thanks.
>
> --
> Pedro Alves
>

Thanks for your help, Pedro.  Checked in.

Best,
Hui

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

[Michael Snyder]

Did this get checked in?

I didn't think the discussion was finished.

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

[Hui Zhu]

On Sat, May 15, 2010 at 03:08, Michael Snyder <msnyder@vmware.com> wrote:
> Did this get checked in?
>
> I didn't think the discussion was finished.
>
>

Sorry the prev patch was checked in.  But it has some bug.  So I post
a new patch to fix them in
http://sourceware.org/ml/gdb-patches/2010-05/msg00299.html

Thanks,
Hui