Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: many tests on s390x fail when -covermode=atomic is used #24449

Closed
mvo5 opened this issue Mar 19, 2018 · 11 comments

Comments

Projects
None yet
6 participants
@mvo5
Copy link
Contributor

commented Mar 19, 2018

What version of Go are you using (go version)?

The problematic behaviour is shown by: go1.10 linux/s390x

I also tested with go1.9 and go1.8 (from the go snap) and that works fine. Happy to
test more versions.

Does this issue reproduce with the latest release?

I have not tested master but it does reproduce with 1.10.

What operating system and processor architecture are you using (go env)?

GOARCH="s390x"
GOBIN=""
GOCACHE="/home/ubuntu/.cache/go-build"
GOEXE=""
GOHOSTARCH="s390x"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ubuntu/go"
GORACE=""
GOROOT="/usr/lib/go-1.10"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.10/pkg/tool/linux_s390x"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -march=z196 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build233531017=/tmp/go-build -gno-record-gcc-switches"

What did you do?

On the s390x (with Ubuntu 18.04 (bionic)) many tests for our "snapd" project fail when using -covermode="atomic" but work fine with set other cover modes. A simple example:

$ go get github.com/snapcore/snapd
$ cd ~/go/src/github.com/snapcore/snapd/spdx
$ go test
OK: 3 passed
PASS
ok  	github.com/snapcore/snapd/spdx	0.003s
$ go test -covermode="atomic"
----------------------------------------------------------------------
PANIC: parser_test.go:53: spdxSuite.TestParseError

... Panic: runtime error: index out of range (PC=0x72EE3)

/usr/lib/go-1.10/src/runtime/panic.go:505
  in gopanic
/usr/lib/go-1.10/src/runtime/panic.go:28
  in panicindex
scanner.go:45
  in spdxSplit
/usr/lib/go-1.10/src/bufio/scan.go:140
  in Scanner.Scan
parser.go:89
  in parser.validate
parser.go:76
  in parser.Validate
validate.go:33
  in ValidateLicense
parser_test.go:74
  in spdxSuite.TestParseError
/usr/lib/go-1.10/src/reflect/value.go:308
  in Value.Call
/usr/lib/go-1.10/src/runtime/asm_s390x.s:986
  in goexit

What did you expect to see?

I was expecting the test to work regardless of the covermode. The covermodes "set" and "count" are fine, only with "atomic" things break.

What did you see instead?

Same output regardless of -covermode= setting.

[edit: typos]

@mvdan mvdan changed the title Many tests on go1.10/s390x fail when `-covermode="atomic"` is used Many tests on go1.10/s390x fail when -covermode=atomic is used Mar 19, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Could you provide a self-contained piece of code or test that reproduces this problem? Ideally much smaller than go test in a large package.

@mvo5

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

Here is a small testcase:

go-test-coverage-atomic-s390x.tar.gz

I extracted a scanner module from our spdx parser. Its a bit of a random example but it is pretty small. I also simplified the testcase to use no external libs (it used to use gocheck.v1) and only a single trivial test input.

When I run it on a s390x:

ubuntu@clound01:/tmp/22$ go test
PASS
ok  	_/tmp/22	0.001s
ubuntu@clound01:/tmp/22$ go test -covermode=atomic
--- FAIL: TestScannerHappy (0.00s)
panic: runtime error: index out of range [recovered]
	panic: runtime error: index out of range

goroutine 18 [running]:
testing.tRunner.func1(0xc4200b00f0)
	/usr/lib/go-1.10/src/testing/testing.go:742 +0x2f2
panic(0x142700, 0x210100)
	/usr/lib/go-1.10/src/runtime/panic.go:505 +0x26e
_/tmp/22.spdxSplit(0xc4200b6004, 0x0, 0xffc, 0x100000000000ffc, 0x4, 0xc4200b6000, 0x4, 0x1000, 0x0, 0x0)
	/tmp/22/scanner.go:43 +0x388
bufio.(*Scanner).Scan(0xc4200ae080, 0xc4200b6000)
	/usr/lib/go-1.10/src/bufio/scan.go:140 +0x640
