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: s390x floating point <-> integer conversions clobbering the condition code #39651

Closed
panlinux opened this issue Jun 17, 2020 · 12 comments
Assignees
Milestone

Comments

@panlinux
Copy link

@panlinux panlinux commented Jun 17, 2020

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:

  1. 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
  1. 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
@panlinux
Copy link
Author

@panlinux panlinux commented Jun 17, 2020

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
Copy link
Member

@mundaym mundaym commented Jun 17, 2020

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.

@mundaym mundaym self-assigned this Jun 17, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 18, 2020

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

@mundaym
Copy link
Member

@mundaym mundaym commented Jun 18, 2020

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
Copy link
Member

@mundaym mundaym commented Jun 18, 2020

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

@mundaym mundaym added this to the Go1.16 milestone Jun 18, 2020
@panlinux
Copy link
Author

@panlinux panlinux commented Jun 18, 2020

Thanks a lot @mundaym !

@gopherbot gopherbot closed this in 377c153 Jun 18, 2020
@mundaym 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
@mundaym
Copy link
Member

@mundaym mundaym commented Jun 18, 2020

@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
Copy link

@gopherbot gopherbot commented Jun 18, 2020

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.

@dmitshur dmitshur added the NeedsFix label Jul 10, 2020
@dmitshur dmitshur modified the milestones: Go1.16, Go1.15 Jul 10, 2020
@gopherbot

This comment has been hidden.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 13, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 13, 2020

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

gopherbot pushed a commit that referenced this issue Aug 21, 2020
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.