Re: Weird behaviour with --target_board="unix{var1,var2}"

Re: Weird behaviour with --target_board="unix{var1,var2}"

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]

On 23/08/16 09:07 +0100, Jonathan Wakely wrote:
>On 22/08/16 21:16 +0100, Pedro Alves wrote:
>>On 08/22/2016 03:40 PM, Jonathan Wakely wrote:
>>
>>>What's going on?!
>>>
>>>Have I fundamentally misunderstood something about how RUNTESTFLAGS or
>>>effective-target keywords work?
>>>
>>
>>Here's a wild guess.
>>
>>In gdb's testsuite, I've seen odd problems like these being caused by some
>>tcl global getting set on the first run using one board, then then the
>>second run using the second board finds the variable already set and
>>skips something.  E.g.:
>>
>> https://sourceware.org/ml/gdb-patches/2015-04/msg00261.html
>>
>>I'd look at the code that decides whether to run a test in a particular
>>mode with the above in mind.  Given that reordering the boards makes
>>a difference, it kind of sounds like some "last checked mode" or some
>>such variable is bleeding between runs.
>
>I've tracked it down to this block of code in
>/usr/share/dejagnu/dg.exp:
>
>   # Don't do this if we're testing an interpreter.
>   # FIXME: why?
>   if { ${dg-interpreter-batch-mode} == 0 } {
>	# Catch excess errors (new bugs or incomplete testcases).
>	if {${dg-excess-errors-flag}} {
>	    setup_xfail "*-*-*"
>	}
>	if {![string match "" $comp_output]} {
>	    fail "$name (test for excess errors)"
>	    send_log "Excess errors:\n$comp_output\n"
>	} else {
>	    pass "$name (test for excess errors)"
>	}
>   }
>
>Adding some logging to the libstdc++-dg-test callback shows that
>${dg-interpreter-batch-mode}  == 1 for the tests which don't print
>their result.
>
>That's being set by prettyprinters.exp and xmethods.exp (so it's GDB's
>fault! ;-) because they want to keep the executables around, to run
>GDB on. When we run with multiple boards that variable doesn't get
>reset to 0 for the next run of conformance.exp
>
>That also explains why putting RUNTESTFLAGS="conformance.exp..." fixes
>it, because the other .exp files aren't used, so batch mode is never
>set.
>
>Shouldn't be too hard to fix.

This seems to work. I'll do some more testing and commit later today.




[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2030 bytes --]

commit 5bdadc8628977b3a462876efd0e7919b1ec8f5a7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Aug 23 09:09:18 2016 +0100

    Restore dg-interpreter-batch-mode for libstdc++ tests
    
    	* testsuite/libstdc++-prettyprinters/prettyprinters.exp: Restore
    	original value of dg-interpreter-batch-mode.
    	* testsuite/libstdc++-xmethods/xmethods.exp: Likewise.

diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp b/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp
index 0c62b5e..dcc99fb 100644
--- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp
+++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp
@@ -44,6 +44,7 @@ if {! [gdb_version_check]} {
 # This can be used to keep the .exe around.  dg-test has an option for
 # this but there is no way to pass it through dg-runtest.
 global dg-interpreter-batch-mode
+set saved-dg-interpreter-batch-mode ${dg-interpreter-batch-mode}
 set dg-interpreter-batch-mode 1
 
 global DEFAULT_CXXFLAGS
@@ -55,4 +56,6 @@ if [info exists guality_gdb_name] {
     unsetenv GUALITY_GDB_NAME
 }
 
+set dg-interpreter-batch-mode ${saved-dg-interpreter-batch-mode}
+
 dg-finish
diff --git a/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp b/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp
index ee9b31a..5809d74 100644
--- a/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp
+++ b/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp
@@ -45,6 +45,7 @@ if {! [gdb_version_check_xmethods]} {
 # This can be used to keep the .exe around.  dg-test has an option for
 # this but there is no way to pass it through dg-runtest.
 global dg-interpreter-batch-mode
+set saved-dg-interpreter-batch-mode ${dg-interpreter-batch-mode}
 set dg-interpreter-batch-mode 1
 
 global DEFAULT_CXXFLAGS
@@ -56,5 +57,7 @@ if [info exists guality_gdb_name] {
     unsetenv GUALITY_GDB_NAME
 }
 
+set dg-interpreter-batch-mode ${saved-dg-interpreter-batch-mode}
+
 dg-finish
 gcc_parallel_test_enable 1

Re: Weird behaviour with --target_board="unix{var1,var2}"

[Pedro Alves]

On 08/23/2016 10:54 AM, Jonathan Wakely wrote:

>> That's being set by prettyprinters.exp and xmethods.exp (so it's GDB's
>> fault! ;-) 

:-)

> This seems to work. I'll do some more testing and commit later today.

LGTM.

Though IME, save/restoring globals in a constant source of trouble,
for occasionally someone adds an early return that inadvertently
skips the restore.  Of course in gdb every test must be written
using custom .exp code, so we're more prone to being bitten.  Still...

The way we solve that on gdb systematically is with convenience
wrappers that handle the save/restore.  E.g. see:

 $ grep "proc with_" *
 gdb.exp:proc with_test_prefix { prefix body } {
 gdb.exp:proc with_gdb_prompt { prompt body } {
 gdb.exp:proc with_target_charset { target_charset body } {
 gdb.exp:proc with_spawn_id { spawn_id body } {
 gdb.exp:proc with_timeout_factor { factor body } {

In this particular case, I'd add a wrapper method to
libstdc++-v3/testsuite/lib/gdb-test.exp, such as:

# Like dg-runtest but keep the .exe around.  dg-test has an option for
# this but there is no way to pass it through dg-runtest.

proc gdb-dg-runtest {args} {
  global dg-interpreter-batch-mode
  set saved-dg-interpreter-batch-mode ${dg-interpreter-batch-mode}  
  set dg-interpreter-batch-mode 1

  eval dg-runtest $args

  set dg-interpreter-batch-mode ${saved-dg-interpreter-batch-mode}
}

And then use gdb-dg-runtest instead of dg-runtest in 
prettyprinters.exp and xmethods.exp.

(Maybe put even move more of the duplicate code around the
current dg-runtest calls to the wrapper, and then give the
wrapper named arguments.)

Re: Weird behaviour with --target_board="unix{var1,var2}"

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

On 23/08/16 12:05 +0100, Pedro Alves wrote:
>On 08/23/2016 10:54 AM, Jonathan Wakely wrote:
>
>>> That's being set by prettyprinters.exp and xmethods.exp (so it's GDB's
>>> fault! ;-)
>
>:-)
>
>> This seems to work. I'll do some more testing and commit later today.
>
>LGTM.
>
>Though IME, save/restoring globals in a constant source of trouble,
>for occasionally someone adds an early return that inadvertently
>skips the restore.  Of course in gdb every test must be written
>using custom .exp code, so we're more prone to being bitten.  Still...
>
>The way we solve that on gdb systematically is with convenience
>wrappers that handle the save/restore.  E.g. see:
>
> $ grep "proc with_" *
> gdb.exp:proc with_test_prefix { prefix body } {
> gdb.exp:proc with_gdb_prompt { prompt body } {
> gdb.exp:proc with_target_charset { target_charset body } {
> gdb.exp:proc with_spawn_id { spawn_id body } {
> gdb.exp:proc with_timeout_factor { factor body } {
>
>In this particular case, I'd add a wrapper method to
>libstdc++-v3/testsuite/lib/gdb-test.exp, such as:
>
># Like dg-runtest but keep the .exe around.  dg-test has an option for
># this but there is no way to pass it through dg-runtest.
>
>proc gdb-dg-runtest {args} {
>  global dg-interpreter-batch-mode
>  set saved-dg-interpreter-batch-mode ${dg-interpreter-batch-mode}
>  set dg-interpreter-batch-mode 1
>
>  eval dg-runtest $args
>
>  set dg-interpreter-batch-mode ${saved-dg-interpreter-batch-mode}
>}
>
>And then use gdb-dg-runtest instead of dg-runtest in
>prettyprinters.exp and xmethods.exp.

That works nicely.

>(Maybe put even move more of the duplicate code around the
>current dg-runtest calls to the wrapper, and then give the
>wrapper named arguments.)

That didn't seem to make things simpler.

This is what I plan to commit.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2976 bytes --]

commit a487dae3a870d0c238bef06937d63d0136a13465
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Aug 23 09:09:18 2016 +0100

    Restore dg-interpreter-batch-mode for libstdc++ tests
    
    2016-08-24  Jonathan Wakely  <jwakely@redhat.com>
    	    Pedro Alves  <palves@redhat.com>
    
    	* testsuite/lib/gdb-test.exp (gdb-dg-runtest): Define wrapper to save
    	and restore dg-interpreter-batch-mode.
    	* testsuite/libstdc++-prettyprinters/prettyprinters.exp: Use
    	gdb-dg-runtest instead of dg-runtest.
    	* testsuite/libstdc++-xmethods/xmethods.exp: Likewise.

diff --git a/libstdc++-v3/testsuite/lib/gdb-test.exp b/libstdc++-v3/testsuite/lib/gdb-test.exp
index 5570009..a029883 100644
--- a/libstdc++-v3/testsuite/lib/gdb-test.exp
+++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
@@ -277,3 +277,15 @@ proc gdb_version_check_xmethods {} {
 	      "python import gdb.xmethod; print(gdb.xmethod.XMethod)" \
 	      "<class 'gdb\\.xmethod\\.XMethod'>"]
 }
+
+# Like dg-runtest but keep the .exe around.  dg-test has an option for
+# this but there is no way to pass it through dg-runtest.
+proc gdb-dg-runtest {args} {
+  global dg-interpreter-batch-mode
+  set saved-dg-interpreter-batch-mode ${dg-interpreter-batch-mode}
+  set dg-interpreter-batch-mode 1
+
+  eval dg-runtest $args
+
+  set dg-interpreter-batch-mode ${saved-dg-interpreter-batch-mode}
+}
diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp b/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp
index 0c62b5e..cc003cd 100644
--- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp
+++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp
@@ -41,14 +41,9 @@ if {! [gdb_version_check]} {
     return
 }
 
-# This can be used to keep the .exe around.  dg-test has an option for
-# this but there is no way to pass it through dg-runtest.
-global dg-interpreter-batch-mode
-set dg-interpreter-batch-mode 1
-
 global DEFAULT_CXXFLAGS
 global PCH_CXXFLAGS
-dg-runtest [lsort [glob $srcdir/$subdir/*.cc]] \
+gdb-dg-runtest [lsort [glob $srcdir/$subdir/*.cc]] \
   "" "$DEFAULT_CXXFLAGS $PCH_CXXFLAGS"
 
 if [info exists guality_gdb_name] {
diff --git a/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp b/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp
index ee9b31a..e580d73 100644
--- a/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp
+++ b/libstdc++-v3/testsuite/libstdc++-xmethods/xmethods.exp
@@ -42,14 +42,9 @@ if {! [gdb_version_check_xmethods]} {
     return
 }
 
-# This can be used to keep the .exe around.  dg-test has an option for
-# this but there is no way to pass it through dg-runtest.
-global dg-interpreter-batch-mode
-set dg-interpreter-batch-mode 1
-
 global DEFAULT_CXXFLAGS
 global PCH_CXXFLAGS
-dg-runtest [lsort [glob $srcdir/$subdir/*.cc]] \
+gdb-dg-runtest [lsort [glob $srcdir/$subdir/*.cc]] \
   "" "$DEFAULT_CXXFLAGS $PCH_CXXFLAGS"
 
 if [info exists guality_gdb_name] {