cmd/compile: s390x floating point <-> integer conversions clobbering the condition code #39651

panlinux · 2020-06-17T13:27:30Z

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

$ go version
go version go1.14.4 linux/s390x

Does this issue reproduce with the latest release?

Yes

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

Ubuntu Groovy on s390x

go env

$ go env
GO111MODULE=""
GOARCH="s390x"
GOBIN=""
GOCACHE="/home/ubuntu/.cache/go-build"
GOENV="/home/ubuntu/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="s390x"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ubuntu/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.14"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.14/pkg/tool/linux_s390x"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build209301451=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Run example.go on s390x (works elsewhere): https://play.golang.org/p/CGraP0pNaJi

What did you expect to see?

No errors in the output, i.e.:

--- t:
{Easy! {1 [3 4]}}

--- t dump:
a: Easy!
b:
  c: 1
  d: [3, 4]


--- m:
map[a:Easy! b:map[c:1 d:[3 4]]]

--- m dump:
a: Easy!
b:
  c: 1
  d:
  - 3
  - 4

This code comes from the gopkg.in/yaml.v2 example in their README.md except I replaced "c: 1" with "c: 1.0" in the data variable.

What did you see instead?

ubuntu@groovy-s390x:~$ ./example 
2020/06/17 13:14:58 error: yaml: unmarshal errors:
  line 4: cannot unmarshal !!float `1.0` into int

Initially I thought it was a bug in gopkg.in/yaml.v2, and filed an issue there, but I suspect something in go itself now, because either of these silly changes make the code work:

Just printf in https://github.com/go-yaml/yaml/blob/v2/decode.go#L502, for debugging purposes, made it work:

--- a/decode.go
+++ b/decode.go
@@ -500,6 +500,7 @@ func (d *decoder) scalar(n *node, out reflect.Value) bool {
                                return true
                        }
                case float64:
