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

[Luis Machado <lgustavo@codesourcery.com>]

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