[PATCH 4/4] Gccgo port to s390[x] -- part II

[PATCH 4/4] Gccgo port to s390[x] -- part II

[Dominik Vogt]

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

See commit comment and ChangeLog for details.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0004-ChangeLog --]
[-- Type: text/plain, Size: 761 bytes --]

gcc/testsuite/ChangeLog
2014-11-04  Dominik Vogt  <vogt@linux.vnet.ibm.com>

	* go.test/test/ken/cplx2.go:
	Fix s390 test failures.

2014-11-04  Dominik Vogt  <vogt@linux.vnet.ibm.com>

	* go.test/test/nilptr_s390.go: Port nilptr.go to s390.
	* go.test/test/nilptr_s390x.go: Port nilptr.go to s390x.
	* go.test/test/nilptr.go: Do not run on s390/s390x.
	* go.test/go-test.exp:
	"// +build " matches complete words without whitespace.  This is
	rnecessary to distinguish s390 from s390x.  It also handles negation
	with the '!' prefix.

2014-11-04  Dominik Vogt  <vogt@linux.vnet.ibm.com>

	* go.test/go-test.exp (go-set-goarch): Enable tests on s390[x].

2014-11-04  Dominik Vogt  <vogt@linux.vnet.ibm.com>

	* go.test/go-test.exp: Add handling of "// +build".

[-- Attachment #3: 0004-go.test-Changes-required-for-s390-x-port.patch --]
[-- Type: text/x-diff, Size: 10145 bytes --]

From 5b0eaf73d005bd152fff5a1a922f5618da4be939 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Tue, 4 Nov 2014 10:13:22 +0100
Subject: [PATCH 4/4] go.test: Changes required for s390[x] port.

1) Add Go architectures s390 and s390x.

2) Do not run test nilptr.go on s390 -> platform specific test.

   * Detects word boundaries to distinguish between s390 and s390x.
     (Switch to using regular expressions.)
   * Implement the Prefix '!' to exclude targets from build.

3) Fix go/test/ken/cplx2.go test failures.
---
 gcc/testsuite/go.test/go-test.exp          |   6 +
 gcc/testsuite/go.test/test/ken/cplx2.go    |  20 ++-
 gcc/testsuite/go.test/test/nilptr.go       |   1 +
 gcc/testsuite/go.test/test/nilptr_s390.go  | 190 +++++++++++++++++++++++++++++
 gcc/testsuite/go.test/test/nilptr_s390x.go | 190 +++++++++++++++++++++++++++++
 5 files changed, 405 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/go.test/test/nilptr_s390.go
 create mode 100644 gcc/testsuite/go.test/test/nilptr_s390x.go

diff --git a/gcc/testsuite/go.test/go-test.exp b/gcc/testsuite/go.test/go-test.exp
index 71272a3..25e405b 100644
--- a/gcc/testsuite/go.test/go-test.exp
+++ b/gcc/testsuite/go.test/go-test.exp
@@ -244,6 +244,12 @@ proc go-set-goarch { } {
 		set goarch "ppc64"
 	    }
 	}
