diff mbox

Fix for follow-fork: followed child doesn't stop

Message ID 5390E4D1.2050108@codesourcery.com
State Changes Requested
Headers show

Commit Message

Don Breazeal June 5, 2014, 9:44 p.m. UTC 
Hi Luis,
Thanks for the review.

>> +		    if (tp->control.step_resume_breakpoint != NULL)
>> +		      tp->control.step_resume_breakpoint->loc->inserted = 0;
> 
> Maybe add a little more context as to why this conditional is doing what 
> it is doing? I imagine someone scratching their head to figure this out. 
> Your description above makes good sense.

I added a comment above the conditional.  Let me know if it isn't what
you had in mind.

--snip--
>> +    test_follow_fork "parent" "on" "next 2"
>> +    test_follow_fork "parent" "on" "continue"
>> +    test_follow_fork "child" "on" "next 2"
>> +    test_follow_fork "child" "on" "continue"
>> +    test_follow_fork "parent" "off" "next 2"
>> +    test_follow_fork "parent" "off" "continue"
>> +    test_follow_fork "child" "off" "next 2"
>> +    test_follow_fork "child" "off" "continue"
> 
> Instead of hardcoding calls to tests, what about creating a list of 
> permutations (follow mode, detach on fork, execution command) and using 
> them inside a for loop?
> 
> That may be a bit cleaner, but i'm not totally against these calls.
> 
I made the change as you suggested, and also added more comments.

--snip--
> 
> The rest of the test looks good to me.
> 
> Luis
> 
Thanks, the updated patch follows.
--Don

gdb/

2014-06-05  Don Breazeal  <donb@codesourcery.com>

	* infrun.c (follow_fork): clear 'inserted' flag in single-
	step breakpoint.

gdb/testsuite/

2014-06-05  Don Breazeal  <donb@codesourcery.com>

	* gdb.base/foll-fork.exp (default_fork_parent_follow):
	Deleted procedure.
	(explicit_fork_parent_follow): Deleted procedure.
	(explicit_fork_child_follow): Deleted procedure.
	(test_follow_fork): New procedure.
	(do_fork_tests): Replace calls to deleted procedures with
	calls to test_follow_fork and reset GDB for subsequent
	procedure calls.

Using the test program gdb.base/foll-fork.c, with follow-fork-mode
set to "child" and detach-on-fork set to "on", stepping or running
past the fork call results in the child process running to completion,
when it should just finish the single step.

This was the result of how the single-step state was transferred from
the parent to the child in infrun.c:follow_fork.  For the parent, the
single-step breakpoint was marked as "inserted" (bp->loc->inserted).
The breakpoint is transferred to the child, where it clearly has not
been inserted.  So when the child process is resumed, GDB doesn't insert
the breakpoint and the process runs to completion.

The solution was to clear the "inserted" flag when the single-step
breakpoint is associated with the child process.

Along with the fix above, the coverage of the follow-fork test
gdb.base/foll-fork.exp was expanded to

1) cover all the combinations of values for
   follow-fork-mode and detach-on-fork

2) make sure that both user breakpoints and
   single-step breakpoints are propagated
   correctly to the child

3) check that the inferior list has the
   expected contents after following the fork.

This was tested by running the testsuite on a Linux x64 system.

---
 gdb/infrun.c                         |   10 ++
 gdb/testsuite/gdb.base/foll-fork.exp |  173
++++++++++++++++++++++------------
 2 files changed, 122 insertions(+), 61 deletions(-)

     # followed, and continue.  Make the catchpoint permanent.

Comments

