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 · 13 comments
Milestone

Comments

@AudriusButkevicius
Copy link
Contributor

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

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

@AudriusButkevicius
Copy link
Contributor Author

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

Ok, thanks for the explanation.

@cherrymui
Copy link
Member

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

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

@griesemer - do you have opinions on this?

@cherrymui
Copy link
Member

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

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

@ianlancetaylor
Copy link
Contributor

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

Thanks. I edited my comment.

@cherrymui cherrymui removed their assignment Jan 24, 2017
@cherrymui
Copy link
Member

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

@bigsheeper
Copy link

bigsheeper commented Aug 15, 2022

As the documentation of Limit mentioned:

// A zero Limit allows no events.

The expected behavior should be not allowed, here's a more detailed test:

package main

import "time"

func main() {
	limiter := NewLimiter(0, 1)
	limiter.AllowN(time.Now(), 1)
	for i := 0; i < 10000; i++ {
		allow := limiter.AllowN(time.Now(), 1)
		if allow {
			panic("should not happen")
		}
	}
}

@bigsheeper
Copy link

bigsheeper commented Aug 15, 2022

And this has been fixed in x/time/rate package: https://github.com/golang/time/blob/e5dcc9cfc0b9553953e355dde5bdf4ff9f82f742/rate/rate.go#L405

Use the latest version could solve this issue, for example:

golang.org/x/time v0.0.0-20220722155302-e5dcc9cfc0b9

Or here's a workaround for this issue:

limiter := NewLimiter(math.SmallestNonzeroFloat64, 1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants