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.

Sign up for GitHub

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

Jump to bottom

math: Pow(negativeZero, number_that_overflows_int64) produces incorrect result on arm64 #57465

Closed
dop251 opened this issue Dec 26, 2022 · 11 comments
Closed

math: Pow(negativeZero, number_that_overflows_int64) produces incorrect result on arm64 #57465

dop251 opened this issue Dec 26, 2022 · 11 comments
Labels
arch-arm64 FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin
Milestone
Backlog

Comments

@dop251
Copy link
Contributor

dop251 commented Dec 26, 2022

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

$ go version

go version go1.19.4 darwin/arm64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env

GO111MODULE=""

GOARCH="arm64"

GOBIN=""

GOCACHE="/Users/go/Library/Caches/go-build"

GOENV="/Users/go/Library/Application Support/go/env"

GOEXE=""

GOEXPERIMENT=""

GOFLAGS=""

GOHOSTARCH="arm64"

GOHOSTOS="darwin"

GOINSECURE=""

GOMODCACHE="/Users/go/go/pkg/mod"

GONOPROXY=""

GONOSUMDB=""

GOOS="darwin"

GOPATH="/Users/go/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/Users/go/godistr/go"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/Users/go/godistr/go/pkg/tool/darwin_arm64"

GOVCS=""

GOVERSION="go1.19.4"

GCCGO="gccgo"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="/dev/null"

GOWORK=""

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0_/3nm5fc5x7k1b8vlkcdwq5_040000gt/T/go-build133314584=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://go.dev/play/p/r5lmTzi5NYw

What did you expect to see?

0

What did you see instead?

-0

I did some digging, it turns out that isOddInt does a float64 to int64 conversion without checking for overflows. If an overflow occurs the behaviour is implementation-dependent and on amd64 it returns 0x80000000_00000000 (which is an even number), but on arm64 it returns '0x7fffffff_ffffffff which is an odd number.

I think for any values that overflow int64 the function should return true.
@seankhliao
Copy link
Member

Does seem to break the documented special case:

Pow(±0, y) = +0 for finite y > 0 and not an odd integer

@seankhliao seankhliao added OS-Darwin NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-arm64 labels Dec 26, 2022
@seankhliao
Copy link
Member

cc @griesemer @rsc

@cuonglm
Copy link
Member

cuonglm commented Dec 27, 2022 •
edited

For non-constants conversion involve floating-point, the result value is implementation dependent (see previously #56023), so I think the result is reasonable because y is interpreted as an odd integer on arm64 (after overflow):

Pow(±0, y) = ±0 for y an odd integer > 0

@dop251
Copy link
Contributor Author

dop251 commented Dec 27, 2022

I disagree. It explains why the result is different, however, nowhere in the specification does it say that y is converted to int64 during Pow. It only says it must be odd integer. 1e19 is not an odd integer.

Besides, it just makes sense from the math point of view.

@dop251
Copy link
Contributor Author

dop251 commented Dec 27, 2022

It is also very trivial to fix.

@cuonglm
Copy link
Member

cuonglm commented Dec 27, 2022

I disagree. It explains why the result is different, however, nowhere in the specification does it say that y is converted to int64 during Pow. It only says it must be odd integer. 1e19 is not an odd integer.

Besides, it just makes sense from the math point of view.

The param is a float64, so you need to convert it to an int64 to check whether it's odd or even, no?

@dop251
Copy link
Contributor Author

dop251 commented Dec 27, 2022

Not necessarily. For example I can do a comparison with float64 representations of MinInt64 and MaxInt64 to determine if the value overflows int64 and claim it's an even integer if it does.

In any case, it's implementation details, the specification does not say how it's determined that y is an odd integer, it also doesn't mention anything about this being implementation-dependent. It either needs to be specified or fixed, in my view.

@cuonglm
Copy link
Member

cuonglm commented Dec 27, 2022

Not necessarily. For example I can do a comparison with float64 representations of MinInt64 and MaxInt64 to determine if the value overflows int64 and claim it's an even integer if it does.

In any case, it's implementation details, the specification does not say how it's determined that y is an odd integer, it also doesn't mention anything about this being implementation-dependent. It either needs to be specified or fixed, in my view.

If it does overflow, how could you claim it's an even integer? 1e19 and 1e19-1 are both overflow, your claim:

I think for any values that overflow int64 the function should return true.

is not true.

@dop251
Copy link
Contributor Author

dop251 commented Dec 27, 2022

I can claim it because 1e19-1 has exactly the same float64 representation as 1e19 and the operand type (as you rightly mentioned earlier) is float64.

@randall77
Copy link
Contributor

I'm leaning towards fixing this, as the fix is simple. But it's kind of weird to use the terms "even" and "odd" for floats larger than 2^52, as the binary digit that matters is less than the precision that is represented.

What's the last odd number? 2^53-1, maybe?

dop251 added a commit to dop251/go that referenced this issue Dec 28, 2022
@dop251
math: handle int64 overflows when checking for odd integer exponents …
9a6d458 
…in Pow(-0, y)

The existing implementation does a float64 to int64 comversion in order to check whether the number is odd, however it does not check for overflows.
If an overflow occurs, the result is implementation-defined and while it happens to work on amd64 and i386, it produces an incorrect result on arm64
and possibly other architectures.

This change fixes that and also avoids calling isOddInt altogether if the base is +0, because it's unnecessary.

(I was considering avoiding the extra check if runtime.GOARCH is "amd64" or "i386", but I can't see this pattern being used anywhere outside the tests.
And having separate files with build tags just for isOddInt() seems like an overkill)

Fixes golang#57465
@dop251 dop251 mentioned this issue Dec 28, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/459815 mentions this issue: math: handle int64 overflows for odd integer exponents in Pow(-0, y)

@seankhliao seankhliao added this to the Backlog milestone Jan 20, 2023
@golang golang locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-arm64 FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

math: handle int64 overflows for odd integer exponents in Pow(-0, y) dop251/go
5 participants
@dop251 @cuonglm @randall77 @gopherbot @seankhliao