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

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

[Don Breazeal]

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

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

The solution is 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.

Regards,
Don

gdb/

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

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

gdb/testsuite/

2014-06-04  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.


---
 gdb/infrun.c                         |    2 +
 gdb/testsuite/gdb.base/foll-fork.exp |  166 +++++++++++++++++++++-------------
 2 files changed, 104 insertions(+), 64 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 47604c7..6c70cd0 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -522,6 +522,8 @@ follow_fork (void)
 		    tp = inferior_thread ();
 		    tp->control.step_resume_breakpoint
 		      = step_resume_breakpoint;
+		    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..c1ba041 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -53,60 +53,100 @@ proc check_fork_catchpoints {} {
   }
 }
 
-proc default_fork_parent_follow {} {
+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 +294,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 +327,28 @@ 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.
-    #
-    if [runto_main] then { explicit_fork_parent_follow }
-
-    # 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 }
-
-    # 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.
-    #
+    # Test all combinations of values for follow-fork-mode and
+    # detach-on-fork, using both a breakpoint and single-step
+    # to execute past the fork.
+    test_follow_fork "default" "default" "next 2"
+    test_follow_fork "default" "default" "continue"
+    # At this point we should be convinced that the defaults are either
+    # working or not working, so there is no need to test further using
+    # the defaults (e.g. "default" "on", "parent" "default", etc.).
+    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"
+
+    # Catchpoint tests.
+
+    # 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
     # followed, and continue.  Make the catchpoint permanent.
-- 
1.7.0.4

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Pedro Alves]

Hi Don,

On 06/04/2014 11:19 PM, Don Breazeal wrote:
> Using the test program gdb.base/foll-fork.c, with follow-fork-mode
> set to "child" and detach-on-fork set to "on", stepping past the
> fork call results in the child process running to completion, when
> it should just finish the single step.
> 
> This is the result of how the single-step state is transferred from
> the parent to the child in infrun.c:follow_fork.  For the parent, the
> single-step breakpoint is marked as "inserted" (bp->loc->inserted).

> The breakpoint is transferred to the child, where it clearly has never
> been inserted.  

Was it removed from the parent already at this point?  If so,
why is it still marked as inserted?  If not, then it would sound
like your patch would make us miss removing it.

-- 
Pedro Alves

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Luis Machado]

Hi,

On 06/04/2014 11:19 PM, Don Breazeal wrote:
> Using the test program gdb.base/foll-fork.c, with follow-fork-mode
> set to "child" and detach-on-fork set to "on", stepping past the
> fork call results in the child process running to completion, when
> it should just finish the single step.
>
> This is the result of how the single-step state is transferred from
> the parent to the child in infrun.c:follow_fork.  For the parent, the
> single-step breakpoint is marked as "inserted" (bp->loc->inserted).
> The breakpoint is transferred to the child, where it clearly has never
> been inserted.  So when the child process is resumed, GDB doesn't insert
> the breakpoint and the process runs to completion.
>
> The solution is 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.
>
> Regards,
> Don
>

Thanks. That is a nice addition.


> gdb/
>
> 2014-06-04  Don Breazeal  <donb@codesourcery.com>
>
> 	* infrun.c (follow_fork): clear 'inserted' flag in single-
> 	step breakpoint.
>
> gdb/testsuite/
>
> 2014-06-04  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.
>
>
> ---
>   gdb/infrun.c                         |    2 +
>   gdb/testsuite/gdb.base/foll-fork.exp |  166 +++++++++++++++++++++-------------
>   2 files changed, 104 insertions(+), 64 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 47604c7..6c70cd0 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -522,6 +522,8 @@ follow_fork (void)
>   		    tp = inferior_thread ();
>   		    tp->control.step_resume_breakpoint
>   		      = step_resume_breakpoint;
> +		    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;


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.

