[PATCH,ARM] Fix single step on vfork

[PATCH,ARM] Fix single step on vfork

[Yao Qi]

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

Hi,
Recently, we find some failures in gdb testsute on ARM,

FAIL: gdb.base/watch-vfork.exp: Watchpoint triggers after vfork (hw)
(the program exited)
FAIL: gdb.base/watch-vfork.exp: Watchpoint triggers after vfork (sw)
(the program exited)

Program exits when we stepping over svc instruction in vfork(), which is
caused by child process hits software single step breakpoint inserted
for parent process.

This patch is to fix this problem by 'when inferior's
wait_for_vfork_done is true, clear step to zero and don't use displaced
stepping'.

Tested on GDB CVS on ARM and X86-64.  Fix these two failures above on
ARM, and no regressions.  OK to apply?

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

[-- Attachment #2: single_step_vfork_1.patch --]
[-- Type: text/x-patch, Size: 1858 bytes --]

2010-09-02  Yao Qi  <yao@codesourcery.com>

	* infrunc(resume): When inferior is waiting_for_vfork_done,
	clear step and don't use displaced stepping.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index dd89e78..2f28380 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1550,6 +1550,19 @@ resume (int step, enum target_signal sig)
 
   QUIT;
 
+  /* Don't consider single-stepping when the inferior is 
+     waiting_for_vfork_done, either software or hardware step.  In
+     software step, child process will hit the software single step
+     breakpoint inserted in parent process.  In hardware step, GDB
+     can resumes inferior, and waiting for vfork_done event.  */
+  if (current_inferior()->waiting_for_vfork_done)
+    {
+      if (debug_infrun)
+	fprintf_unfiltered (gdb_stdlog,
+			    "infrun: resume : clear step\n");
+      step = 0;
+    }
+
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog,
                         "infrun: resume (step=%d, signal=%d), "
@@ -1577,11 +1590,16 @@ a command like `return' or `jump' to continue execution."));
      We can't use displaced stepping when we have a signal to deliver;
      the comments for displaced_step_prepare explain why.  The
      comments in the handle_inferior event for dealing with 'random
-     signals' explain what we do instead.  */
+     signals' explain what we do instead.
+
+     We can't use displaced stepping when we are waiting for vfork_done
+     event, displaced stepping breaks the vfork child similarly as single
+     step software breakpoint.  */
   if (use_displaced_stepping (gdbarch)
       && (tp->trap_expected
 	  || (step && gdbarch_software_single_step_p (gdbarch)))
-      && sig == TARGET_SIGNAL_0)
+      && sig == TARGET_SIGNAL_0
+      && !current_inferior()->waiting_for_vfork_done)
     {
       struct displaced_step_inferior_state *displaced;
 

Re: [PATCH,ARM] Fix single step on vfork

[Pedro Alves]

On Wednesday 01 September 2010 17:56:32, Yao Qi wrote:
> 
> +  /* Don't consider single-stepping when the inferior is 
> +     waiting_for_vfork_done, either software or hardware step.  In
> +     software step, child process will hit the software single step
> +     breakpoint inserted in parent process.  In hardware step, GDB
> +     can resumes inferior, and waiting for vfork_done event.  */

This last sentence looks incomplete?  At least, I can't seem to
parse it.

> +  if (current_inferior()->waiting_for_vfork_done)

Space before parens.

> +    {
> +      if (debug_infrun)
> +       fprintf_unfiltered (gdb_stdlog,
> +                           "infrun: resume : clear step\n");
> +      step = 0;
> +    }
> +
>    if (debug_infrun)
>      fprintf_unfiltered (gdb_stdlog,
>                          "infrun: resume (step=%d, signal=%d), "
> @@ -1577,11 +1590,16 @@ a command like `return' or `jump' to continue execution."));
>       We can't use displaced stepping when we have a signal to deliver;
>       the comments for displaced_step_prepare explain why.  The
>       comments in the handle_inferior event for dealing with 'random
> -     signals' explain what we do instead.  */
> +     signals' explain what we do instead.
> +
> +     We can't use displaced stepping when we are waiting for vfork_done
> +     event, displaced stepping breaks the vfork child similarly as single
> +     step software breakpoint.  */
>    if (use_displaced_stepping (gdbarch)
>        && (tp->trap_expected
>           || (step && gdbarch_software_single_step_p (gdbarch)))
> -      && sig == TARGET_SIGNAL_0)
> +      && sig == TARGET_SIGNAL_0
> +      && !current_inferior()->waiting_for_vfork_done)

Space before parens.

Otherwise okay.  Thanks!

-- 
Pedro Alves

Re: [PATCH,ARM] Fix single step on vfork

[Yao Qi]

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

Pedro Alves wrote:
> On Wednesday 01 September 2010 17:56:32, Yao Qi wrote:
>> +  /* Don't consider single-stepping when the inferior is 
>> +     waiting_for_vfork_done, either software or hardware step.  In
>> +     software step, child process will hit the software single step
>> +     breakpoint inserted in parent process.  In hardware step, GDB
>> +     can resumes inferior, and waiting for vfork_done event.  */
> 
> This last sentence looks incomplete?  At least, I can't seem to
> parse it.
> 
I replaced "waiting" by "wait" in the new patch.

>> +  if (current_inferior()->waiting_for_vfork_done)
> 
> Space before parens.
> 

Fixed.

>>    if (use_displaced_stepping (gdbarch)
>>        && (tp->trap_expected
>>           || (step && gdbarch_software_single_step_p (gdbarch)))
>> -      && sig == TARGET_SIGNAL_0)
>> +      && sig == TARGET_SIGNAL_0
>> +      && !current_inferior()->waiting_for_vfork_done)
> 
> Space before parens.
> 
Fixed.