+	"s390-*-*" {
+	    set goarch "s390"
+	}
+	"s390x-*-*" {
+	    set goarch "s390x"
+	}
 	"sparc*-*-*" {
 	    if [check_effective_target_ilp32] {
 		set goarch "sparc"
diff --git a/gcc/testsuite/go.test/test/ken/cplx2.go b/gcc/testsuite/go.test/test/ken/cplx2.go
index eb1da7b..d11e33c 100644
--- a/gcc/testsuite/go.test/test/ken/cplx2.go
+++ b/gcc/testsuite/go.test/test/ken/cplx2.go
@@ -97,13 +97,29 @@ func main() {
 	}
 
 	cd := c5 / c6
-	if cd != Cd {
+	dr := real(Cd) - real(cd)
+	if dr < 0 {
+		dr = -dr
+	}
+	di := imag(Cd) - imag(cd)
+	if di < 0 {
+		di = -di
+	}
+	if dr > .000000059604644775390625 || di > 0 {
 		println("opcode x", cd, Cd)
 		panic("fail")
 	}
 
 	ce := cd * c6
-	if ce != Ce {
+	dr = real(Ce) - real(ce)
+	if dr < 0 {
+		dr = -dr
+	}
+	di = imag(Ce) - imag(ce)
+	if di < 0 {
+		di = -di
+	}
+	if dr > 0 || di > 0.00000095367431640625 {
 		println("opcode x", ce, Ce)
 		panic("fail")
 	}
diff --git a/gcc/testsuite/go.test/test/nilptr.go b/gcc/testsuite/go.test/test/nilptr.go
index 9631d16..574d662 100644
--- a/gcc/testsuite/go.test/test/nilptr.go
+++ b/gcc/testsuite/go.test/test/nilptr.go
@@ -1,3 +1,4 @@
+// +build !s390 !s390x
 // run
 
 // Copyright 2011 The Go Authors.  All rights reserved.
diff --git a/gcc/testsuite/go.test/test/nilptr_s390.go b/gcc/testsuite/go.test/test/nilptr_s390.go
new file mode 100644
index 0000000..d79916d
--- /dev/null
+++ b/gcc/testsuite/go.test/test/nilptr_s390.go
@@ -0,0 +1,190 @@
+// +build s390
+// run
+
+// Copyright 2011 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Test that the implementation catches nil ptr indirection in a
+// large address space.
+
+package main
+
+import "unsafe"
+
+// Having a big address space means that indexing at a large
+// offset from a nil pointer might not cause a memory access
+// fault.  This test checks that Go is doing the correct explicit
+// checks to catch these nil pointer accesses, not just relying on
+// the hardware.
+//
+// Give us a big address space somewhere near min_bss_offset.
+const in_mem_size uintptr = 256 << 20 // 256 MiB
+const min_bss_offset uintptr = 1 << 22 // 0x00400000
+const maxlen uintptr = (1 << 31) - 2 // 0x7ffffffe
+var dummy [in_mem_size]byte
+
+func main() {
+	// The test only tests what we intend to test if dummy
+	// starts near 0x0040000.  Otherwise there might not be
+	// anything mapped at the address that might be
+	// accidentally dereferenced below.
+	if uintptr(unsafe.Pointer(&dummy)) > in_mem_size + min_bss_offset {
+		panic("dummy too far out")
+	} else if uintptr(unsafe.Pointer(&dummy)) < min_bss_offset {
+		panic("dummy too close")
+	}
+
+	shouldPanic(p1)
+	shouldPanic(p2)
+	shouldPanic(p3)
+	shouldPanic(p4)
+	shouldPanic(p5)
+	shouldPanic(p6)
+	shouldPanic(p7)
+	shouldPanic(p8)
+	shouldPanic(p9)
+	shouldPanic(p10)
+	shouldPanic(p11)
+	shouldPanic(p12)
+	shouldPanic(p13)
+	shouldPanic(p14)
+	shouldPanic(p15)
+	shouldPanic(p16)
+}
+
+func shouldPanic(f func()) {
+	defer func() {
+		if recover() == nil {
+			panic("memory reference did not panic")
+		}
+	}()
+	f()
+}
+
+func p1() {
+	// Array index.
+	var p *[maxlen]byte = nil
+	// very likely to be inside dummy, but should panic
+	println(p[min_bss_offset + in_mem_size / 2])
+}
+
+var xb byte
+
+func p2() {
+	var p *[maxlen]byte = nil
+	xb = 123
+
+	// Array index.
+	println(p[uintptr(unsafe.Pointer(&xb))]) // should panic
+}
+
+func p3() {
+	// Array to slice.
+	var p *[maxlen]byte = nil
+	var x []byte = p[0:] // should panic
+	_ = x
+}
+
+var q *[maxlen]byte
+
+func p4() {
+	// Array to slice.
+	var x []byte
+	var y = &x
+	*y = q[0:] // should crash (uses arraytoslice runtime routine)
+}
+
+func fb([]byte) {
+	panic("unreachable")
+}
+
+func p5() {
+	// Array to slice.
+	var p *[maxlen]byte = nil
+	fb(p[0:]) // should crash
+}
+
+func p6() {
+	// Array to slice.
+	var p *[maxlen]byte = nil
+	var _ []byte = p[10 : len(p)-10] // should crash
+}
+
+type T struct {
+	x [in_mem_size]byte
+	i int
+}
+
+func f() *T {
+	return nil
+}
+
+var y *T
+var x = &y
+
+func p7() {
+	// Struct field access with large offset.
+	println(f().i) // should crash
+}
+
+func p8() {
+	// Struct field access with large offset.
+	println((*x).i) // should crash
+}
+
+func p9() {
+	// Struct field access with large offset.
+	var t *T
+	println(&t.i) // should crash
+}
+
+func p10() {
+	// Struct field access with large offset.
+	var t *T
+	println(t.i) // should crash
+}
+
+type T1 struct {
+	T
+}
+
+type T2 struct {
+	*T1
+}
+
+func p11() {
+	t := &T2{}
+	p := &t.i
+	println(*p)
+}
+
+// ADDR(DOT(IND(p))) needs a check also
+func p12() {
+	var p *T = nil
+	println(*(&((*p).i)))
+}
+
+// Tests suggested in golang.org/issue/6080.
+
+func p13() {
+	var x *[10]int
+	y := x[:]
+	_ = y
+}
+
+func p14() {
+	println((*[1]int)(nil)[:])
+}
+
+func p15() {
+	for i := range (*[1]int)(nil)[:] {
+		_ = i
+	}
+}
+
+func p16() {
+	for i, v := range (*[1]int)(nil)[:] {
+		_ = i + v
+	}
+}
diff --git a/gcc/testsuite/go.test/test/nilptr_s390x.go b/gcc/testsuite/go.test/test/nilptr_s390x.go
new file mode 100644
index 0000000..e5f6d56
--- /dev/null
+++ b/gcc/testsuite/go.test/test/nilptr_s390x.go
@@ -0,0 +1,190 @@
+// +build s390x
+// run
+
+// Copyright 2011 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Test that the implementation catches nil ptr indirection in a
+// large address space.
+
+package main
+
+import "unsafe"
+
+// Having a big address space means that indexing at a large
+// offset from a nil pointer might not cause a memory access
+// fault.  This test checks that Go is doing the correct explicit
+// checks to catch these nil pointer accesses, not just relying on
+// the hardware.
+//
+// Give us a big address space somewhere near min_bss_offset.
+const in_mem_size uintptr = 256 << 20 // 256 MiB
+const min_bss_offset uintptr = 1 << 31 // 0x80000000
+const maxlen uintptr = (1 << 32) - 1 // 0xffffffff
+var dummy [in_mem_size]byte
+
+func main() {
+	// The test only tests what we intend to test if dummy
+	// starts near 0x8000000.  Otherwise there might not be
+	// anything mapped at the address that might be
+	// accidentally dereferenced below.
+	if uintptr(unsafe.Pointer(&dummy)) > in_mem_size + min_bss_offset {
+		panic("dummy too far out")
+	} else if uintptr(unsafe.Pointer(&dummy)) < min_bss_offset {
+		panic("dummy too close")
+	}
+
+	shouldPanic(p1)
+	shouldPanic(p2)
+	shouldPanic(p3)
+	shouldPanic(p4)
+	shouldPanic(p5)
+	shouldPanic(p6)
+	shouldPanic(p7)
+	shouldPanic(p8)
+	shouldPanic(p9)
+	shouldPanic(p10)
+	shouldPanic(p11)
+	shouldPanic(p12)
+	shouldPanic(p13)
+	shouldPanic(p14)
+	shouldPanic(p15)
+	shouldPanic(p16)
+}
+
+func shouldPanic(f func()) {
+	defer func() {
+		if recover() == nil {
+			panic("memory reference did not panic")
+		}
+	}()
+	f()
+}
+
+func p1() {
+	// Array index.
+	var p *[maxlen]byte = nil
+	// very likely to be inside dummy, but should panic
+	println(p[min_bss_offset + in_mem_size / 2])
+}
+
+var xb byte
+
+func p2() {
+	var p *[maxlen]byte = nil
+	xb = 123
+
+	// Array index.
+	println(p[uintptr(unsafe.Pointer(&xb))]) // should panic
+}
+
+func p3() {
+	// Array to slice.
+	var p *[maxlen]byte = nil
+	var x []byte = p[0:] // should panic
+	_ = x
+}
+
+var q *[maxlen]byte
+
+func p4() {
+	// Array to slice.
+	var x []byte
+	var y = &x
+	*y = q[0:] // should crash (uses arraytoslice runtime routine)
+}
+
+func fb([]byte) {
+	panic("unreachable")
+}
+
+func p5() {
+	// Array to slice.
+	var p *[maxlen]byte = nil
+	fb(p[0:]) // should crash
+}
+
+func p6() {
+	// Array to slice.
+	var p *[maxlen]byte = nil
+	var _ []byte = p[10 : len(p)-10] // should crash
+}
+
+type T struct {
+	x [in_mem_size]byte
+	i int
+}
+
+func f() *T {
+	return nil
+}
+
+var y *T
+var x = &y
+
+func p7() {
+	// Struct field access with large offset.
+	println(f().i) // should crash
+}
+
+func p8() {
+	// Struct field access with large offset.
+	println((*x).i) // should crash
+}
+
+func p9() {
+	// Struct field access with large offset.
+	var t *T
+	println(&t.i) // should crash
+}
+
+func p10() {
+	// Struct field access with large offset.
+	var t *T
+	println(t.i) // should crash
+}
+
+type T1 struct {
+	T
+}
+
+type T2 struct {
+	*T1
+}
+
+func p11() {
+	t := &T2{}
+	p := &t.i
+	println(*p)
+}
+
+// ADDR(DOT(IND(p))) needs a check also
+func p12() {
+	var p *T = nil
+	println(*(&((*p).i)))
+}
+
+// Tests suggested in golang.org/issue/6080.
+
+func p13() {
+	var x *[10]int
+	y := x[:]
+	_ = y
+}
+
+func p14() {
+	println((*[1]int)(nil)[:])
+}
+
+func p15() {
+	for i := range (*[1]int)(nil)[:] {
+		_ = i
+	}
+}
+
+func p16() {
+	for i, v := range (*[1]int)(nil)[:] {
+		_ = i + v
+	}
+}
-- 
1.8.4.2

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Ian Taylor]

I committed the change to go-test.exp.  Thanks.

The other changes are not OK.  As described in
gcc/testsuite/go.test/test/README.gcc, the files in
gcc/testsuite/go.test/test are an exact copy of the master Go
testsuite.  Any changes must be made to the master Go testsuite first.

I don't know what's up with the complex number change.  In general the
Go compiler and libraries go to some effort to produce the same
answers on all platforms.  We need to understand why we get different
answers on s390 (you may understand the differences, but I don't).  I
won't change the tests without a clear understanding of why we are
changing them.

The nilptr test doesn't run on some other platforms when using
gccgo--search for "nilptr" in go-test.exp.  If you want to work out a
way to change the master Go testsuite such that the nilptr test passes
on more platforms, that would be great.  The way to do it is not by
copying the test.  If the test needs to be customized, add additional
files that use // +build lines to pick which files is built.  Move
them into a directory, like method4.go or other tests that use
"rundir".

Ian

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Dominik Vogt]

On Tue, Nov 04, 2014 at 08:16:51PM -0800, Ian Taylor wrote:
> I committed the change to go-test.exp.  Thanks.
> 
> The other changes are not OK.  As described in
> gcc/testsuite/go.test/test/README.gcc, the files in
> gcc/testsuite/go.test/test are an exact copy of the master Go
> testsuite.  Any changes must be made to the master Go testsuite first.

I understand that, but I'm unsure how to handle a set of patches
that all depend on each other but refer to three different
reposiories.  So I posted this patch intentionally in the wrong
place, not knowing how to do it in a better way.

> I don't know what's up with the complex number change. In general the
> Go compiler and libraries go to some effort to produce the same
> answers on all platforms.  We need to understand why we get different
> answers on s390 (you may understand the differences, but I don't).  I
> won't change the tests without a clear understanding of why we are
> changing them.

It's actually not a Go specific problem, the same deviation occurs
in C code too.  The cause is that constant folding is done with a
higher precision and may yield a different result than the run
time calculations.  There is a Gcc bug report for that "issue":
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60181

> The nilptr test doesn't run on some other platforms when using
> gccgo--search for "nilptr" in go-test.exp.  If you want to work out a
> way to change the master Go testsuite such that the nilptr test passes
> on more platforms, that would be great.

I don't have the slightest clue how this could be done in a
platform independent way because the test heavily depends on the
target's memory map layout.

> The way to do it is not by
> copying the test.  If the test needs to be customized, add additional
> files that use // +build lines to pick which files is built.  Move
> them into a directory, like method4.go or other tests that use
> "rundir".

I'll check that.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Ian Taylor]

On Wed, Nov 5, 2014 at 2:05 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> On Tue, Nov 04, 2014 at 08:16:51PM -0800, Ian Taylor wrote:
>> I committed the change to go-test.exp.  Thanks.
>>
>> The other changes are not OK.  As described in
>> gcc/testsuite/go.test/test/README.gcc, the files in
>> gcc/testsuite/go.test/test are an exact copy of the master Go
>> testsuite.  Any changes must be made to the master Go testsuite first.
>
> I understand that, but I'm unsure how to handle a set of patches
> that all depend on each other but refer to three different
> reposiories.  So I posted this patch intentionally in the wrong
> place, not knowing how to do it in a better way.

Changes to the master Go repository must follow the procedure
described at http://golang.org/doc/contribute.html.


>> I don't know what's up with the complex number change. In general the
>> Go compiler and libraries go to some effort to produce the same
>> answers on all platforms.  We need to understand why we get different
>> answers on s390 (you may understand the differences, but I don't).  I
>> won't change the tests without a clear understanding of why we are
>> changing them.
>
> It's actually not a Go specific problem, the same deviation occurs
> in C code too.  The cause is that constant folding is done with a
> higher precision and may yield a different result than the run
> time calculations.  There is a Gcc bug report for that "issue":
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60181

So far it doesn't sound appropriate to change the Go testsuite for
this.  If the immediate goal is simply to get the s390 tests to pass,
let's change go-test.exp to xfail the test unless and until somebody
figures out the whole issue.

Ian

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Dominik Vogt]

On Tue, Nov 04, 2014 at 08:16:51PM -0800, Ian Taylor wrote:
> The way to do it is not by
> copying the test.  If the test needs to be customized, add additional
> files that use // +build lines to pick which files is built.  Move
> them into a directory, like method4.go or other tests that use
> "rundir".

Currently go-test.exp does not look at the "build" lines of the
files in subdirectories.  Before I add that to the gcc testsuite
start adding that, is it certain that the golang testsuite will be
able to understand that and compile only the requested files?

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Ian Taylor]

On Thu, Nov 6, 2014 at 4:04 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> On Tue, Nov 04, 2014 at 08:16:51PM -0800, Ian Taylor wrote:
>> The way to do it is not by
>> copying the test.  If the test needs to be customized, add additional
>> files that use // +build lines to pick which files is built.  Move
>> them into a directory, like method4.go or other tests that use
>> "rundir".
>
> Currently go-test.exp does not look at the "build" lines of the
> files in subdirectories.  Before I add that to the gcc testsuite
> start adding that, is it certain that the golang testsuite will be
> able to understand that and compile only the requested files?

Hmmm, that is a good point.  The testsuite doesn't use the go command
to build the files in subdirectories, so it won't honor the +build
lines.  I didn't think of that.  Sorry for pointing you in the wrong
direction.

I'd still like to avoid the rampant duplication if possible.  One
approach would be to put most of the test in something like
nilptr_tests.go marked with "// skip".  Then we can have top-level
nilptrXX.go tests with +build lines that use "// run nilptr_tests.go".

(By the way, it's not "golang;" it's "Go."  Please try to avoid the
term "golang."  Thanks.)

Ian

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Dominik Vogt]

On Thu, Nov 06, 2014 at 09:06:18AM -0800, Ian Taylor wrote:
> On Thu, Nov 6, 2014 at 4:04 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> > On Tue, Nov 04, 2014 at 08:16:51PM -0800, Ian Taylor wrote:
> >> The way to do it is not by
> >> copying the test.  If the test needs to be customized, add additional
> >> files that use // +build lines to pick which files is built.  Move
> >> them into a directory, like method4.go or other tests that use
> >> "rundir".
> >
> > Currently go-test.exp does not look at the "build" lines of the
> > files in subdirectories.  Before I add that to the gcc testsuite
> > start adding that, is it certain that the golang testsuite will be
> > able to understand that and compile only the requested files?
> 
> Hmmm, that is a good point.  The testsuite doesn't use the go command
> to build the files in subdirectories, so it won't honor the +build
> lines.  I didn't think of that.  Sorry for pointing you in the wrong
> direction.

That's no problem, I can enhance go-test.exp in Gcc.  The question
is if test cases extended in such a way would run in the master Go
repository too.  Are the tests there run with the Go tool?

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Ian Taylor]

On Fri, Nov 7, 2014 at 12:51 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> On Thu, Nov 06, 2014 at 09:06:18AM -0800, Ian Taylor wrote:
>> On Thu, Nov 6, 2014 at 4:04 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
>> > On Tue, Nov 04, 2014 at 08:16:51PM -0800, Ian Taylor wrote:
>> >> The way to do it is not by
>> >> copying the test.  If the test needs to be customized, add additional
>> >> files that use // +build lines to pick which files is built.  Move
>> >> them into a directory, like method4.go or other tests that use
>> >> "rundir".
>> >
>> > Currently go-test.exp does not look at the "build" lines of the
>> > files in subdirectories.  Before I add that to the gcc testsuite
>> > start adding that, is it certain that the golang testsuite will be
>> > able to understand that and compile only the requested files?
>>
>> Hmmm, that is a good point.  The testsuite doesn't use the go command
>> to build the files in subdirectories, so it won't honor the +build
>> lines.  I didn't think of that.  Sorry for pointing you in the wrong
>> direction.
>
> That's no problem, I can enhance go-test.exp in Gcc.  The question
> is if test cases extended in such a way would run in the master Go
> repository too.  Are the tests there run with the Go tool?

I'm sorry, I wasn't clear.  The test cases will not work in the master
Go repository.  When I said "the testsuite doesn't use go command" I
was referring to the master testsuite.  Sorry for the confusion.

Ian

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Michael Hudson-Doyle]

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