> diff --git a/gdb/testsuite/gdb.base/foll-fork.exp b/gdb/testsuite/gdb.base/foll-fork.exp
> index e1201d7..c1ba041 100644
> --- a/gdb/testsuite/gdb.base/foll-fork.exp
> +++ b/gdb/testsuite/gdb.base/foll-fork.exp
> @@ -53,60 +53,100 @@ proc check_fork_catchpoints {} {
>     }
>   }
>
> -proc default_fork_parent_follow {} {
> +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 +294,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 +327,28 @@ 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.
> -    #
> -    if [runto_main] then { explicit_fork_parent_follow }
> -
> -    # 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 }
> -
> -    # 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.
> -    #
> +    # Test all combinations of values for follow-fork-mode and
> +    # detach-on-fork, using both a breakpoint and single-step
> +    # to execute past the fork.
> +    test_follow_fork "default" "default" "next 2"
> +    test_follow_fork "default" "default" "continue"
> +    # At this point we should be convinced that the defaults are either
> +    # working or not working, so there is no need to test further using
> +    # the defaults (e.g. "default" "on", "parent" "default", etc.).
> +    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.

> +
> +    # Catchpoint tests.
> +
> +    # 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
>       # followed, and continue.  Make the catchpoint permanent.
>

The rest of the test looks good to me.

Luis

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Breazeal, Don]

Hi Pedro,
Thanks for looking at this.

On 6/5/2014 5:52 AM, Pedro Alves wrote:
> Hi Don,
> 
> On 06/04/2014 11:19 PM, Don Breazeal wrote:
>> Using the test program gdb.base/foll-fork.c, with follow-fork-mode
>> set to "child" and detach-on-fork set to "on", stepping past the
>> fork call results in the child process running to completion, when
>> it should just finish the single step.
>>
>> This is the result of how the single-step state is transferred from
>> the parent to the child in infrun.c:follow_fork.  For the parent, the
>> single-step breakpoint is marked as "inserted" (bp->loc->inserted).
> 
>> The breakpoint is transferred to the child, where it clearly has never
>> been inserted.  
> 
> Was it removed from the parent already at this point?  If so,
> why is it still marked as inserted?  If not, then it would sound
> like your patch would make us miss removing it.
> 
Yes, by the time the 'inserted' flag is cleared, the breakpoint has been
removed from the parent.  The flag is set because the child's breakpoint
is a clone of the parent's breakpoint that was created before the
parent's breakpoint was removed.

The step-resume breakpoint is cloned from the parent's copy earlier in
the function, bringing the value of the 'inserted' flag along with it.
Then the parent's breakpoint is deleted, with the side-effect of
removing it.

From infrun.c:follow_fork:

	[...here 'tp' is the parent thread...]
        if (follow_child && should_resume)
          {
            step_resume_breakpoint = clone_momentary_breakpoint
                                 (tp->control.step_resume_breakpoint);
	    [...]
            delete_step_resume_breakpoint (tp);
	    [...here bkpt has been removed from the parent...]
          }
	[...calls target_follow_fork...]
            /* If we followed the child, switch to it...  */
            if (follow_child)
              {
                switch_to_thread (child);

                /* ... and preserve the stepping state, in case the
                   user was stepping over the fork call.  */
                if (should_resume)
                  {
                    tp = inferior_thread ();
                    tp->control.step_resume_breakpoint
                      = step_resume_breakpoint;
		    [...this only affects the child process...]
                    if (tp->control.step_resume_breakpoint != NULL)
                      tp->control.step_resume_breakpoint->loc->inserted = 0;

Does that explanation make sense?

Luis suggests adding a comment that explains why the flag is cleared.
Would that be sufficient?

Thanks
--Don

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Breazeal, Don]

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(-)

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
     # followed, and continue.  Make the catchpoint permanent.
-- 
1.7.0.4

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Pedro Alves]

