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

mvo5 · 2018-03-19T15:02:01Z

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]

The text was updated successfully, but these errors were encountered:

mvdan · 2018-03-19T15:31:05Z

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 · 2018-03-19T16:36:49Z

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 · 2018-03-19T16:40:49Z

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 · 2018-03-19T16:51:10Z

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 · 2018-03-19T16:53:09Z

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 · 2018-03-19T17:43:02Z

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

mundaym · 2018-03-20T09:46:26Z

Reopening for backport to Go 1.10.1.

mundaym · 2018-03-20T11:49:02Z

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

mvo5 · 2018-03-22T09:50:39Z

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 · 2018-03-27T22:11:10Z

CL 101455 OK for Go 1.10.1

gopherbot · 2018-03-27T23:14:21Z

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

…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>

mvo5 mentioned this issue Mar 19, 2018

tests: add workaround for s390x failure snapcore/snapd#4872

Merged

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 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 19, 2018

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

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

gopherbot closed this as completed in ae10914 Mar 20, 2018

mundaym reopened this Mar 20, 2018

mundaym removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 23, 2018

andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 23, 2018

andybons mentioned this issue Mar 27, 2018

all: Go 1.10.1 release status #24563

Closed

andybons added CherryPickApproved Used during the release process for point releases and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 27, 2018

andybons closed this as completed Mar 29, 2018

golang locked and limited conversation to collaborators Mar 29, 2019

gopherbot added the FrozenDueToAge label Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mvo5 commented Mar 19, 2018 •

edited

mvdan commented Mar 19, 2018

mvo5 commented Mar 19, 2018

mvdan commented Mar 19, 2018

mundaym commented Mar 19, 2018

mvo5 commented Mar 19, 2018

gopherbot commented Mar 19, 2018

mundaym commented Mar 20, 2018

mundaym commented Mar 20, 2018

mvo5 commented Mar 22, 2018

andybons commented Mar 27, 2018

gopherbot commented Mar 27, 2018

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

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

Comments

mvo5 commented Mar 19, 2018 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

mvdan commented Mar 19, 2018

mvo5 commented Mar 19, 2018

mvdan commented Mar 19, 2018

mundaym commented Mar 19, 2018

mvo5 commented Mar 19, 2018

gopherbot commented Mar 19, 2018

mundaym commented Mar 20, 2018

mundaym commented Mar 20, 2018

mvo5 commented Mar 22, 2018

andybons commented Mar 27, 2018

gopherbot commented Mar 27, 2018

mvo5 commented Mar 19, 2018 •

edited

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

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