_/tmp/22.TestScannerHappy(0xc4200b00f0)
	/tmp/22/scanner_test.go:29 +0xea
testing.tRunner(0xc4200b00f0, 0x16fe38)
	/usr/lib/go-1.10/src/testing/testing.go:777 +0xd8
created by testing.(*T).Run
	/usr/lib/go-1.10/src/testing/testing.go:824 +0x30e
exit status 2
FAIL	_/tmp/22	0.004s

What is interessting is that if I add a debug line into the scanner.go code like this:

       if start == len(data) {
                return start, nil, nil
        }
        fmt.Printf("start=%#v (%[1]T); len(data)=%#v (%[2]T); start == len(data): %#v (%[3]T) \n", start, len(data), start == len(data))

this produces:

ubuntu@clound01:/tmp/22$ go test -covermode=atomic
start=0 (int); len(data)=4 (int); start == len(data): false (bool) 
start=0 (int); len(data)=4 (int); start == len(data): false (bool) 
start=0 (int); len(data)=0 (int); start == len(data): false (bool) 

The last line looks suspicious - start == len(data) should be true here it seems.

go-test-coverage-atomic-s390x-with-debug.tar.gz

I also added the debug enabled script.

But this is just one instance of the failure, we see many more so it seems like something fundamental is wrong with coverage=atomic.

@mvdan mvdan changed the title Many tests on go1.10/s390x fail when -covermode=atomic is used cmd/compile: many tests on s390x fail when -covermode=atomic is used Mar 19, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

I've seen a couple of these "code isn't working as expected" before, and they tend to be compiler issues. This one likely has something to do with its s390x support.

Have you been able to try tip, i.e. the master branch?

/cc @mundaym

@mundaym

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

I had a quick look. I think the issue is that the LAA op (atomic add) is missing the clobberFlags=true property. The compiler is moving a conditional branch over the LAA op in this case, so it is branching to the wrong place.

@mvo5

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

I tested with:

$ sudo snap refresh go --edge
$ /snap/bin/go version
go version devel +b61b1d2 Sun Mar 18 16:53:53 2018 +0000 linux/s390x

and see the same issue.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 19, 2018

Change https://golang.org/cl/101455 mentions this issue: cmd/compile: mark LAA and LAAG as clobbering flags on s390x

@randall77 randall77 added this to the Go1.10.1 milestone Mar 19, 2018

@gopherbot gopherbot closed this in ae10914 Mar 20, 2018

@mundaym

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

Reopening for backport to Go 1.10.1.

@mundaym mundaym reopened this Mar 20, 2018

@mundaym

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

@mvo5 Thanks for the report, the issue should now be fixed at tip. Are you able to confirm?

@mvo5

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2018

Thanks for the super quick fix. I can confirm the fix works:

$ /snap/bin/go test -covermode=atomic
start=0 (int); len(data)=4 (int); start == len(data): false (bool) 
start=0 (int); len(data)=4 (int); start == len(data): false (bool) 
PASS
coverage: 84.2% of statements
ok  	_/tmp/22	0.002s
$ /snap/bin/go version
go version devel +9eb2194 Wed Mar 21 21:57:15 2018 +0000 linux/s390x
@andybons

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

CL 101455 OK for Go 1.10.1

@gopherbot

This comment has been minimized.

Copy link

commented Mar 27, 2018

Change https://golang.org/cl/102788 mentions this issue: [release-branch.go1.10] cmd/compile: mark LAA and LAAG as clobbering flags on s390x

gopherbot pushed a commit that referenced this issue Mar 29, 2018

[release-branch.go1.10] cmd/compile: mark LAA and LAAG as clobbering …
…flags on s390x

The atomic add instructions modify the condition code and so need to
be marked as clobbering flags.

Fixes #24449.

Change-Id: Ic69c8d775fbdbfb2a56c5e0cfca7a49c0d7f6897
Reviewed-on: https://go-review.googlesource.com/101455
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/102788
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Michael Munday <mike.munday@ibm.com>

@andybons andybons closed this Mar 29, 2018

@golang golang locked and limited conversation to collaborators Mar 29, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.