On 06/05/2014 07:34 PM, Breazeal, Don wrote:
> Hi Pedro,
> Thanks for looking at this.
> 
> On 6/5/2014 5:52 AM, Pedro Alves wrote:
>> Hi Don,
>>
>> On 06/04/2014 11:19 PM, Don Breazeal wrote:
>>> Using the test program gdb.base/foll-fork.c, with follow-fork-mode
>>> set to "child" and detach-on-fork set to "on", stepping past the
>>> fork call results in the child process running to completion, when
>>> it should just finish the single step.
>>>
>>> This is the result of how the single-step state is transferred from
>>> the parent to the child in infrun.c:follow_fork.  For the parent, the
>>> single-step breakpoint is marked as "inserted" (bp->loc->inserted).
>>
>>> The breakpoint is transferred to the child, where it clearly has never
>>> been inserted.  
>>
>> Was it removed from the parent already at this point?  If so,
>> why is it still marked as inserted?  If not, then it would sound
>> like your patch would make us miss removing it.
>>
> Yes, by the time the 'inserted' flag is cleared, the breakpoint has been
> removed from the parent.  The flag is set because the child's breakpoint
> is a clone of the parent's breakpoint that was created before the
> parent's breakpoint was removed.

It actually hasn't.

> 
> The step-resume breakpoint is cloned from the parent's copy earlier in
> the function, bringing the value of the 'inserted' flag along with it.
> Then the parent's breakpoint is deleted, with the side-effect of
> removing it.

No, when the parent's breakpoint is deleted, nothing is removed,
because the child's breakpoint, being a clone of the parent's is
still associated with the parent thread and pspace, so the breakpoints
module considers it a duplicate of the parent's sr breakpoint.
So, when we delete the parent breakpoint, given the child's is
around, we don't actually remove the breakpoint from the
target.  We just happen to remove the new _child_'s breakpoint
from the parent a bit later when we detach the parent, again
because the child's is still associated with the parent:

(top-gdb) bt
#0  target_remove_breakpoint (gdbarch=0x1000ee0, bp_tgt=0x10db7d8) at ../../src/gdb/target.c:1938
#1  0x0000000000584aac in bkpt_remove_location (bl=0x10db740) at ../../src/gdb/breakpoint.c:13189
#2  0x0000000000574095 in remove_breakpoint_1 (bl=0x10db740, is=mark_uninserted) at ../../src/gdb/breakpoint.c:3823
#3  0x0000000000574436 in remove_breakpoint (bl=0x10db740, is=mark_uninserted) at ../../src/gdb/breakpoint.c:3940
#4  0x0000000000572d41 in remove_breakpoints_pid (pid=1408) at ../../src/gdb/breakpoint.c:3119
#5  0x000000000063d5f8 in target_detach (args=0x0, from_tty=0) at ../../src/gdb/target.c:2079
#6  0x00000000004b1c81 in linux_child_follow_fork (ops=0xdcfc30, follow_child=1, detach_fork=1) at ../../src/gdb/linux-nat.c:657
#7  0x00000000006372b6 in delegate_follow_fork (self=0xdcfc30, arg1=1, arg2=1) at ../../src/gdb/target-delegates.c:464
#8  0x000000000063daed in target_follow_fork (follow_child=1, detach_fork=1) at ../../src/gdb/target.c:2220
#9  0x00000000005eda7a in follow_fork () at ../../src/gdb/infrun.c:493
(top-gdb) up
#1  0x0000000000584aac in bkpt_remove_location (bl=0x10db740) at ../../src/gdb/breakpoint.c:13189
13189         return target_remove_breakpoint (bl->gdbarch, bp_tgt);
(top-gdb) p bl->owner->type
$8 = bp_step_resume

> 
> From infrun.c:follow_fork:
> 
> 	[...here 'tp' is the parent thread...]
>         if (follow_child && should_resume)
>           {
>             step_resume_breakpoint = clone_momentary_breakpoint
>                                  (tp->control.step_resume_breakpoint);
> 	    [...]
>             delete_step_resume_breakpoint (tp);
> 	    [...here bkpt has been removed from the parent...]

It hasn't here no.  This is all quite brittle.  :-/

I think we should instead make the clone be disabled to begin with,
so that it isn't marked as duplicate/inserted, and then re-enable it
in follow_inferior_reset_breakpoints, _after_ it's been re_set
to the child thread.

-- 
Pedro Alves

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Pedro Alves]

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

-- 
Pedro Alves


-- 
Pedro Alves

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Pedro Alves]

Also, exception_resume_breakpoint surely needs the same treatment.

-- 
Pedro Alves

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Luis Machado]

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

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Breazeal, Don]

Hi Pedro,
Thanks for clarifying this for me.

On 6/6/2014 4:34 AM, Pedro Alves wrote:
> On 06/05/2014 10:44 PM, Breazeal, Don wrote:
>> +proc test_follow_fork { who detach cmd } {
--snip--
> 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.
> 
Done.

--snip--
>> +    } elseif {$who == "parent" && $detach == "on"} {
>> +	set expected_re "Detaching after fork from .*set breakpoint here.*"
> 
> Something odd with indentation here.
> 
Fixed.

--snip--
>> +    # 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
>         }
>       }
>     }
> 
Done.

On 6/6/2014 4:12 AM, Pedro Alves wrote:
> I think we should instead make the clone be disabled to begin with,
> so that it isn't marked as duplicate/inserted, and then re-enable it
> in follow_inferior_reset_breakpoints, _after_ it's been re_set
> to the child thread.

Here is the updated patch with (one hopes) fewer assumptions and errors.
 The problem statement has been corrected (detach-on-fork set to "off",
not "on", although in the end this was mostly irrelevant), the fix and
tests have been revised per comments, and the test slightly enhanced to
check that unfollowed inferiors can be resumed.

Symptom:

Using the test program gdb.base/foll-fork.c, with follow-fork-mode set
to "child" and detach-on-fork set to "off", stepping past the fork call
results in the child process running to completion, when it should just
finish the single step.  In addition, the single-step breakpoint is not
removed from the parent process, so if it is resumed it receives a SIGTRAP.

Cause:

No matter what the setting for detach-on-fork, when stepping past a
fork, the insertion state of the single-step breakpoint
(step_resume_breakpoint) is not handled correctly in the parent.  The SR
breakpoint is cloned for use with the child process. Before the clone is
associated with the child, it is treated as a duplicate of the original,
associated with the parent.  This results in the insertion state of the
original SR breakpoint and the clone being "swapped" by
breakpoint.c:update_global_location_list, so that the clone is marked as
inserted.

In the case where the parent is not detached, the two breakpoints remain
in the state (clone marked inserted, original not marked inserted).  As
a result the breakpoint is never inserted in the child, because although
the cloned SR breakpoint is eventually associated with the child, it is
marked as already inserted.  When the child is resumed, it runs to
completion.  Similarly, the breakpoint is never removed from the parent,
so that if it is resumed after the child exits, it gets a SIGTRAP.

In the case where the parent is detached (via target_detach), the clone
is still associated with the parent and marked as inserted, so the SR
breakpoint is removed from the parent and the clone's inserted flag is
cleared.  Although there are no visible symptoms, the maintenance of the
'inserted' flag is incorrect.

Fix:

Set the cloned breakpoint as disabled from the moment it is created.
This is done by modifying clone_momentary_breakpoint to take an
additional argument, LOC_ENABLED, which is used as the value of the
bp_location->enabled member.  The clone must be disabled at that point,
rather than later, because clone_momentary_breakpoint calls
update_global_location_list, which will treat the clone as a duplicate
of the original breakpoint if it is enabled.

All the calls to clone_momentary_breakpoint had to be modified to pass
'1' or '0'.  I looked at implementing an enum for the enabled member,
but concluded that readability would suffer because there are so many
places it is used as a boolean, e.g. "if (bl->enabled)".

