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

x/time/rate: Limiter depends on implementation-defined float to int conversion #18762

Open
AudriusButkevicius opened this issue Jan 23, 2017 · 11 comments
Milestone

Comments

@AudriusButkevicius
Copy link
Contributor

@AudriusButkevicius AudriusButkevicius commented Jan 23, 2017

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

Audrius@Audrius foo $ go version
go version go1.8rc2 windows/amd64

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

Audrius@Audrius foo $ go env
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Gohome
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\cygwin64\tmp\go-build503767043=/tmp/go-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2

What did you do?

Compile two binaries using the following:

package main

import (
        "golang.org/x/time/rate"
        "fmt"
)

func main() {
        r := rate.NewLimiter(0, 100)
        if r.Allow() {
                fmt.Println("broken")
        } else {
                fmt.Println("ok")
        }
}

Compile as:

Audrius@Audrius foo $ GOOS=linux GOARCH=arm GOARM=5 go build main.go; mv main main-arm
Audrius@Audrius foo $ GOOS=linux GOARCH=arm64 GOARM=5 go build main.go; mv main main-arm64

Run them on arm64 device (Pixel XL):

marlin:/data/data/com.nutomic.syncthingandroid.debug $ ./main-arm
broken
marlin:/data/data/com.nutomic.syncthingandroid.debug $ ./main-arm64
ok

Run the same code on amd64 device:

Audrius@Audrius foo $ go run main.go
broken

What did you expect to see?

The behaviour between the binaries on should be the same.
I suspect it's related to how floats are handled between the arches.

What did you see instead?

Binaries return different results based on how they were compiled.

@bradfitz bradfitz added this to the Go1.9 milestone Jan 23, 2017
@AudriusButkevicius AudriusButkevicius changed the title cmd/compile: Inconsistent float behaviour on arm64 cmd/compile: Inconsistent float behaviour on arm64 compared to arm/amd64 Jan 23, 2017
@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jan 23, 2017

@AudriusButkevicius Sorry, I'm confused: are you expecting "ok" or "broken"?

@AudriusButkevicius

This comment has been minimized.

Copy link
Contributor Author

@AudriusButkevicius AudriusButkevicius commented Jan 23, 2017

Well it depends.

It's either a bug in all arches apart from arm64, as the code does not behave as x/time/rate documents...
Or a bug in x/time/rate (or it's documentation) and arm64.

I've tested this on amd64, 386, arm, arm64.

I've raised #18763 in case it's arm64 being broken, in which case there should also be a fix for the library.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jan 23, 2017

Ok, thanks for the explanation.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jan 23, 2017

It seems that the difference comes from an +Inf to int64 conversion. On AMD64, it results in a big negative int64, whereas on ARM64 it results in a big positive int64. As far as I know, the spec doesn't require any particular value should results in this case. So it seems the compiler is ok.

The conversion is https://go.googlesource.com/time/+/master/rate/rate.go#364, which passed to waitDuration in https://go.googlesource.com/time/+/master/rate/rate.go#305, then compared with 0.

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Jan 23, 2017

This appears to be an artifact of the hardware conversion -- arm64 and ppc64 convert +/-Inf into the largest/smallest integer values; amd64 converts both +Inf and -Inf to the smallest integer value.

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Jan 23, 2017

@griesemer - do you have opinions on this?

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jan 23, 2017

The spec actually says (https://golang.org/ref/spec#Conversions)
"In all non-constant conversions involving floating-point or complex values, if the result type cannot represent the value the conversion succeeds but the result value is implementation-dependent."

So I think the library's implementation is problematic -- it depends on implementation-defined behavior, which may vary on different platforms.

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Jan 23, 2017

@dr2chase: @cherrymui said it all. The spec doesn't say anything specific here.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 24, 2017

@cherrymui Please avoid saying "undefined behavior", as that has a different meaning for people who read standards and language specs (https://en.wikipedia.org/wiki/Undefined_behavior). The problem here is that library depends on "implementation-defined behavior".

@josharian josharian changed the title cmd/compile: Inconsistent float behaviour on arm64 compared to arm/amd64 x/time/rate: Limiter depends on implementation-defined float to int conversion Jan 24, 2017
@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jan 24, 2017

Thanks. I edited my comment.

@cherrymui cherrymui removed their assignment Jan 24, 2017
@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jan 24, 2017

Unassign myself as I'm not familiar with this package.

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
6 participants
You can’t perform that action at this time.