[PATCH][gdb/testsuite] Fix target_supports_scheduler_locking raciness

[PATCH][gdb/testsuite] Fix target_supports_scheduler_locking raciness

[Tom de Vries]

Hi,

When calling gdb_start_cmd, it's the caller's responsibility to wait for gdb
to return to the prompt.  In target_supports_scheduler_locking, that's not the
case, and consequently, target_supports_scheduler_locking fails spuriously.

Fix by using runto_main instead.

Build and reg-tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite] Fix target_supports_scheduler_locking raciness

2018-10-04  Tom de Vries  <tdevries@suse.de>

	* gdb.base/gdb-caching-proc.exp: New file.
	* lib/gdb.exp (target_supports_scheduler_locking): Replace gdb_start_cmd
	with runto_main.

---
 gdb/testsuite/gdb.base/gdb-caching-proc.exp | 111 ++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                   |   4 +-
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/gdb-caching-proc.exp b/gdb/testsuite/gdb.base/gdb-caching-proc.exp
new file mode 100644
index 0000000000..87c20fc574
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-caching-proc.exp
@@ -0,0 +1,111 @@
+#   Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# When caching a proc using gdb_caching_proc, it will become less likely to
+# be executed, and consequently it's going to be harder to detect that the
+# proc is racy.  OTOH, in general the proc is easy to rerun.  So, run all
+# uncached gdb_caching_procs a number of times and detect inconsistent results.
+# The purpose of caching is to reduce runtime, so rerunning is somewhat
+# counter-productive in that aspect, but it's better than uncached, because the
+# number of reruns is constant-bounded, and the increase in runtime is bound to
+# this test-case, and could be disabled on slow targets.
+
+# Test gdb_caching_proc NAME
+proc test_proc { name } {
+    set real_name gdb_real__$name
+
+    set first [$real_name]
+    lappend resultlist $first
+
+    # Ten repetitions was enough to trigger target_supports_scheduler_locking,
+    # and costs about 20 seconds on an i7-6600U.
+    set repeat 10
+
+    set resultlist [list]
+    set racy 0
+    for {set i 0} {$i < $repeat} {incr i} {
+	set rerun [$real_name]
+	lappend resultlist $rerun
+	if { $rerun != $first } {
+	    set racy 1
+	}
+    }
+
+    if { $racy  == 0 } {
+	pass "$name consistency"
+    } else {
+	fail "$name consistency"
+	verbose -log "$name: $resultlist"
+    }
+}
+
+# Test gdb_caching_procs in FILE
+proc test_file { file } {
+    upvar obj obj
+    set procnames [list]
+
+    set fp [open $file]
+    while { [gets $fp line] >= 0 } {
+	if [regexp -- "^gdb_caching_proc \[ \t\]*(\[^ \t\]*)" $line \
+		match procname] {
+	    lappend procnames $procname
+	}
+    }
+    close $fp
+
+    if { [llength $procnames] == 0 } {
+	return
+    }
+
+    if { [file tail $file] == "gdb.exp" } {
+	# Already loaded
+    } else {
+	load_lib [file tail $file]
+    }
+
+    foreach procname $procnames {
+	switch $procname {
+	    "is_address_zero_readable" { set setup_gdb 1 }
+	    "target_is_gdbserver" { set setup_gdb 1 }
+	    default {set setup_gdb 0 }
+	}
+
+	if { $setup_gdb } {
+	    clean_restart $obj
+	}
+
+	test_proc $procname
+
+	if { $setup_gdb } {
+	    gdb_exit
+	}
+    }
+}
+
+# Init
+set me "gdb_caching_proc"
+set src { int main() { return 0; } }
+if { ![gdb_simple_compile $me $src executable] } {
+    return 0
+}
+
+# Test gdb_caching_procs in gdb/testsuite/lib/*.exp
+set files [eval glob -types f $srcdir/lib/*.exp]
+foreach file $files {
+    test_file $file
+}
+
+# Cleanup
+remote_file build delete $obj
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index fbdb436e33..83b83eb23d 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5971,7 +5971,9 @@ gdb_caching_proc target_supports_scheduler_locking {
     }
 
     clean_restart $obj
-    gdb_start_cmd
+    if ![runto_main] {
+        return 0
+    }
 
     set supports_schedule_locking -1
     set current_schedule_locking_mode ""

Re: [PATCH][gdb/testsuite] Fix target_supports_scheduler_locking raciness

[Pedro Alves]

On 10/04/2018 06:40 PM, Tom de Vries wrote:
> Hi,
> 
> When calling gdb_start_cmd, it's the caller's responsibility to wait for gdb
> to return to the prompt.  In target_supports_scheduler_locking, that's not the
> case, and consequently, target_supports_scheduler_locking fails spuriously.
> 
> Fix by using runto_main instead.
> 
> Build and reg-tested on x86_64-linux.
> 
> OK for trunk?

The fix itself it OK.  Thanks for doing that.  Can you please
submit the gdb-caching-proc.exp separately, with its own rationale?

Thanks,
Pedro Alves

[gdb/testsuite] Add gdb-caching-proc.exp testcase

[Tom de Vries]

On Tue, Oct 09, 2018 at 01:58:26PM +0100, Pedro Alves wrote:
> Can you please
> submit the gdb-caching-proc.exp separately, with its own rationale?
> 

Hi,

When caching a proc using gdb_caching_proc, it will become less likely to
be executed, and consequently it's going to be harder to detect that the
proc is racy.  OTOH, in general the proc is easy to rerun.  So, add a
test-case to run all uncached gdb_caching_procs a number of times and detect
inconsistent results.

The purpose of caching is to reduce runtime, so rerunning is somewhat
counter-productive in that aspect, but it's better than uncached, because the
number of reruns is constant-bounded, and the increase in runtime is bound to
this test-case, and can be disabled on slow targets.

Tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite] Add gdb-caching-proc.exp testcase

2018-10-09  Tom de Vries  <tdevries@suse.de>

	* gdb.base/gdb-caching-proc.exp: New file.

---
 gdb/testsuite/gdb.base/gdb-caching-proc.exp | 111 ++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/gdb/testsuite/gdb.base/gdb-caching-proc.exp b/gdb/testsuite/gdb.base/gdb-caching-proc.exp
new file mode 100644
index 0000000000..87c20fc574
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-caching-proc.exp
@@ -0,0 +1,111 @@
+#   Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# When caching a proc using gdb_caching_proc, it will become less likely to
+# be executed, and consequently it's going to be harder to detect that the
+# proc is racy.  OTOH, in general the proc is easy to rerun.  So, run all
+# uncached gdb_caching_procs a number of times and detect inconsistent results.
+# The purpose of caching is to reduce runtime, so rerunning is somewhat
+# counter-productive in that aspect, but it's better than uncached, because the
+# number of reruns is constant-bounded, and the increase in runtime is bound to
+# this test-case, and could be disabled on slow targets.
+
+# Test gdb_caching_proc NAME
+proc test_proc { name } {
+    set real_name gdb_real__$name
+
+    set first [$real_name]
+    lappend resultlist $first
+
+    # Ten repetitions was enough to trigger target_supports_scheduler_locking,
+    # and costs about 20 seconds on an i7-6600U.
+    set repeat 10
+
+    set resultlist [list]
+    set racy 0
+    for {set i 0} {$i < $repeat} {incr i} {
+	set rerun [$real_name]
+	lappend resultlist $rerun
+	if { $rerun != $first } {
+	    set racy 1
+	}
+    }
+
+    if { $racy  == 0 } {
+	pass "$name consistency"
+    } else {
+	fail "$name consistency"
+	verbose -log "$name: $resultlist"
+    }
+}
+
+# Test gdb_caching_procs in FILE
+proc test_file { file } {
+    upvar obj obj
+    set procnames [list]
+
+    set fp [open $file]
+    while { [gets $fp line] >= 0 } {
+	if [regexp -- "^gdb_caching_proc \[ \t\]*(\[^ \t\]*)" $line \
+		match procname] {
+	    lappend procnames $procname
+	}
+    }
+    close $fp
+
+    if { [llength $procnames] == 0 } {
+	return
+    }
+
+    if { [file tail $file] == "gdb.exp" } {
+	# Already loaded
+    } else {
+	load_lib [file tail $file]
+    }
+
+    foreach procname $procnames {
+	switch $procname {
+	    "is_address_zero_readable" { set setup_gdb 1 }
+	    "target_is_gdbserver" { set setup_gdb 1 }
+	    default {set setup_gdb 0 }
+	}
+
+	if { $setup_gdb } {
+	    clean_restart $obj
+	}
+
+	test_proc $procname
+
+	if { $setup_gdb } {
+	    gdb_exit
+	}
+    }
+}
+
+# Init
+set me "gdb_caching_proc"
+set src { int main() { return 0; } }
+if { ![gdb_simple_compile $me $src executable] } {
+    return 0
+}
+
+# Test gdb_caching_procs in gdb/testsuite/lib/*.exp
+set files [eval glob -types f $srcdir/lib/*.exp]
+foreach file $files {
+    test_file $file
+}
+
+# Cleanup
+remote_file build delete $obj

[PING][gdb/testsuite] Add gdb-caching-proc.exp testcase

[Tom de Vries]

On 10/9/18 3:47 PM, Tom de Vries wrote:
> On Tue, Oct 09, 2018 at 01:58:26PM +0100, Pedro Alves wrote:
>> Can you please
>> submit the gdb-caching-proc.exp separately, with its own rationale?
>>
> 
> Hi,
> 
> When caching a proc using gdb_caching_proc, it will become less likely to
> be executed, and consequently it's going to be harder to detect that the
> proc is racy.  OTOH, in general the proc is easy to rerun.  So, add a
> test-case to run all uncached gdb_caching_procs a number of times and detect
> inconsistent results.
> 
> The purpose of caching is to reduce runtime, so rerunning is somewhat
> counter-productive in that aspect, but it's better than uncached, because the
> number of reruns is constant-bounded, and the increase in runtime is bound to
> this test-case, and can be disabled on slow targets.
> 
> Tested on x86_64-linux.
> 
> OK for trunk?
> 

Ping.

Thanks,
- Tom

> [gdb/testsuite] Add gdb-caching-proc.exp testcase
> 
> 2018-10-09  Tom de Vries  <tdevries@suse.de>
> 
> 	* gdb.base/gdb-caching-proc.exp: New file.
> 
> ---
>  gdb/testsuite/gdb.base/gdb-caching-proc.exp | 111 ++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.base/gdb-caching-proc.exp b/gdb/testsuite/gdb.base/gdb-caching-proc.exp
> new file mode 100644
> index 0000000000..87c20fc574
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gdb-caching-proc.exp
> @@ -0,0 +1,111 @@
> +#   Copyright 2018 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# When caching a proc using gdb_caching_proc, it will become less likely to
> +# be executed, and consequently it's going to be harder to detect that the
> +# proc is racy.  OTOH, in general the proc is easy to rerun.  So, run all
> +# uncached gdb_caching_procs a number of times and detect inconsistent results.
> +# The purpose of caching is to reduce runtime, so rerunning is somewhat
> +# counter-productive in that aspect, but it's better than uncached, because the
> +# number of reruns is constant-bounded, and the increase in runtime is bound to
> +# this test-case, and could be disabled on slow targets.
> +
> +# Test gdb_caching_proc NAME
> +proc test_proc { name } {
> +    set real_name gdb_real__$name
> +
> +    set first [$real_name]
> +    lappend resultlist $first
> +
> +    # Ten repetitions was enough to trigger target_supports_scheduler_locking,
> +    # and costs about 20 seconds on an i7-6600U.
> +    set repeat 10
> +
> +    set resultlist [list]
> +    set racy 0
> +    for {set i 0} {$i < $repeat} {incr i} {
> +	set rerun [$real_name]
> +	lappend resultlist $rerun
> +	if { $rerun != $first } {
> +	    set racy 1
> +	}
> +    }
> +
> +    if { $racy  == 0 } {
> +	pass "$name consistency"
> +    } else {
> +	fail "$name consistency"
> +	verbose -log "$name: $resultlist"
> +    }
> +}
> +
> +# Test gdb_caching_procs in FILE
> +proc test_file { file } {
> +    upvar obj obj
> +    set procnames [list]
> +
> +    set fp [open $file]
> +    while { [gets $fp line] >= 0 } {
> +	if [regexp -- "^gdb_caching_proc \[ \t\]*(\[^ \t\]*)" $line \
> +		match procname] {
> +	    lappend procnames $procname
> +	}
> +    }
> +    close $fp
> +
> +    if { [llength $procnames] == 0 } {
> +	return
> +    }
> +
> +    if { [file tail $file] == "gdb.exp" } {
> +	# Already loaded
> +    } else {
> +	load_lib [file tail $file]
> +    }
> +
> +    foreach procname $procnames {
> +	switch $procname {
> +	    "is_address_zero_readable" { set setup_gdb 1 }
> +	    "target_is_gdbserver" { set setup_gdb 1 }
> +	    default {set setup_gdb 0 }
> +	}
> +
> +	if { $setup_gdb } {
> +	    clean_restart $obj
> +	}
> +
> +	test_proc $procname
> +
> +	if { $setup_gdb } {
> +	    gdb_exit
> +	}
> +    }
> +}
> +
> +# Init
> +set me "gdb_caching_proc"
> +set src { int main() { return 0; } }
> +if { ![gdb_simple_compile $me $src executable] } {
> +    return 0
> +}
> +
> +# Test gdb_caching_procs in gdb/testsuite/lib/*.exp
> +set files [eval glob -types f $srcdir/lib/*.exp]
> +foreach file $files {
> +    test_file $file
> +}
> +
> +# Cleanup
> +remote_file build delete $obj
> 

Re: [gdb/testsuite] Add gdb-caching-proc.exp testcase

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> 2018-10-09  Tom de Vries  <tdevries@suse.de>
Tom> 	* gdb.base/gdb-caching-proc.exp: New file.

Thanks for the patch.  I didn't see a review or see this go in, so
hopefully this isn't redundant.

Tom> +# Test gdb_caching_proc NAME
Tom> +proc test_proc { name } {
Tom> +    set real_name gdb_real__$name
Tom> +
Tom> +    set first [$real_name]
Tom> +    lappend resultlist $first

I think this lappend is unnecessary...

Tom> +    set resultlist [list]

... because resultlist is reset here.
But maybe you intended to hoist this earlier?
Either way is fine by me.

This is ok with this nit addressed.

Tom

Re: [gdb/testsuite] Add gdb-caching-proc.exp testcase

[Tom de Vries]

On 30-11-18 20:49, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> 2018-10-09  Tom de Vries  <tdevries@suse.de>
> Tom> 	* gdb.base/gdb-caching-proc.exp: New file.
> 
> Thanks for the patch.  I didn't see a review or see this go in, so
> hopefully this isn't redundant.
> 

Hi Tom,

indeed this was not reviewed yet.

> Tom> +# Test gdb_caching_proc NAME
> Tom> +proc test_proc { name } {
> Tom> +    set real_name gdb_real__$name
> Tom> +
> Tom> +    set first [$real_name]
> Tom> +    lappend resultlist $first
> 
> I think this lappend is unnecessary...
> 
> Tom> +    set resultlist [list]
> 
> ... because resultlist is reset here.
> But maybe you intended to hoist this earlier?

Yep, I did, fixed in the commit.

> Either way is fine by me.
> 
> This is ok with this nit addressed.

Thanks for the review.

- Tom