Ian Taylor <iant@golang.org> writes:

> I don't know what's up with the complex number change.  In general the
> Go compiler and libraries go to some effort to produce the same
> answers on all platforms.  We need to understand why we get different
> answers on s390 (you may understand the differences, but I don't).

Oh, I know this one.  I've even filed yet another bug about it:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59714

I assume s390 has a fused multiply add instruction?  It's because
libgcc's implementation of complex division is written in a way such
that gcc compiles an expression like a*b-c*d as fma(a,b,-c*d) and even if
a==c and b==d the latter expression doesn't return 0 unless things are
in power of 2 ratios with one another.

> I won't change the tests without a clear understanding of why we are
> changing them.

I think the *real* fix is for libgcc to use ""Kahan's compensated
algorithm for 2 by 2 determinants"[1] to compute a*b-c*d when fma is
available.

Cheers,
mwh

[1] That's something like this:

    // This implements what is sometimes called "Kahan's compensated algorithm for
    // 2 by 2 determinants".  It returns a*b + c*d to a high degree of precision
    // but depends on a fused-multiply add operation that rounds once.
    //
    // The obvious algorithm has problems when a*b and c*d nearly cancel, but the
    // trick is the calculation of 'e': "a*b = w + e" is exact when the operands
    // are considered as real numbers.  So if c*d nearly cancels out w, e restores
    // the result to accuracy.
    double
    Kahan(double a, double b, double c, double d)
    {
      double w, e, f;
      w = b * a;
      e = fma(b, a, -w);
      f = fma(d, c, w);
      return f + e;
    }

    Algorithms like this is why the fma operation was introduced in the
    first place!

[-- Attachment #2: Type: application/pgp-signature, Size: 818 bytes --]

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Dominik Vogt]

> I'd still like to avoid the rampant duplication if possible.  One
> approach would be to put most of the test in something like
> nilptr_tests.go marked with "// skip".  Then we can have top-level
> nilptrXX.go tests with +build lines that use "// run nilptr_tests.go".

I fail to see how that could be done with "// run".  There is one
example use, namely cmplxdivide.go".  That is not run in gcc
because the "run" line does not match anything in go-test.exp.  If
I add a rule for that, how does that help me to compile a test
that consists of multiple files?

At the moment, I've no idea how to tackle the multi file problem
with the existing go-test.exp.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Ian Taylor]