-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

[-- Attachment #2: single_step_vfork_1.patch --]
[-- Type: text/x-patch, Size: 1857 bytes --]

2010-09-02  Yao Qi  <yao@codesourcery.com>

	* infrunc(resume): When inferior is waiting_for_vfork_done,
	clear step and don't use displaced stepping.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index dd89e78..9d40f7c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1550,6 +1550,19 @@ resume (int step, enum target_signal sig)
 
   QUIT;
 
+  /* Don't consider single-stepping when the inferior is 
+     waiting_for_vfork_done, either software or hardware step.  In
+     software step, child process will hit the software single step
+     breakpoint inserted in parent process.  In hardware step, GDB
+     can resumes inferior, and wait for vfork_done event.  */
+  if (current_inferior ()->waiting_for_vfork_done)
+    {
+      if (debug_infrun)
+	fprintf_unfiltered (gdb_stdlog,
+			    "infrun: resume : clear step\n");
+      step = 0;
+    }
+
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog,
                         "infrun: resume (step=%d, signal=%d), "
@@ -1577,11 +1590,16 @@ a command like `return' or `jump' to continue execution."));
      We can't use displaced stepping when we have a signal to deliver;
      the comments for displaced_step_prepare explain why.  The
      comments in the handle_inferior event for dealing with 'random
-     signals' explain what we do instead.  */
+     signals' explain what we do instead.
+
+     We can't use displaced stepping when we are waiting for vfork_done
+     event, displaced stepping breaks the vfork child similarly as single
+     step software breakpoint.  */
   if (use_displaced_stepping (gdbarch)
       && (tp->trap_expected
 	  || (step && gdbarch_software_single_step_p (gdbarch)))
-      && sig == TARGET_SIGNAL_0)
+      && sig == TARGET_SIGNAL_0
+      && !current_inferior ()->waiting_for_vfork_done)
     {
       struct displaced_step_inferior_state *displaced;
 

Re: [PATCH,ARM] Fix single step on vfork

[Pedro Alves]

On Thursday 02 September 2010 02:03:53, Yao Qi wrote:
> Pedro Alves wrote:
> > On Wednesday 01 September 2010 17:56:32, Yao Qi wrote:
> >> +  /* Don't consider single-stepping when the inferior is 
> >> +     waiting_for_vfork_done, either software or hardware step.  In
> >> +     software step, child process will hit the software single step
> >> +     breakpoint inserted in parent process.  In hardware step, GDB
> >> +     can resumes inferior, and waiting for vfork_done event.  */
> > 
> > This last sentence looks incomplete?  At least, I can't seem to
> > parse it.
> > 
> I replaced "waiting" by "wait" in the new patch.

Thanks.  Please go ahead with your patch.  Hope you don't mind I tweak
the comment a bit afterwards.

-- 
Pedro Alves

Re: [PATCH,ARM] Fix single step on vfork

[Yao Qi]

Pedro Alves wrote:
> 
> Thanks.  Please go ahead with your patch.  Hope you don't mind I tweak
> the comment a bit afterwards.
> 
Pedro,
Thanks for your view, and free to tweak the comment.

Committed to GDB mainline,
http://www.cygwin.com/ml/gdb-cvs/2010-09/msg00042.html

Once you changed the comment, I'd like to merge it to GDB 7.2 branch then.

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

Re: [PATCH,ARM] Fix single step on vfork

[Pedro Alves]

On Monday 06 September 2010 14:29:29, Yao Qi wrote:
> Pedro Alves wrote:
> > 
> > Thanks.  Please go ahead with your patch.  Hope you don't mind I tweak
> > the comment a bit afterwards.
> > 
> Pedro,
> Thanks for your view, and free to tweak the comment.
> 
> Committed to GDB mainline,
> http://www.cygwin.com/ml/gdb-cvs/2010-09/msg00042.html
> 
> Once you changed the comment, I'd like to merge it to GDB 7.2 branch then.

Okay.  I've now applied this to mainline.

-- 
Pedro Alves

2010-09-06  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (resume): Extend comment on ignoring single-step
	requests on vfork parents waiting for a vfork-done.

---
 gdb/infrun.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2010-09-06 14:33:12.000000000 +0100
+++ src/gdb/infrun.c	2010-09-06 15:13:27.000000000 +0100
@@ -1560,13 +1560,19 @@ resume (int step, enum target_signal sig
 
   QUIT;
 
-  /* Don't consider single-stepping when the inferior is 
-     waiting_for_vfork_done, either software or hardware step.  In
-     software step, child process will hit the software single step
-     breakpoint inserted in parent process.  In hardware step, GDB
-     can resumes inferior, and wait for vfork_done event.  */
   if (current_inferior ()->waiting_for_vfork_done)
     {
+      /* Don't try to single-step a vfork parent that is waiting for
+	 the child to get out of the shared memory region (by exec'ing
+	 or exiting).  This is particularly important on software
+	 single-step archs, as the child process would trip on the
+	 software single step breakpoint inserted for the parent
+	 process.  Since the parent will not actually execute any
+	 instruction until the child is out of the shared region (such
+	 are vfork's semantics), it is safe to simply continue it.
+	 Eventually, we'll see a TARGET_WAITKIND_VFORK_DONE event for
+	 the parent, and tell it to `keep_going', which automatically
+	 re-sets it stepping.  */
       if (debug_infrun)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: resume : clear step\n");

Re: [PATCH,ARM] Fix single step on vfork

[Yao Qi]

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

Pedro Alves wrote:
> On Monday 06 September 2010 14:29:29, Yao Qi wrote:
>> Pedro Alves wrote:
>>> Thanks.  Please go ahead with your patch.  Hope you don't mind I tweak
>>> the comment a bit afterwards.
>>>
>> Pedro,
>> Thanks for your view, and free to tweak the comment.
>>
>> Committed to GDB mainline,
>> http://www.cygwin.com/ml/gdb-cvs/2010-09/msg00042.html
>>
>> Once you changed the comment, I'd like to merge it to GDB 7.2 branch then.
> 
> Okay.  I've now applied this to mainline.

Committed it to GDB 7.2 branch.
http://www.cygwin.com/ml/gdb-cvs/2010-09/msg00054.html

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

[-- Attachment #2: single_step_vfork.patch --]
[-- Type: text/x-patch, Size: 2408 bytes --]

Index: infrun.c
2010-09-08  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <pedro@codesourcery.com>

	* infrunc(resume): When inferior is waiting_for_vfork_done,
	clear step and don't use displaced stepping.
	Extend comment on ignoring single-step requests on vfork 
	parents waiting for a vfork-done.

===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.445.2.1
diff -u -r1.445.2.1 infrun.c
--- infrun.c	31 Aug 2010 19:31:23 -0000	1.445.2.1
+++ infrun.c	8 Sep 2010 12:16:17 -0000
@@ -1549,6 +1549,25 @@
 
   QUIT;
 
+  /* Don't try to single-step a vfork parent that is waiting for
+     the child to get out of the shared memory region (by exec'ing
+     or exiting).  This is particularly important on software
+     single-step archs, as the child process would trip on the
+     software single step breakpoint inserted for the parent
+     process.  Since the parent will not actually execute any
+     instruction until the child is out of the shared region (such
+     are vfork's semantics), it is safe to simply continue it.
+     Eventually, we'll see a TARGET_WAITKIND_VFORK_DONE event for
+     the parent, and tell it to `keep_going', which automatically
+     re-sets it stepping.  */
+  if (current_inferior ()->waiting_for_vfork_done)
+    {
+      if (debug_infrun)
+	fprintf_unfiltered (gdb_stdlog,
+			    "infrun: resume : clear step\n");
+      step = 0;
+    }
+
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog,
                         "infrun: resume (step=%d, signal=%d), "
@@ -1576,11 +1595,16 @@
      We can't use displaced stepping when we have a signal to deliver;
      the comments for displaced_step_prepare explain why.  The
      comments in the handle_inferior event for dealing with 'random
-     signals' explain what we do instead.  */
+     signals' explain what we do instead.
+
+     We can't use displaced stepping when we are waiting for vfork_done
+     event, displaced stepping breaks the vfork child similarly as single
+     step software breakpoint.  */
   if (use_displaced_stepping (gdbarch)
       && (tp->trap_expected
 	  || (step && gdbarch_software_single_step_p (gdbarch)))
-      && sig == TARGET_SIGNAL_0)
+      && sig == TARGET_SIGNAL_0
+      && !current_inferior ()->waiting_for_vfork_done)
     {
       struct displaced_step_inferior_state *displaced;