[patch] testsuite: Fix a racy FAIL on gdb.base/multi-forks.exp

[patch] testsuite: Fix a racy FAIL on gdb.base/multi-forks.exp

[Jan Kratochvil]

Hi,

Fix a racy FAIL on gdb.base/multi-forks.exp <follow child, print pids>.

Instead of the output suppression by `close (1)' some sleep would also fix it
if close (1) is not compatible enough.


Thanks,
Jan

    
13836 done
[Switching to process 13836]

13839 done
Breakpoint 2, main () at ../.././gdb/testsuite/gdb.base/multi-forks.c:35
35        exit (0);     /* Set exit breakpoint here.  */
(gdb) 13841 done
13842 done
13835 done
13844 done
print pids
13845 done
13838 done
13846 done
13840 done
13834 done
13847 done
13843 done
13837 done
13833 done
$1 = {0, 013826 done
, 0, 0}
(gdb) FAIL: gdb.base/multi-forks.exp: follow child, print pids


gdb/testsuite/
2009-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix a racy FAIL.
	* gdb.base/multi-forks.exp (follow child, print pids): Call `close (1)'.

diff --git a/gdb/testsuite/gdb.base/multi-forks.exp b/gdb/testsuite/gdb.base/multi-forks.exp
index ac56fb0..70c1a20 100644
--- a/gdb/testsuite/gdb.base/multi-forks.exp
+++ b/gdb/testsuite/gdb.base/multi-forks.exp
@@ -66,6 +66,10 @@ global gdb_prompt
 # The result should be that each of the 4 forks returns zero.
 
 runto_main
+
+gdb_test "call close (1)" "= 0" \
+	 "Suppress the inferior output mixing with GDB output"
+
 set exit_bp_loc [gdb_get_line_number "Set exit breakpoint here."]
 gdb_test "break $exit_bp_loc" "Breakpoint.* at .*" "Break at exit"
 gdb_test "set follow child" "" ""

Re: [patch] testsuite: Fix a racy FAIL on gdb.base/multi-forks.exp

[Joel Brobecker]

I am not very familiar with this testcase, so please consider this
as an informal review. I started looking at this just because I was
actually wondering about the availability of "close"....