On Mon, Nov 10, 2014 at 6:00 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
>> I'd still like to avoid the rampant duplication if possible.  One
>> approach would be to put most of the test in something like
>> nilptr_tests.go marked with "// skip".  Then we can have top-level
>> nilptrXX.go tests with +build lines that use "// run nilptr_tests.go".
>
> I fail to see how that could be done with "// run".  There is one
> example use, namely cmplxdivide.go".  That is not run in gcc
> because the "run" line does not match anything in go-test.exp.  If
> I add a rule for that, how does that help me to compile a test
> that consists of multiple files?

That test is run (all tests are run or explicitly skipped or marked
unsupported).  In go-test.exp look for
    $test_line == "// run cmplxdivide1.go"

Ian

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Dominik Vogt]

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

On Fri, Nov 07, 2014 at 08:24:15AM -0800, Ian Taylor wrote:
> On Fri, Nov 7, 2014 at 12:51 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> > On Thu, Nov 06, 2014 at 09:06:18AM -0800, Ian Taylor wrote:
> >> On Thu, Nov 6, 2014 at 4:04 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> >> > On Tue, Nov 04, 2014 at 08:16:51PM -0800, Ian Taylor wrote:
> >> >> The way to do it is not by
> >> >> copying the test.  If the test needs to be customized, add additional
> >> >> files that use // +build lines to pick which files is built.  Move
> >> >> them into a directory, like method4.go or other tests that use
> >> >> "rundir".
> >> >
> >> > Currently go-test.exp does not look at the "build" lines of the
> >> > files in subdirectories.  Before I add that to the gcc testsuite
> >> > start adding that, is it certain that the golang testsuite will be
> >> > able to understand that and compile only the requested files?
> >>
> >> Hmmm, that is a good point.  The testsuite doesn't use the go command
> >> to build the files in subdirectories, so it won't honor the +build
> >> lines.  I didn't think of that.  Sorry for pointing you in the wrong
> >> direction.
> >
> > That's no problem, I can enhance go-test.exp in Gcc.  The question
> > is if test cases extended in such a way would run in the master Go
> > repository too.  Are the tests there run with the Go tool?