+                       fmt.Printf("DEBUG: int64(resolved) = %d\n", int64(resolved))
                        if resolved <= math.MaxInt64 && !out.OverflowInt(int64(resolved)) {
                                out.SetInt(int64(resolved))
                                return true

Output:

ubuntu@groovy-s390x:~$ ./example 
DEBUG: int64(resolved) = 1
--- t:
{Easy! {1 [3 4]}}

--- t dump:
a: Easy!
b:
  c: 1
  d: [3, 4]


--- m:
map[a:Easy! b:map[c:1 d:[3 4]]]

--- m dump:
a: Easy!
b:
  c: 1
  d:
  - 3
  - 4

Or this change:

--- a/decode.go
+++ b/decode.go
@@ -500,7 +500,8 @@ func (d *decoder) scalar(n *node, out reflect.Value) bool {
                                return true
                        }
                case float64:
-                       if resolved <= math.MaxInt64 && !out.OverflowInt(int64(resolved)) {
+                       is_overflow := out.OverflowInt(int64(resolved))
+                       if resolved <= math.MaxInt64 && !is_overflow {
                                out.SetInt(int64(resolved))
                                return true
                        }

Also makes it work and produces the correct output:

ubuntu@groovy-s390x:~$ rm example
ubuntu@groovy-s390x:~$ go build example.go
ubuntu@groovy-s390x:~$ ./example 
--- t:
{Easy! {1 [3 4]}}

--- t dump:
a: Easy!
b:
  c: 1
  d: [3, 4]


--- m:
map[a:Easy! b:map[c:1 d:[3 4]]]

--- m dump:
a: Easy!
b:
  c: 1
  d:
  - 3
  - 4

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

panlinux · 2020-06-17T13:46:54Z

The Ubuntu golang-1.14 package carries these patches: https://git.launchpad.net/ubuntu/+source/golang-1.14/tree/debian/patches:

panlinux · 2020-06-17T14:10:45Z

I downloaded https://dl.google.com/go/go1.14.4.linux-s390x.tar.gz and with that binary the example.go code also fails in the same way on s390x.

mundaym · 2020-06-17T14:14:00Z

Thanks for the bug report @panlinux. I've seen a bug in the compiler show up in yaml.v3 before as #38195. That bug should be fixed in go 1.14.4 though, so its probably something different. I'll take a look.

gopherbot · 2020-06-18T12:29:40Z

Change https://golang.org/cl/238628 mentions this issue: cmd/compile: mark s390x int <-> float conversions as clobbering flags

mundaym · 2020-06-18T13:13:51Z

Smaller reproducer:

// Test that float -> integer conversion doesn't clobber
// flags.

package main

//go:noinline
func f(x, y float64, a, b *bool, r *int64) {
        *a = x < y    // set flags
        *r = int64(x) // clobber flags
        *b = x == y   // use flags
}

func main() {
        var a, b bool
        var r int64
        f(1, 1, &a, &b, &r)
        if a || !b {
                panic("comparison incorrect")
        }
}

The floating point <-> integer conversion instructions set the condition code but the compiler wasn't aware of that behavior causing an incorrect condition code to be seen by a load-on-condition instruction. CL 238628 fixes the issue. Sorry about that.

mundaym · 2020-06-18T13:14:28Z

I'll take a look at backporting the fix once the CL has been approved.

panlinux · 2020-06-18T13:44:52Z

Thanks a lot @mundaym !

mundaym · 2020-06-18T16:42:05Z

@gopherbot please consider this for a backport

This is a bug that affects the control flow of a program running on s390x and therefore can result in it behaving incorrectly. The fix is simple and safe and only affects s390x.

gopherbot · 2020-06-18T16:42:15Z

Backport issue(s) opened: #39689 (for 1.13), #39690 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

gopherbot · 2020-07-13T09:32:53Z

Change https://golang.org/cl/242237 mentions this issue: [release-branch.go1.14] cmd/compile: mark s390x int <-> float conversions as clobbering flags

gopherbot · 2020-07-13T09:33:31Z

Change https://golang.org/cl/242238 mentions this issue: [release-branch.go1.13] cmd/compile: mark s390x int <-> float conversions as clobbering flags

…ions as clobbering flags These conversion instructions set the condition code and so should be marked as clobbering flags. Updates #39651. Fixes #39690. Change-Id: I1e3f2cf33337128d321b52ac72f46d1b8798ebd9 Reviewed-on: https://go-review.googlesource.com/c/go/+/242237 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>

mundaym self-assigned this Jun 17, 2020

mundaym added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 17, 2020

mundaym added arch-s390x Issues solely affecting the s390x architecture. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 18, 2020

mundaym added this to the Go1.16 milestone Jun 18, 2020

gopherbot closed this as completed in 377c153 Jun 18, 2020

mundaym changed the title ~~s390x evaluation weirdness with 1.13, 1.14. Works with 1.10, 1.12~~ cmd/compile: s390x floating point <-> integer conversions clobbering the condition code Jun 18, 2020

This was referenced Jun 18, 2020

cmd/compile: s390x floating point <-> integer conversions clobbering the condition code [1.13 backport] #39689

Closed

cmd/compile: s390x floating point <-> integer conversions clobbering the condition code [1.14 backport] #39690

Closed

dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 10, 2020

dmitshur modified the milestones: Go1.16, Go1.15 Jul 10, 2020

This comment has been minimized.

Sign in to view

golang locked and limited conversation to collaborators Jul 13, 2021

gopherbot added the FrozenDueToAge label Jul 13, 2021

rsc unassigned mundaym Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: s390x floating point <-> integer conversions clobbering the condition code #39651

cmd/compile: s390x floating point <-> integer conversions clobbering the condition code #39651

panlinux commented Jun 17, 2020

panlinux commented Jun 17, 2020

panlinux commented Jun 17, 2020

mundaym commented Jun 17, 2020

gopherbot commented Jun 18, 2020

mundaym commented Jun 18, 2020

mundaym commented Jun 18, 2020

panlinux commented Jun 18, 2020

mundaym commented Jun 18, 2020

gopherbot commented Jun 18, 2020

This comment has been minimized.

gopherbot commented Jul 13, 2020

gopherbot commented Jul 13, 2020

cmd/compile: s390x floating point <-> integer conversions clobbering the condition code #39651

cmd/compile: s390x floating point <-> integer conversions clobbering the condition code #39651

Comments

panlinux commented Jun 17, 2020

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?

panlinux commented Jun 17, 2020

panlinux commented Jun 17, 2020

mundaym commented Jun 17, 2020

gopherbot commented Jun 18, 2020

mundaym commented Jun 18, 2020

mundaym commented Jun 18, 2020

panlinux commented Jun 18, 2020

mundaym commented Jun 18, 2020

gopherbot commented Jun 18, 2020

This comment has been minimized.

gopherbot commented Jul 13, 2020

gopherbot commented Jul 13, 2020

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

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