> Instead of the output suppression by `close (1)' some sleep would also fix it
> if close (1) is not compatible enough.

As far as I can tell, "close" is POSIX, so should be fairly portable.
I'm not entirely sure about Windows, but I do think it's available.
The symbol name might be _close, though.

That being said, I see later in the same testcase that this problem
is already handled a different way. Basically, we know that the
inferiors are going to print a bunch of traces, so we just wait
for all of them to be printed before we do the next test:

    # The output from the child processes can be interleaved arbitrarily
    # with the output from GDB and the parent process.  If we don't
    # consume it all now, it can confuse later interactions.
    set seen_done 0
    set seen_break 0
    set seen_prompt 0
    set seen_timeout 0
    while { ($seen_done < 16 || ! $seen_prompt) && ! $seen_timeout } {
        # We don't know what order the interesting things will arrive in.
        # Using a pattern of the form 'x|y|z' instead of -re x ... -re y
        # ... -re z ensures that expect always chooses the match that
        # occurs leftmost in the input, and not the pattern appearing
        # first in the script that occurs anywhere in the input, so that
        # we don't skip anything.
        gdb_expect {
            -re "($decimal done)|(Breakpoint)|($gdb_prompt)" {
                if {[info exists expect_out(1,string)]} {
                    incr seen_done
                } elseif {[info exists expect_out(2,string)]} {
                    set seen_break 1
                } elseif {[info exists expect_out(3,string)]} {
                    set seen_prompt 1
                }
                array unset expect_out
            }
            timeout { set seen_timeout 1 }
        }
    }

Would it make sense to do the same in the case where we follow
the child?

On a side note, I try to avoid delays such as "sleep" like the plague.
A one or two second delay on its own is not that bad, but they tend
to add-up pretty quickly. Since our testsuite is sequential, it's time
wasted doing nothing. So I only use them if I don't have any other choice.

-- 
Joel

Re: [patch] testsuite: Fix a racy FAIL on gdb.base/multi-forks.exp

[Jan Kratochvil]

On Fri, 13 Mar 2009 17:14:23 +0100, Joel Brobecker wrote:
> That being said, I see later in the same testcase that this problem
> is already handled a different way.

I did not notice before, I agree, patch updated.


> On a side note, I try to avoid delays such as "sleep" like the plague.

I also try when possible.


Thanks,
Jan


gdb/testsuite/
2009-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix a racy FAIL.
	* gdb.base/multi-forks.exp (continue_to_exit_bp_loc): New function with
	code from `follow parent, print pids'.
	(`follow child, print pids', `follow parent, print pids'): Call it.
	Replace `gdb_test "break..."' by gdb_breakpoint.

--- gdb/testsuite/gdb.base/multi-forks.exp	3 Jan 2009 05:58:03 -0000	1.15
+++ gdb/testsuite/gdb.base/multi-forks.exp	13 Mar 2009 19:43:22 -0000
@@ -51,7 +51,60 @@ global gdb_prompt
 
 # This is a test of gdb's ability to follow the parent, child or both
 # parent and child of multiple Unix fork() system calls.
-#
+
+set exit_bp_loc [gdb_get_line_number "Set exit breakpoint here."]
+
+# Continue till where $exit_bp_loc points at eating all the inferior output
+# which could otherwise corrupt our later communication with GDB.
+
+proc continue_to_exit_bp_loc {} {
+    global exit_bp_loc decimal gdb_prompt
+
+    gdb_breakpoint $exit_bp_loc
+
+    send_gdb "continue\n"
+
+    # The output from the child processes can be interleaved arbitrarily
+    # with the output from GDB and the parent process.  If we don't
+    # consume it all now, it can confuse later interactions.
+    set seen_done 0
+    set seen_break 0
+    set seen_prompt 0
+    set seen_timeout 0
+    while { ($seen_done < 16 || ! $seen_prompt) && ! $seen_timeout } {
+	# We don't know what order the interesting things will arrive in.
+	# Using a pattern of the form 'x|y|z' instead of -re x ... -re y
+	# ... -re z ensures that expect always chooses the match that
+	# occurs leftmost in the input, and not the pattern appearing
+	# first in the script that occurs anywhere in the input, so that
+	# we don't skip anything.
+	gdb_expect {
+	    -re "($decimal done)|(Breakpoint)|($gdb_prompt)" {
+		if {[info exists expect_out(1,string)]} {
+		    incr seen_done
+		} elseif {[info exists expect_out(2,string)]} {
+		    set seen_break 1
+		} elseif {[info exists expect_out(3,string)]} {
+		    set seen_prompt 1
+		}
+		array unset expect_out
+	    }
+	    timeout { set seen_timeout 1 }
+	}
+    }
+
+    if { $seen_timeout } {
+	fail "run to exit 2 (timeout)"
+    } elseif { ! $seen_prompt } {
+	fail "run to exit 2 (no prompt)"
+    } elseif { ! $seen_break } {
+	fail "run to exit 2 (no breakpoint hit)"
+    } elseif { $seen_done != 16 } {
+	fail "run to exit 2 (missing done messages)"
+    } else {
+	pass "run to exit 2"
+    }
+}
 
 # The inferior program builds a tree of processes by executing a loop
 # four times, calling fork at each iteration.  Thus, at each
@@ -66,69 +119,17 @@ global gdb_prompt
 # The result should be that each of the 4 forks returns zero.
 
 runto_main
-set exit_bp_loc [gdb_get_line_number "Set exit breakpoint here."]
-gdb_test "break $exit_bp_loc" "Breakpoint.* at .*" "Break at exit"
-gdb_test "set follow child" "" ""
-
-send_gdb "continue\n"
-gdb_expect {
-    -re ".*Break.* main .*$gdb_prompt.*$" {}
-    -re ".*$gdb_prompt $" {fail "run to exit 1"}
-    default {fail "run to exit 1 (timeout)"}
-}
+gdb_test "set follow child"
+continue_to_exit_bp_loc
 
 gdb_test "print pids" "\\$.* = \\{0, 0, 0, 0\\}.*" "follow child, print pids"
 
 # Now set gdb to follow the parent.
 # Result should be that none of the 4 forks returns zero.
 
-delete_breakpoints
 runto_main
-gdb_test "break $exit_bp_loc" "Breakpoint.* at .*" "Break at exit"
 gdb_test "set follow parent" "" ""
-
-send_gdb "continue\n"
-
-# The output from the child processes can be interleaved arbitrarily
-# with the output from GDB and the parent process.  If we don't
-# consume it all now, it can confuse later interactions.
-set seen_done 0
-set seen_break 0
-set seen_prompt 0
-set seen_timeout 0
-while { ($seen_done < 16 || ! $seen_prompt) && ! $seen_timeout } {
-    # We don't know what order the interesting things will arrive in.
-    # Using a pattern of the form 'x|y|z' instead of -re x ... -re y
-    # ... -re z ensures that expect always chooses the match that
-    # occurs leftmost in the input, and not the pattern appearing
-    # first in the script that occurs anywhere in the input, so that
-    # we don't skip anything.
-    gdb_expect {
-        -re "($decimal done)|(Breakpoint)|($gdb_prompt)" {
-            if {[info exists expect_out(1,string)]} {
-                incr seen_done
-            } elseif {[info exists expect_out(2,string)]} {
-                set seen_break 1
-            } elseif {[info exists expect_out(3,string)]} {
-                set seen_prompt 1
-            }
-            array unset expect_out
-        }
-        timeout { set seen_timeout 1 }
-    }
-}
-
-if { $seen_timeout } {
-    fail "run to exit 2 (timeout)"
-} elseif { ! $seen_prompt } {
-    fail "run to exit 2 (no prompt)"
-} elseif { ! $seen_break } {
-    fail "run to exit 2 (no breakpoint hit)"
-} elseif { $seen_done != 16 } {
-    fail "run to exit 2 (missing done messages)"
-} else {
-    pass "run to exit 2"
-}
+continue_to_exit_bp_loc
 
 gdb_test "print pids\[0\]==0 || pids\[1\]==0 || pids\[2\]==0 || pids\[3\]==0" \
     " = 0" "follow parent, print pids"
@@ -138,7 +139,7 @@ gdb_test "print pids\[0\]==0 || pids\[1\
 #
 
 runto_main
-gdb_test "break $exit_bp_loc" "Breakpoint.* at .*" ""
+gdb_breakpoint $exit_bp_loc
 
 gdb_test "help set detach-on-fork" "whether gdb will detach the child.*" \
     "help set detach"

Re: [patch] testsuite: Fix a racy FAIL on gdb.base/multi-forks.exp

[Joel Brobecker]

> gdb/testsuite/
> 2009-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix a racy FAIL.
> 	* gdb.base/multi-forks.exp (continue_to_exit_bp_loc): New function with
> 	code from `follow parent, print pids'.
> 	(`follow child, print pids', `follow parent, print pids'): Call it.
> 	Replace `gdb_test "break..."' by gdb_breakpoint.

This seems pretty reasonable to me...

> +# Continue till where $exit_bp_loc points at eating all the inferior output
> +# which could otherwise corrupt our later communication with GDB.

I am so sorry to be picking on your comments. I very much appreciate
the fact that you spend the time writing them, especially since most
people tend to not write comments. In this case, this comments also
sounds strange. Here is a suggestion:

# Insert a breakpoint at the location provided by the exit_bp_loc global
# and resume the execution until hitting that breakpoint.  We also make
# sure to consume all the expected output from all processes as well,
# to make sure it doesn't cause trouble during a subsequent test.

-- 
Joel

Re: [patch] testsuite: Fix a racy FAIL on gdb.base/multi-forks.exp

[Jan Kratochvil]

On Sat, 14 Mar 2009 00:00:33 +0100, Joel Brobecker wrote:
> > gdb/testsuite/
> > 2009-03-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Fix a racy FAIL.
> > 	* gdb.base/multi-forks.exp (continue_to_exit_bp_loc): New function with
> > 	code from `follow parent, print pids'.
> > 	(`follow child, print pids', `follow parent, print pids'): Call it.
> > 	Replace `gdb_test "break..."' by gdb_breakpoint.
> 
> This seems pretty reasonable to me...

Checked-in: http://sourceware.org/ml/gdb-cvs/2009-03/msg00090.html


> I am so sorry to be picking on your comments. I very much appreciate
> the fact that you spend the time writing them, especially since most
> people tend to not write comments. In this case, this comments also
> sounds strange. Here is a suggestion:

Used your text.  I will give the same effort on the comments as on user
visible texts.


Thanks,
Jan

Re: [patch] testsuite: Fix a racy FAIL on gdb.base/multi-forks.exp

[Eli Zaretskii]

> Date: Fri, 13 Mar 2009 09:14:23 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > Instead of the output suppression by `close (1)' some sleep would also fix it
> > if close (1) is not compatible enough.
> 
> As far as I can tell, "close" is POSIX, so should be fairly portable.
> I'm not entirely sure about Windows, but I do think it's available.
> The symbol name might be _close, though.

Windows has both _close and close.

What happens after you close(1) in a Windows console application is
anybody's guess, though.  I didn't try that.