What do you think about the attached patches?  They work for me, but I'm
not sure whether the patch to go-test.exp is good because I know
nothing about tcl.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-go.test-Fix-build-lines-that-have-only-negative-spec.patch --]
[-- Type: text/x-diff, Size: 1067 bytes --]

From 155982afbcaf36fc79a89d222f0ff1b9da17464a Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Mon, 10 Nov 2014 13:21:42 +0100
Subject: [PATCH 1/6] go.test: Fix "// +build" lines that have only negative
 specifiers

Lines like this did not cause the file to be built on other platforms:

-- snip --
// +build !s390 !s390x
-- snip --
---
 gcc/testsuite/go.test/go-test.exp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/testsuite/go.test/go-test.exp b/gcc/testsuite/go.test/go-test.exp
index 25e405b..590aaf9 100644
--- a/gcc/testsuite/go.test/go-test.exp
+++ b/gcc/testsuite/go.test/go-test.exp
@@ -469,6 +469,9 @@ proc go-gc-tests { } {
 		    set matches_pos 1
 		} elseif { [ regexp -line "\[ 	\]windows\(\[ 	\]\|\$\)" $test_line ] } {
 		    set matches_neg 1
+		} elseif { [ regexp -line "// \\+build(\[ 	\]+!\[^ 	!\]+)*\[ 	\]*\$" $test_line ] } {
+		    # line has only negative terms, default to positive match
+		    set matches_pos 1
 		}
 		if { $matches_pos == 1 && $matches_neg == 0 } {
 		    continue
-- 
1.8.4.2


[-- Attachment #3: 0001-ChangeLog --]
[-- Type: text/plain, Size: 179 bytes --]

gcc/testsuite/ChangeLog
2014-11-10  Dominik Vogt  <vogt@linux.vnet.ibm.com>

	* go.test/go-test.exp: Fix "// +build" lines that have only negative
	specifiers (prefixed with '!')

[-- Attachment #4: 0002-go.test-Do-not-run-test-nilptr.go-on-s390-platform-s.patch --]
[-- Type: text/x-diff, Size: 7693 bytes --]

From 35d0e1052dce2eabed57f3c8c76bfbb62cf48676 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Tue, 4 Nov 2014 10:13:22 +0100
Subject: [PATCH 3/6] go.test: Do not run test nilptr.go on s390 -> platform
 specific test.

* Detects word boundaries to distinguish between s390 and s390x.
  (Switch to using regular expressions.)
* Implement the Prefix '!' to exclude targets from build.
---
 gcc/testsuite/go.test/test/nilptr.go             | 35 ++++++++----------------
 gcc/testsuite/go.test/test/nilptr_other.go       | 31 +++++++++++++++++++++
 gcc/testsuite/go.test/test/nilptr_s390.go        | 23 ++++++++++++++++
 gcc/testsuite/go.test/test/nilptr_s390_common.go | 23 ++++++++++++++++
 gcc/testsuite/go.test/test/nilptr_s390x.go       | 23 ++++++++++++++++
 5 files changed, 111 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/go.test/test/nilptr_other.go
 create mode 100644 gcc/testsuite/go.test/test/nilptr_s390.go
 create mode 100644 gcc/testsuite/go.test/test/nilptr_s390_common.go
 create mode 100644 gcc/testsuite/go.test/test/nilptr_s390x.go

diff --git a/gcc/testsuite/go.test/test/nilptr.go b/gcc/testsuite/go.test/test/nilptr.go
index 9631d16..9bc8045 100644
--- a/gcc/testsuite/go.test/test/nilptr.go
+++ b/gcc/testsuite/go.test/test/nilptr.go
@@ -1,4 +1,4 @@
-// run
+// skip
 
 // Copyright 2011 The Go Authors.  All rights reserved.
 // Use of this source code is governed by a BSD-style
@@ -11,22 +11,8 @@ package main
 
 import "unsafe"
 
-// Having a big address space means that indexing
-// at a 256 MB offset from a nil pointer might not
-// cause a memory access fault. This test checks
-// that Go is doing the correct explicit checks to catch
-// these nil pointer accesses, not just relying on the hardware.
-var dummy [256 << 20]byte // give us a big address space
-
 func main() {
-	// the test only tests what we intend to test
-	// if dummy starts in the first 256 MB of memory.
-	// otherwise there might not be anything mapped
-	// at the address that might be accidentally
-	// dereferenced below.
-	if uintptr(unsafe.Pointer(&dummy)) > 256<<20 {
-		panic("dummy too far out")
-	}
+	sanityCheck()
 
 	shouldPanic(p1)
 	shouldPanic(p2)
@@ -57,14 +43,15 @@ func shouldPanic(f func()) {
 
 func p1() {
 	// Array index.
-	var p *[1 << 30]byte = nil
-	println(p[256<<20]) // very likely to be inside dummy, but should panic
+	var p *[maxlen]byte = nil
+	// very likely to be inside dummy, but should panic
+	println(p[inMaxlenArray])
 }
 
 var xb byte
 
 func p2() {
-	var p *[1 << 30]byte = nil
+	var p *[maxlen]byte = nil
 	xb = 123
 
 	// Array index.
@@ -73,12 +60,12 @@ func p2() {
 
 func p3() {
 	// Array to slice.
-	var p *[1 << 30]byte = nil
+	var p *[maxlen]byte = nil
 	var x []byte = p[0:] // should panic
 	_ = x
 }
 
-var q *[1 << 30]byte
+var q *[maxlen]byte
 
 func p4() {
 	// Array to slice.
@@ -93,18 +80,18 @@ func fb([]byte) {
 
 func p5() {
 	// Array to slice.
-	var p *[1 << 30]byte = nil
+	var p *[maxlen]byte = nil
 	fb(p[0:]) // should crash
 }
 
 func p6() {
 	// Array to slice.
-	var p *[1 << 30]byte = nil
+	var p *[maxlen]byte = nil
 	var _ []byte = p[10 : len(p)-10] // should crash
 }
 
 type T struct {
-	x [256 << 20]byte
+	x [inMemSize]byte
 	i int
 }
 
diff --git a/gcc/testsuite/go.test/test/nilptr_other.go b/gcc/testsuite/go.test/test/nilptr_other.go
new file mode 100644
index 0000000..7747f70
--- /dev/null
+++ b/gcc/testsuite/go.test/test/nilptr_other.go
@@ -0,0 +1,31 @@
+// +build !s390 !s390x
+// run nilptr.go
+
+// Copyright 2011 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Platform specific test configuration
+
+package main
+
+// Having a big address space means that indexing
+// at a 256 MB offset from a nil pointer might not
+// cause a memory access fault. This test checks
+// that Go is doing the correct explicit checks to catch
+// these nil pointer accesses, not just relying on the hardware.
+const inMemSize uintptr = 256 << 20 // 256 MiB
+const maxlen uintptr = (1 << 30) - 2 // 0x40000000
+const inMaxlenArray uintptr = 256 << 20
+var dummy [256 << 20]byte // give us a big address space
+
+func sanityCheck() {
+	// the test only tests what we intend to test
+	// if dummy starts in the first 256 MB of memory.
+	// otherwise there might not be anything mapped
+	// at the address that might be accidentally
+	// dereferenced below.
+	if uintptr(unsafe.Pointer(&dummy)) > 256<<20 {
+		panic("dummy too far out")
+	}
+}
diff --git a/gcc/testsuite/go.test/test/nilptr_s390.go b/gcc/testsuite/go.test/test/nilptr_s390.go
new file mode 100644
index 0000000..8a810ad
--- /dev/null
+++ b/gcc/testsuite/go.test/test/nilptr_s390.go
@@ -0,0 +1,23 @@
+// +build s390
+// run nilptr.go nilptr_s390_common.go
+
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Platform specific test configuration
+
+package main
+
+// Having a big address space means that indexing at a large
+// offset from a nil pointer might not cause a memory access
+// fault.  This test checks that Go is doing the correct explicit
+// checks to catch these nil pointer accesses, not just relying on
+// the hardware.
+//
+// Give us a big address space somewhere near minBssOffset.
+const inMemSize uintptr = 256 << 20 // 256 MiB
+const minBssOffset uintptr = 1 << 22 // 0x00400000
+const maxlen uintptr = (1 << 31) - 2 // 0x7ffffffe
+const inMaxlenArray uintptr = minBssOffset + inMemSize / 2
+var dummy [inMemSize]byte
diff --git a/gcc/testsuite/go.test/test/nilptr_s390_common.go b/gcc/testsuite/go.test/test/nilptr_s390_common.go
new file mode 100644
index 0000000..be76beb
--- /dev/null
+++ b/gcc/testsuite/go.test/test/nilptr_s390_common.go
@@ -0,0 +1,23 @@
+// skip
+
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Shared s390 and s390x specific test configuration
+
+package main
+
+import "unsafe"
+
+func sanityCheck() {
+	// The test only tests what we intend to test if dummy
+	// starts near minBssOffset uintptr.  Otherwise there
+	// might not be anything mapped at the address that might
+	// be accidentally dereferenced below.
+	if uintptr(unsafe.Pointer(&dummy)) > inMemSize + minBssOffset {
+		panic("dummy too far out")
+	} else if uintptr(unsafe.Pointer(&dummy)) < minBssOffset {
+		panic("dummy too close")
+	}
+}
diff --git a/gcc/testsuite/go.test/test/nilptr_s390x.go b/gcc/testsuite/go.test/test/nilptr_s390x.go
new file mode 100644
index 0000000..85521b5
--- /dev/null
+++ b/gcc/testsuite/go.test/test/nilptr_s390x.go
@@ -0,0 +1,23 @@
+// +build s390x
+// run nilptr.go nilptr_s390_common.go
+
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Platform specific test configuration
+
+package main
+
+// Having a big address space means that indexing at a large
+// offset from a nil pointer might not cause a memory access
+// fault.  This test checks that Go is doing the correct explicit
+// checks to catch these nil pointer accesses, not just relying on
+// the hardware.
+//
+// Give us a big address space somewhere near minBssOffset.
+const inMemSize uintptr = 256 << 20 // 256 MiB
+const minBssOffset uintptr = 1 << 31 // 0x80000000
+const maxlen uintptr = (1 << 32) - 1 // 0xffffffff
+const inMaxlenArray uintptr = minBssOffset + inMemSize / 2
+var dummy [inMemSize]byte
-- 
1.8.4.2


[-- Attachment #5: 0002-ChangeLog --]
[-- Type: text/plain, Size: 310 bytes --]

gcc/testsuite/ChangeLog
2014-11-10  Dominik Vogt  <vogt@linux.vnet.ibm.com>

	* go.test/test/nilptr_s390.go: New file
	* go.test/test/nilptr_s390x.go: New file
	* go.test/test/nilptr_s390_common.go: New file
	* go.test/test/nilptr_other.go: New file
	* go.test/test/nilptr.go: Platform specific test structure

[-- Attachment #6: 0003-go.test-Add-special-handling-of-nilptr.go-to-go-test.patch --]
[-- Type: text/x-diff, Size: 1333 bytes --]

From 728cb48a3c214d387b0b567fd6ef30c80eb0fd73 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Thu, 13 Nov 2014 11:45:44 +0100
Subject: [PATCH 5/6] go.test: Add special handling of nilptr.go to
 go-test.exp.

---
 gcc/testsuite/go.test/go-test.exp | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gcc/testsuite/go.test/go-test.exp b/gcc/testsuite/go.test/go-test.exp
index 590aaf9..c6dbcc5 100644
--- a/gcc/testsuite/go.test/go-test.exp
+++ b/gcc/testsuite/go.test/go-test.exp
@@ -1039,6 +1039,17 @@ proc go-gc-tests { } {
 		fail $name
 	    }
 	    file delete $output_file
+	} elseif { [string match "// run nilptr.go*" $test_line] } {
+	    regsub "/\[^/\]*$" $test "" path
+	    regsub -all " " $test_line " $path/" files
+	    regsub "^// .*run " $files "" files
+	    set hold_runtests $runtests
+	    set runtests "go-test.exp"
+	    set dg-do-what-default "run"
+	    set go_compile_args ""
+	    append go_compile_args $files
+	    go-torture-execute "$test"
+	    set runtests $hold_runtests
 	} elseif { $test_line == "// \$G \$D/\$F.go && \$L \$F.\$A &&" \
 		       && $test_line2 == "// ./\$A.out -pass 0 >tmp.go && \$G tmp.go && \$L -o \$A.out1 tmp.\$A && ./\$A.out1 &&" \
 		       && $test_line3 == "// ./\$A.out -pass 1 >tmp.go && errchk \$G -e tmp.go &&" \
-- 
1.8.4.2


[-- Attachment #7: 0003-ChangeLog --]
[-- Type: text/plain, Size: 166 bytes --]

gcc/testsuite/ChangeLog
2014-11-13  Dominik Vogt  <vogt@linux.vnet.ibm.com>

	* go.test/go-test.exp (go-gc-tests): Add special handling of nilptr.go
	to go-test.exp.

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Ian Taylor]

On Thu, Nov 13, 2014 at 2:58 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
>
> What do you think about the attached patches?  They work for me, but I'm
> not sure whether the patch to go-test.exp is good because I know
> nothing about tcl.

Looks plausible to me.

Ian

Re: [gofrontend-dev] [PATCH 4/4] Gccgo port to s390[x] -- part II

[Dominik Vogt]

On Sat, Nov 15, 2014 at 07:46:40AM -0800, Ian Taylor wrote:
> On Thu, Nov 13, 2014 at 2:58 AM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> > What do you think about the attached patches?  They work for me, but I'm
> > not sure whether the patch to go-test.exp is good because I know
> > nothing about tcl.
> 
> Looks plausible to me.

All right, if you could take care of the two patches for
(go-test.exp) I'll make a patch with the changes to the nilptr
test for the Go repository.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany