diff mbox

libffi testsuite: Use split to ensure valid tcl list

Message ID 87wppswqqt.fsf@kepler.schwinge.homeip.net
State New
Headers show

Commit Message

Thomas Schwinge Feb. 25, 2016, 7:10 p.m. UTC
Hi!

Already had noticed something odd here months ago; now finally looked
into it...

On Sat, 28 Mar 2015 13:59:30 -0400, John David Anglin <dave.anglin@bell.net> wrote:
> The attached change fixes tcl errors that occur running the complex.exp and go.exp test sets.
> See: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65567>.
> 
> Tested on hppa2.0w-hp-hpux11.11.  Okay for trunk?

(Got approved, and installed as r221765.)

> 2015-03-28  John David Anglin  <danglin@gcc.gnu.org>
> 
> 	PR libffi/65567
> 	* testsuite/lib/libffi.exp (libffi_feature_test): Use split to ensure
> 	lindex is applied to a list.
> 
> Index: testsuite/lib/libffi.exp
> ===================================================================
> --- testsuite/lib/libffi.exp	(revision 221591)
> +++ testsuite/lib/libffi.exp	(working copy)
> @@ -238,7 +239,7 @@
>      set lines [libffi_target_compile $src "" "preprocess" ""]
>      file delete $src
>  
> -    set last [lindex $lines end]
> +    set last [lindex [split $lines] end]
>      return [regexp -- "xyzzy" $last]
>  }

On my several systems, this has the effect that any user of
libffi_feature_test has their test results regress from PASS to
UNSUPPORTED.  Apparently the regexp xyzzy matching doesn't work as
intended.  If I revert your patch, it's OK for me -- but still not for
you, I suppose.  ;-)

How about the followinginstead?  It's conceptually simpler (and similar
to what other such tests are doing), works for me -- but can you also
please test this?



Grüße
 Thomas

Comments