In follow_inferior_reset_breakpoints, the clone is set to enabled once
it has been associated with the child process.  With this, the
bp_location 'inserted' member is maintained correctly throughout the
follow-fork procedure and the behavior is as expected.

The same treatment is given to the exception_resume_breakpoint when
following a fork.

Testing:

Ran 'make check' on Linux x64.

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.

4) check that unfollowed, undetached inferiors can be resumed.


Thanks
--Don


gdb/

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

	* breakpoint.c (set_longjmp_breakpoint): Call
	momentary_breakpoint_from_master with additional argument.
	(set_longjmp_breakpoint_for_call_dummy): Call
	momentary_breakpoint_from_master with additional argument.
	(set_std_terminate_breakpoint): Call
	momentary_breakpoint_from_master with additional argument.
	(momentary_breakpoint_from_master): Add argument to function
	definition and use it to initialize structure member flag.
	(clone_momentary_breakpoint): Call
	momentary_breakpoint_from_master with additional argument.
	* infrun.c (follow_inferior_reset_breakpoints): Clear structure
	member flags set in momentary_breakpoint_from_master.

gdb/testsuite/

2014-06-12  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.

---
 gdb/breakpoint.c                     |   20 ++--
 gdb/infrun.c                         |   15 ++-
 gdb/testsuite/gdb.base/foll-fork.exp |  193
+++++++++++++++++++++++-----------
 3 files changed, 156 insertions(+), 72 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 23c8895..11fe5a7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -154,7 +154,8 @@ struct breakpoint *set_raw_breakpoint (struct
gdbarch *gdbarch,
 static struct breakpoint *
   momentary_breakpoint_from_master (struct breakpoint *orig,
 				    enum bptype type,
-				    const struct breakpoint_ops *ops);
+				    const struct breakpoint_ops *ops,
+				    int loc_enabled);

 static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);

@@ -7390,7 +7391,7 @@ set_longjmp_breakpoint (struct thread_info *tp,
struct frame_id frame)
 	/* longjmp_breakpoint_ops ensures INITIATING_FRAME is cleared again
 	   after their removal.  */
 	clone = momentary_breakpoint_from_master (b, type,
-						  &longjmp_breakpoint_ops);
+						  &longjmp_breakpoint_ops, 1);
 	clone->thread = thread;
       }

@@ -7440,7 +7441,8 @@ set_longjmp_breakpoint_for_call_dummy (void)
 	struct breakpoint *new_b;

 	new_b = momentary_breakpoint_from_master (b, bp_longjmp_call_dummy,
-						  &momentary_breakpoint_ops);
+						  &momentary_breakpoint_ops,
+						  1);
 	new_b->thread = pid_to_thread_id (inferior_ptid);

 	/* Link NEW_B into the chain of RETVAL breakpoints.  */
@@ -7533,7 +7535,7 @@ set_std_terminate_breakpoint (void)
 	&& b->type == bp_std_terminate_master)
       {
 	momentary_breakpoint_from_master (b, bp_std_terminate,
-					  &momentary_breakpoint_ops);
+					  &momentary_breakpoint_ops, 1);
       }
 }