Pedro Alves June 6, 2014, 11:34 a.m. UTC | #1 
On 06/05/2014 10:44 PM, Breazeal, Don wrote:
> +proc test_follow_fork { who detach cmd } {
>      global gdb_prompt
> +    global srcfile
> +    global testfile
> 
> -    gdb_test "show follow-fork" \
> -	"Debugger response to a program call of fork or vfork is \"parent\".*" \
> -	"default show parent follow, no catchpoints"
> +    set test_name "follow $who, detach $detach, command \"$cmd\""


Instead of explicitly prepending "$test_name: " to tests,
please use with_test_prefix to make test messages unique.
Below, runto_main, the untested line, this "set verbose", all
these look like they'll print the same message in gdb.sum.

> 
> -    gdb_test "next 2" \
> -	"Detaching after fork from.*" \
> -	"default parent follow, no catchpoints"
> +    # Start a new debugger session each time so defaults are legitimate.
> +    clean_restart $testfile
> 
> -    # The child has been detached; allow time for any output it might
> -    # generate to arrive, so that output doesn't get confused with
> -    # any expected debugger output from a subsequent testpoint.
> -    #
> -    exec sleep 1
> -}
> +    if ![runto_main] {
> +	untested "could not run to main"
> +	return -1
> +    }
> 
> -proc explicit_fork_parent_follow {} {
> -    global gdb_prompt
> +    # The "Detaching..." and "Attaching..." messages may be hidden by
> +    # default.
> +    gdb_test_no_output "set verbose"



> +    # Set up the output we expect to see after we run.
> +    set expected_re ""
> +    if {$who == "child"} {
> +        set expected_re "Attaching after.* fork to.*set breakpoint here.*"
> +    } elseif {$who == "parent" && $detach == "on"} {
> +	set expected_re "Detaching after fork from .*set breakpoint here.*"

Something odd with indentation here.

> +    } else {
> +        set expected_re ".*set breakpoint here.*"
> +    }



> +    # The first two tests should be sufficient to test the defaults.
> +    # There is no need to test using the defaults in other permutations
> +    # (e.g. "default" "on", "parent" "default", etc.).
> +    set cases [list [list "default" "default" "next 2"]   \
> +                    [list "default" "default" "continue"] \
> +                    [list "parent"  "on"      "next 2"]   \
> +                    [list "parent"  "on"      "continue"] \
> +                    [list "child"   "on"      "next 2"]   \
> +                    [list "child"   "on"      "continue"] \
> +                    [list "parent"  "off"     "next 2"]   \
> +                    [list "parent"  "off"     "continue"] \
> +                    [list "child"   "off"     "next 2"]   \
> +                    [list "child"   "off"     "continue"]]
> +    foreach args $cases {
> +        test_follow_fork [lindex $args 0] [lindex $args 1] [lindex $args 2]
> +    }

This is still quite manual.  Please write:

    foreach cmd {"next 2" "continue"} {
      test_follow_fork "default" "default" $cmd
    }

    # Now test all explicit permutations.
    foreach who {"parent" "child"} {
      foreach detach {"on" "off"} {
	foreach cmd {"next 2" "continue"} {
	  test_follow_fork $who $detach $cmd
        }
      }
    }
Luis Machado June 6, 2014, 12:27 p.m. UTC | #2 
Hi Don,

On 06/05/2014 10:44 PM, Breazeal, Don wrote:
> Hi Luis,
> Thanks for the review.
>
>>> +		    if (tp->control.step_resume_breakpoint != NULL)
>>> +		      tp->control.step_resume_breakpoint->loc->inserted = 0;
>>
>> Maybe add a little more context as to why this conditional is doing what
>> it is doing? I imagine someone scratching their head to figure this out.
>> Your description above makes good sense.
>
> I added a comment above the conditional.  Let me know if it isn't what
> you had in mind.
>

That looks good to me. Thanks.

--snip--
> -    if [runto_main] then { explicit_fork_parent_follow }
> +    # The first two tests should be sufficient to test the defaults.
> +    # There is no need to test using the defaults in other permutations
> +    # (e.g. "default" "on", "parent" "default", etc.).
> +    set cases [list [list "default" "default" "next 2"]   \
> +                    [list "default" "default" "continue"] \
> +                    [list "parent"  "on"      "next 2"]   \
> +                    [list "parent"  "on"      "continue"] \
> +                    [list "child"   "on"      "next 2"]   \
> +                    [list "child"   "on"      "continue"] \
> +                    [list "parent"  "off"     "next 2"]   \
> +                    [list "parent"  "off"     "continue"] \
> +                    [list "child"   "off"     "next 2"]   \
> +                    [list "child"   "off"     "continue"]]
> +    foreach args $cases {
> +        test_follow_fork [lindex $args 0] [lindex $args 1] [lindex $args 2]
> +    }
>

I had suggested what Pedro did, but my mail server wasn't too happy with 
sending my mails out.

Luis
diff mbox

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 47604c7..689f0cb 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -522,6 +522,16 @@  follow_fork (void)
 		    tp = inferior_thread ();
 		    tp->control.step_resume_breakpoint
 		      = step_resume_breakpoint;
+
+		    /* We clear the INSERTED flag since the step-resume
+		       breakpoint has not been inserted in the new child
+		       process.  This doesn't affect the parent process,
+		       since the breakpoint is a clone of the parent's
+		       breakpoint, which was deleted above after it was
+		       cloned.  */
+		    if (tp->control.step_resume_breakpoint != NULL)
+		      tp->control.step_resume_breakpoint->loc->inserted = 0;
+
 		    tp->control.step_range_start = step_range_start;
 		    tp->control.step_range_end = step_range_end;
 		    tp->control.step_frame_id = step_frame_id;
diff --git a/gdb/testsuite/gdb.base/foll-fork.exp
b/gdb/testsuite/gdb.base/foll-fork.exp
index e1201d7..c4ef860 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -53,60 +53,109 @@  proc check_fork_catchpoints {} {
   }
 }