Mike Stump Feb. 25, 2016, 7:45 p.m. UTC | #1
On Feb 25, 2016, at 11:10 AM, Thomas Schwinge <thomas@codesourcery.com> wrote:
> +    set lines [libffi_target_compile $src /dev/null assembly “"]

Does this work on a dos box, or windows or other random non-posix systems?
Thomas Schwinge Feb. 25, 2016, 8:15 p.m. UTC | #2
Hi!

On Thu, 25 Feb 2016 11:45:06 -0800, Mike Stump <mikestump@comcast.net> wrote:
> On Feb 25, 2016, at 11:10 AM, Thomas Schwinge <thomas@codesourcery.com> wrote:
> > +    set lines [libffi_target_compile $src /dev/null assembly “"]
> 
> Does this work on a dos box, or windows or other random non-posix systems?

I don't know, and can't easily test.  However, I had seen the same
pattern be used elsewhere, for example:

    $ git grep dev/null -- libstdc++-v3/testsuite/lib/
    libstdc++-v3/testsuite/lib/libstdc++.exp:       set lines [v3_target_compile $src /dev/null executable ""]
    [several more]

..., so assumed that to be OK.

Now that I'm reading [dejagnu]/target.exp:default_target_compile, I see
that for "[is_remote host]", it *always* passes "-o a.out" and then
"remote_upload host a.out $destfile", so "/dev/null" never escapes from
the system that DejaGnu/runtest is running on, which is always a POSIX
system as far as I know.

Otherwise, we could easily switch all these to a temporary output file,
which is then deleted right away...


Grüße
 Thomas
Mike Stump March 3, 2016, 10:59 p.m. UTC | #3
On Feb 25, 2016, at 12:15 PM, Thomas Schwinge <thomas@codesourcery.com> wrote:
> On Thu, 25 Feb 2016 11:45:06 -0800, Mike Stump <mikestump@comcast.net> wrote:
>> On Feb 25, 2016, at 11:10 AM, Thomas Schwinge <thomas@codesourcery.com> wrote:
>>> +    set lines [libffi_target_compile $src /dev/null assembly “"]
>> 
>> Does this work on a dos box, or windows or other random non-posix systems?
> 
> I don't know, and can't easily test.  However, I had seen the same
> pattern be used elsewhere, for example:
> 
>    $ git grep dev/null -- libstdc++-v3/testsuite/lib/
>    libstdc++-v3/testsuite/lib/libstdc++.exp:       set lines [v3_target_compile $src /dev/null executable ""]
>    [several more]

Ah, I would not worry about it then.
Thomas Schwinge April 21, 2016, 12:21 p.m. UTC | #4
Hi!

Jakub, ping for gcc-6-branch (or, are you seeing useful libffi testing
results there?); I understand Mike's emails to mean that he's approved
the patch for trunk, so I'll commit it there, soon.

I also filed this as <https://github.com/atgreen/libffi/pull/237>; no
reaction so far.

On Thu, 25 Feb 2016 20:10:18 +0100, I wrote:
> Already had noticed something odd here months ago; now finally looked
> into it...
> 
> On Sat, 28 Mar 2015 13:59:30 -0400, John David Anglin <dave.anglin@bell.net> wrote:
> > The attached change fixes tcl errors that occur running the complex.exp and go.exp test sets.
> > See: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65567>.
> > 
> > Tested on hppa2.0w-hp-hpux11.11.  Okay for trunk?
> 
> (Got approved, and installed as r221765.)
> 
> > 2015-03-28  John David Anglin  <danglin@gcc.gnu.org>
> > 
> > 	PR libffi/65567
> > 	* testsuite/lib/libffi.exp (libffi_feature_test): Use split to ensure
> > 	lindex is applied to a list.
> > 
> > Index: testsuite/lib/libffi.exp
> > ===================================================================
> > --- testsuite/lib/libffi.exp	(revision 221591)
> > +++ testsuite/lib/libffi.exp	(working copy)
> > @@ -238,7 +239,7 @@
> >      set lines [libffi_target_compile $src "" "preprocess" ""]
> >      file delete $src
> >  
> > -    set last [lindex $lines end]
> > +    set last [lindex [split $lines] end]
> >      return [regexp -- "xyzzy" $last]
> >  }
> 
> On my several systems, this has the effect that any user of
> libffi_feature_test has their test results regress from PASS to
> UNSUPPORTED.  Apparently the regexp xyzzy matching doesn't work as
> intended.  If I revert your patch, it's OK for me -- but still not for
> you, I suppose.  ;-)
> 
> How about the followinginstead?  It's conceptually simpler (and similar
> to what other such tests are doing), works for me -- but can you also
> please test this?
> 
> --- libffi/testsuite/lib/libffi.exp
> +++ libffi/testsuite/lib/libffi.exp
> @@ -227,20 +227,21 @@ proc libffi_target_compile { source dest type options } {
>  
>  # TEST should be a preprocessor condition.  Returns true if it holds.
>  proc libffi_feature_test { test } {
> -    set src "ffitest.c"
> +    set src "ffitest[pid].c"
>  
>      set f [open $src "w"]
>      puts $f "#include <ffi.h>"
>      puts $f $test
> -    puts $f "xyzzy"
> +    puts $f "/* OK */"
> +    puts $f "#else"
> +    puts $f "# error Failed $test"
>      puts $f "#endif"
>      close $f
>  
> -    set lines [libffi_target_compile $src "" "preprocess" ""]
> +    set lines [libffi_target_compile $src /dev/null assembly ""]
>      file delete $src
>  
> -    set last [lindex [split $lines] end]
> -    return [regexp -- "xyzzy" $last]
> +    return [string match "" $lines]
>  }
>  
>  # Utility routines.


Grüße
 Thomas
Jakub Jelinek April 21, 2016, 1:03 p.m. UTC | #5
On Thu, Apr 21, 2016 at 02:21:07PM +0200, Thomas Schwinge wrote:
> Hi!
> 
> Jakub, ping for gcc-6-branch (or, are you seeing useful libffi testing
> results there?); I understand Mike's emails to mean that he's approved
> the patch for trunk, so I'll commit it there, soon.
> 
> I also filed this as <https://github.com/atgreen/libffi/pull/237>; no
> reaction so far.

This IMNSHO isn't release critical, I'm not against applying it for 6.2, but
for 6.1 we should only get release critical stuff in now.

	Jakub
diff mbox

Patch

--- libffi/testsuite/lib/libffi.exp
+++ libffi/testsuite/lib/libffi.exp
@@ -227,20 +227,21 @@  proc libffi_target_compile { source dest type options } {
 
 # TEST should be a preprocessor condition.  Returns true if it holds.
 proc libffi_feature_test { test } {
-    set src "ffitest.c"
+    set src "ffitest[pid].c"
 
     set f [open $src "w"]
     puts $f "#include <ffi.h>"
     puts $f $test
-    puts $f "xyzzy"
+    puts $f "/* OK */"
+    puts $f "#else"
+    puts $f "# error Failed $test"
     puts $f "#endif"
     close $f
 
-    set lines [libffi_target_compile $src "" "preprocess" ""]
+    set lines [libffi_target_compile $src /dev/null assembly ""]
     file delete $src
 
-    set last [lindex [split $lines] end]
-    return [regexp -- "xyzzy" $last]
+    return [string match "" $lines]
 }
 
 # Utility routines.