@@ -9051,13 +9053,14 @@ set_momentary_breakpoint (struct gdbarch
*gdbarch, struct symtab_and_line sal,
 }

 /* Make a momentary breakpoint based on the master breakpoint ORIG.
-   The new breakpoint will have type TYPE, and use OPS as it
-   breakpoint_ops.  */
+   The new breakpoint will have type TYPE, use OPS as its
+   breakpoint_ops, and will set enabled to LOC_ENABLED.  */

 static struct breakpoint *
 momentary_breakpoint_from_master (struct breakpoint *orig,
 				  enum bptype type,
-				  const struct breakpoint_ops *ops)
+				  const struct breakpoint_ops *ops,
+				  int loc_enabled)
 {
   struct breakpoint *copy;

@@ -9073,6 +9076,7 @@ momentary_breakpoint_from_master (struct
breakpoint *orig,
   copy->loc->probe = orig->loc->probe;
   copy->loc->line_number = orig->loc->line_number;
   copy->loc->symtab = orig->loc->symtab;
+  copy->loc->enabled = loc_enabled;
   copy->frame_id = orig->frame_id;
   copy->thread = orig->thread;
   copy->pspace = orig->pspace;
@@ -9095,7 +9099,7 @@ clone_momentary_breakpoint (struct breakpoint *orig)
   if (orig == NULL)
     return NULL;

-  return momentary_breakpoint_from_master (orig, orig->type, orig->ops);
+  return momentary_breakpoint_from_master (orig, orig->type, orig->ops, 0);
 }

 struct breakpoint *
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 47604c7..4e808d9 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -569,7 +569,9 @@ follow_inferior_reset_breakpoints (void)

   /* Was there a step_resume breakpoint?  (There was if the user
      did a "next" at the fork() call.)  If so, explicitly reset its
-     thread number.
+     thread number.  Cloned step_resume breakpoints are disabled on
+     creation, so enable it here now that it is associated with the
+     correct thread.

      step_resumes are a form of bp that are made to be per-thread.
      Since we created the step_resume bp when the parent process
@@ -579,10 +581,17 @@ follow_inferior_reset_breakpoints (void)
      it is for, or it'll be ignored when it triggers.  */

   if (tp->control.step_resume_breakpoint)
-    breakpoint_re_set_thread (tp->control.step_resume_breakpoint);
+    {
+      breakpoint_re_set_thread (tp->control.step_resume_breakpoint);
+      tp->control.step_resume_breakpoint->loc->enabled = 1;
+    }

+  /* Treat exception_resume breakpoints like step_resume breakpoints.  */
   if (tp->control.exception_resume_breakpoint)
-    breakpoint_re_set_thread (tp->control.exception_resume_breakpoint);
+    {
+      breakpoint_re_set_thread (tp->control.exception_resume_breakpoint);
+      tp->control.exception_resume_breakpoint->loc->enabled = 1;
+    }

   /* Reinsert all breakpoints in the child.  The user may have set
      breakpoints after catching the fork, in which case those
diff --git a/gdb/testsuite/gdb.base/foll-fork.exp
b/gdb/testsuite/gdb.base/foll-fork.exp
index e1201d7..ad8b750 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -53,60 +53,130 @@ 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"
+    with_test_prefix "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"
+	gdb_test "show follow-fork" \
+	"Debugger response to a program call of fork or vfork is \"$who\"." \
+	"show follow-fork"

-    gdb_test "next 2" "Detaching after fork from.*" \
-	"explicit parent follow, no catchpoints"
+	# 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"
+	}

-    # 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
-}
+	gdb_test "show detach-on-fork" \
+		 "Whether gdb will detach.* fork is $detach." \
+		 "show detach-on-fork"
+
+	# Set a 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.*" \
+		     "set breakpoint after fork"
+	}

-proc explicit_fork_child_follow {} {
-    global gdb_prompt
+	# 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.*"
+	}

-    gdb_test_no_output "set follow-fork child"
+	# Test running past and following the fork, using the parameters
+	# set above.
+	gdb_test $cmd $expected_re "$cmd past fork"

-    gdb_test "show follow-fork" \
-	"Debugger response to a program call of fork or vfork is \"child\"." \
-	"explicit show child follow, no catchpoints"
+	# Check that we have the inferiors arranged correctly after
+	# following the fork.
+	set resume_unfollowed 0
+	if {$who == "parent" && $detach == "on"} {

-    gdb_test "next 2" "Attaching after.* fork to.*" \
-	"explicit child follow, no catchpoints"
+	    # Follow parent / detach child: the only inferior is the parent.
+	    gdb_test "info inferiors" "\\* 1 .* process.*" \
+		     "info inferiors"

-    # 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
+	} elseif {$who == "parent" && $detach == "off"} {
+
+	    # Follow parent / keep child: two inferiors under debug, the
+	    # parent is the current inferior.
+	    gdb_test "info inferiors" " 2 .*process.*\\* 1 .*process.*" \
+		     "info inferiors"
+
+	    gdb_test "inferior 2" "Switching to inferior 2 .*"
+	    set resume_unfollowed 1
+
+	} 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 inferiors" "\\* 2 .*process.* 1 .*<null>.*" \
+		     "info inferiors"
+
+	} elseif {$who == "child" && $detach == "off"} {
+
+	    # Follow child / keep parent: two inferiors under debug, the
+	    # child is the current inferior.
+	    gdb_test "info inferiors" "\\* 2 .*process.* 1 .*process.*" \
+		     "info inferiors"
+
+	    gdb_test "inferior 1" "Switching to inferior 1 .*"
+	    set resume_unfollowed 1
+	}
+
+	if {$resume_unfollowed == 1} {
+	    if {$cmd == "next 2"} {
+
+		gdb_continue_to_end "continue unfollowed inferior to end"
+
+	    } elseif {$cmd == "continue"} {
+
+		gdb_continue_to_breakpoint \
+		    "continue unfollowed inferior to bp" \
+		    ".* set breakpoint here.*"
+	    }
+	}
+    }
 }

 proc catch_fork_child_follow {} {
@@ -254,6 +324,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 +357,31 @@ 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.
+    # 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 { default_fork_parent_follow }
-
-    # Test the ability to explicitly follow the parent of a fork, and
-    # detach from the child.  Do this without catchpoints.
-    #
-    if [runto_main] then { explicit_fork_parent_follow }
+    # The first loop 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.).
+    foreach cmd {"next 2" "continue"} {
+        test_follow_fork "default" "default" $cmd
+    }

-    # 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 }
+    # Now test all explicit permutations.
+    foreach who {"parent" "child"} {
+	foreach detach {"on" "off"} {
+	    foreach cmd {"next 2" "continue"} {
+		test_follow_fork $who $detach $cmd
+	    }
+	}
+    }

-    # Test the ability to follow both child and parent of a fork.  Do
-    # this without catchpoints.
-    # ??rehrauer: NYI.  Will add testpoints here when implemented.
-    #
+    # Catchpoint tests.

-    # 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
     # followed, and continue.  Make the catchpoint permanent.
-- 
1.7.0.4

Re: [PATCH] Fix for follow-fork: followed child doesn't stop

[Pedro Alves]

On 06/13/2014 02:06 AM, Breazeal, Don wrote:
> 
> Hi Pedro,
> Thanks for clarifying this for me.

Thanks for working on this.

> 
> On 6/6/2014 4:34 AM, Pedro Alves wrote:

> Set the cloned breakpoint as disabled from the moment it is created.
> This is done by modifying clone_momentary_breakpoint to take an
> additional argument, LOC_ENABLED, which is used as the value of the
> bp_location->enabled member.  The clone must be disabled at that point,
> rather than later, because clone_momentary_breakpoint calls
> update_global_location_list, which will treat the clone as a duplicate
> of the original breakpoint if it is enabled.
> 
> All the calls to clone_momentary_breakpoint had to be modified to pass
> '1' or '0'.  I looked at implementing an enum for the enabled member,
> but concluded that readability would suffer because there are so many
> places it is used as a boolean, e.g. "if (bl->enabled)".
> 
> In follow_inferior_reset_breakpoints, the clone is set to enabled once
> it has been associated with the child process.  With this, the
> bp_location 'inserted' member is maintained correctly throughout the
> follow-fork procedure and the behavior is as expected.
> 
> The same treatment is given to the exception_resume_breakpoint when
> following a fork.
> 
> Testing:
> 
> Ran 'make check' on Linux x64.
> 
> 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.
> 
> 4) check that unfollowed, undetached inferiors can be resumed.
> 

Excellent.

This version is OK.

Thanks!

-- 
Pedro Alves