-proc default_fork_parent_follow {} {
+# Test follow-fork to ensure that the correct process is followed, that
+# the followed process stops where it is expected to stop, that processes
+# are detached (or not) as expected, and that the inferior list has the
+# expected contents after following the fork.  WHO is the argument to
+# the 'set follow-fork-mode' command, DETACH is the argument to the
+# 'set detach-on-fork' command, and CMD is the GDB command used to
+# execute the program past the fork.  If the value of WHO or DETACH is
+# 'default', the corresponding GDB command is skipped for that test.
+# The value of CMD must be either 'next 2' or 'continue'.
+proc test_follow_fork { who detach cmd } {
     global gdb_prompt
+    global srcfile
+    global testfile

-    gdb_test "show follow-fork" \
-	"Debugger response to a program call of fork or vfork is \"parent\".*" \
-	"default show parent follow, no catchpoints"
+    set test_name "follow $who, detach $detach, command \"$cmd\""

-    gdb_test "next 2" \
-	"Detaching after fork from.*" \
-	"default parent follow, no catchpoints"
+    # Start a new debugger session each time so defaults are legitimate.
+    clean_restart $testfile

-    # The child has been detached; allow time for any output it might
-    # generate to arrive, so that output doesn't get confused with
-    # any expected debugger output from a subsequent testpoint.
-    #
-    exec sleep 1
-}
+    if ![runto_main] {
+	untested "could not run to main"
+	return -1
+    }

-proc explicit_fork_parent_follow {} {
-    global gdb_prompt
+    # The "Detaching..." and "Attaching..." messages may be hidden by
+    # default.
+    gdb_test_no_output "set verbose"

-    gdb_test_no_output "set follow-fork parent"
+    # Set follow-fork-mode if we aren't using the default.
+    if {$who == "default"} {
+        set who "parent"
+    } else {
+        gdb_test_no_output "set follow-fork $who"
+    }

     gdb_test "show follow-fork" \
-	"Debugger response to a program call of fork or vfork is \"parent\"." \
-	"explicit show parent follow, no catchpoints"
+	"Debugger response to a program call of fork or vfork is \"$who\"." \
+	"$test_name: show follow-fork"
+
+    # Set detach-on-fork mode if we aren't using the default.
+    if {$detach == "default"} {
+        set detach "on"
+    } else {
+        gdb_test_no_output "set detach-on-fork $detach"
+    }

-    gdb_test "next 2" "Detaching after fork from.*" \
-	"explicit parent follow, no catchpoints"
+    gdb_test "show detach-on-fork" \
+	"Whether gdb will detach.* fork is $detach." \
+	"$test_name: show detach-on-fork"
+
+    # Set the breakpoint after the fork if we aren't single-stepping
+    # past the fork.
+    if {$cmd == "continue"} {
+	set bp_after_fork [gdb_get_line_number "set breakpoint here"]
+	gdb_test "break ${srcfile}:$bp_after_fork" \
+	    "Breakpoint.*, line $bp_after_fork.*" \
+	    "$test_name: set breakpoint after fork"
+    }

-    # The child has been detached; allow time for any output it might
-    # generate to arrive, so that output doesn't get confused with
-    # any expected debugger output from a subsequent testpoint.
-    #
-    exec sleep 1
-}
+    # Set up the output we expect to see after we run.
+    set expected_re ""
+    if {$who == "child"} {
+        set expected_re "Attaching after.* fork to.*set breakpoint here.*"
+    } elseif {$who == "parent" && $detach == "on"} {
+	set expected_re "Detaching after fork from .*set breakpoint here.*"
+    } else {
+        set expected_re ".*set breakpoint here.*"
+    }

-proc explicit_fork_child_follow {} {
-    global gdb_prompt
+    # Test running past and following the fork, using the parameters
+    # set above.
+    gdb_test $cmd $expected_re "$test_name: $cmd past fork"

-    gdb_test_no_output "set follow-fork child"
+    # Check that we have the inferiors arranged correctly after
+    # following the fork.
+    if {$who == "parent" && $detach == "on"} {

-    gdb_test "show follow-fork" \
-	"Debugger response to a program call of fork or vfork is \"child\"." \
-	"explicit show child follow, no catchpoints"
+      # Follow parent / detach child: the only inferior is the parent.
+      gdb_test "info inferior" "\\* 1 .* process.*" \
+          "$test_name: info inferiors"

-    gdb_test "next 2" "Attaching after.* fork to.*" \
-	"explicit child follow, no catchpoints"
+    } elseif {$who == "parent" && $detach == "off"} {

-    # The child has been detached; allow time for any output it might
-    # generate to arrive, so that output doesn't get confused with
-    # any gdb_expected debugger output from a subsequent testpoint.
-    #
-    exec sleep 1
+      # Follow parent / keep child: two inferiors under debug, the
+      # parent is the current inferior.
+      gdb_test "info inferior" " 2 .*process.*\\* 1 .*process.*" \
+          "$test_name: info inferiors"
+
+    } elseif {$who == "child" && $detach == "on"} {
+
+      # Follow child / detach parent: the child is under debug and is
+      # the current inferior.  The parent is listed but is not under
+      # debug.
+      gdb_test "info inferior" "\\* 2 .*process.* 1 .*<null>.*" \
+          "$test_name: info inferiors"
+
+    } elseif {$who == "child" && $detach == "off"} {
+
+      # Follow parent / keep child: two inferiors under debug, the
+      # child is the current inferior.
+      gdb_test "info inferior" "\\* 2 .*process.* 1 .*process.*" \
+          "$test_name: info inferiors"
+    }
 }

 proc catch_fork_child_follow {} {
@@ -254,6 +303,7 @@  proc tcatch_fork_parent_follow {} {

 proc do_fork_tests {} {
     global gdb_prompt
+    global testfile

     # Verify that help is available for "set follow-fork-mode".
     #
@@ -286,31 +336,32 @@  By default, the debugger will follow the parent
process..*" \
     # fork-following is supported.
     if [runto_main] then { check_fork_catchpoints }

-    # Test the default behaviour, which is to follow the parent of a
-    # fork, and detach from the child.  Do this without catchpoints.
-    #
-    if [runto_main] then { default_fork_parent_follow }
-
-    # Test the ability to explicitly follow the parent of a fork, and
-    # detach from the child.  Do this without catchpoints.
+    # Test the basic follow-fork functionality using all combinations of
+    # values for follow-fork-mode and detach-on-fork, using either a
+    # breakpoint or single-step to execute past the fork.
     #
-    if [runto_main] then { explicit_fork_parent_follow }
+    # The first two tests should be sufficient to test the defaults.
+    # There is no need to test using the defaults in other permutations
+    # (e.g. "default" "on", "parent" "default", etc.).
+    set cases [list [list "default" "default" "next 2"]   \
+                    [list "default" "default" "continue"] \
+                    [list "parent"  "on"      "next 2"]   \
+                    [list "parent"  "on"      "continue"] \
+                    [list "child"   "on"      "next 2"]   \
+                    [list "child"   "on"      "continue"] \
+                    [list "parent"  "off"     "next 2"]   \
+                    [list "parent"  "off"     "continue"] \
+                    [list "child"   "off"     "next 2"]   \
+                    [list "child"   "off"     "continue"]]
+    foreach args $cases {
+        test_follow_fork [lindex $args 0] [lindex $args 1] [lindex $args 2]
+    }

-    # Test the ability to follow the child of a fork, and detach from
-    # the parent.  Do this without catchpoints.
-    #
-    if [runto_main] then { explicit_fork_child_follow }
+    # Catchpoint tests.

-    # Test the ability to follow both child and parent of a fork.  Do
-    # this without catchpoints.
-    # ??rehrauer: NYI.  Will add testpoints here when implemented.
-    #
-
-    # Test the ability to have the debugger ask the user at fork-time
-    # whether to follow the parent, child or both.  Do this without
-    # catchpoints.
-    # ??rehrauer: NYI.  Will add testpoints here when implemented.
-    #
+    # Restart to eliminate any effects of the follow-fork tests.
+    clean_restart $testfile
+    gdb_test_no_output "set verbose"

     # Test the ability to catch a fork, specify that the child be