cmd/compile: constant propagation in compiler converts signaling NaN to quiet NaN
fxamacker opened this issue · 19 comments
What version of Go are you using (go version)?
$ go version go version go1.12.12 linux/amd64
Does this issue reproduce with the latest release?
Yes, same result with both go1.12.12 and go 1.13.5.
What operating system and processor architecture are you using (go env)?
go env Output
$ go env GOARCH="amd64" GOBIN="" GOCACHE="/home/user/.cache/go-build" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOOS="linux" GOPATH="/home/user/go" GOPROXY="" GORACE="" GOROOT="/usr/local/go" GOTMPDIR="" GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64" GCCGO="gccgo" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="/dev/null" 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 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build193953141=/tmp/go-build -gno-record-gcc-switches"
What did you do?
- Create a
reflect.Valueobject from float32 sNaN. - Use
Value.Float()on float32 sNaN, but it unexpectedly returns float64 qNaN.
This is different behavior than casting float32 sNaN to a float64, which correctly returns float64 sNaN.
This bug is blocking fxamacker/cbor#91. See fxamacker/cbor#93 for more info. This bug may be related to #36399.
https://play.golang.org/p/T7orv6p_C6h
package main
import (
"fmt"
"math"
"reflect"
)
func main() {
// Create a float32 signalling Nan.
f32 := math.Float32frombits(0x7f800001)
// Create a reflect.Value object from f32.
v := reflect.ValueOf(f32)
// Get its underlying floating-point value as float64.
f64 := v.Float()
// Returned float64 value has quiet-bit on.
u64 := math.Float64bits(f64)
if (u64 & 0x8000000000000) != 0 {
fmt.Println("Want sNaN, got qNaN")
}
}
What did you expect to see?
Value.Float() of float32 sNaN should return float64 sNaN.
This is expected because casting float32 sNaN to float64 returns sNaN.
What did you see instead?
Value.Float() of float32 sNaN returns float64 qNaN instead.
This is different result from casting float32 sNaN to float64.
Here's a reproducer, that shouldn't really fail:
package main
import (
"fmt"
"math"
"reflect"
)
func main() {
x := math.Float32frombits(0x7f800001)
v := reflect.ValueOf(x)
y := v.Interface().(float32)
fmt.Printf("%x %x\n", math.Float32bits(x), math.Float32bits(y))
}
It looks like this is just a constant-folding bug in the compiler.
When I rewrite it to not be able to constant propagate the constant, the bug goes away:
package main
import (
"fmt"
"math"
"reflect"
)
func main() {
x := uint32(0x7f800001)
y := f(x)
fmt.Printf("%x %x\n", x, math.Float32bits(y))
}
//go:noinline
func f(x uint32) float32 {
return reflect.ValueOf(math.Float32frombits(x)).Interface().(float32)
}
I think this happens because we represent constants internally as float64, even when they are representing a float32 constant. So we run into #36399. Maybe we should change the internal representation for Const32f to avoid this problem.
This has been an issue since 1.10, and there is an easy workaround, so not worth putting into 1.14.
Change https://golang.org/cl/213477 mentions this issue: cmd/compile: don't allow NaNs in floating-point constant ops
@randall77 Thanks for looking into this.
Although the workaround works for some scenarios, it isn't working for mine.
I'm writing a generic CBOR encoder/decoder in Go that can encode floats to the smallest floating-point type that preserves original value (including sNaN). Other languages like C have a generic CBOR library that can do this.
In the following code:
-
I can't use
Value.Interface()as proposed in the workaround because I need to handle user defined types without knowing type names in advance like myFloat32. -
Even though myFloat32's underlying type is float32, I can't specify float32 in the type assertion. So
func f2()fails to compile. -
I can't use the
Value.Float()function infunc f3()because it gives me a modified value if the original is sNaN on linux_amd64. I don't know if the same happens on ARMv8, etc.
Are there any workarounds I can use with Go 1.12+ until this is fixed in Go 1.5?
package main
import (
"fmt"
"math"
"reflect"
)
type myFloat32 float32
func main() {
x := uint32(0x7f800001)
y1 := f1(x)
fmt.Printf("f1: %x %x\n", x, math.Float32bits(y1))
// This fails to compile, see func f2()
//y2 := f2(x)
//fmt.Printf("f2: %x %x\n", x, math.Float32bits(y2))
y3 := f3(x)
fmt.Printf("f3: %x %x\n", x, math.Float32bits(y3))
}
func f1(x uint32) float32 {
f32 := math.Float32frombits(x)
return reflect.ValueOf(f32).Interface().(float32)
}
/*
// This fails to compile
func f2(x uint32) float32 {
f32 := myFloat32(math.Float32frombits(x))
return reflect.ValueOf(f32).Interface().(float32)
}
*/
func f3(x uint32) float32 {
f32 := math.Float32frombits(x)
return float32(reflect.ValueOf(f32).Float()) // this returns float64 that modified sNaN to qNaN
}
This compiles and runs:
func f2(x uint32) float32 {
f32 := myFloat32(math.Float32frombits(x))
return reflect.ValueOf(f32).Convert(reflect.TypeOf(float32(0))).Interface().(float32)
}
Unfortunately it converts 32->64->32 under the covers (inside Convert).
Maybe we could fix that.
Change https://golang.org/cl/213497 mentions this issue: reflect: when Converting between float32s, don't lose signal NaNs
I'm not sure what workarounds you might do.
For the constant propagation issue, the workaround is just to put the constant in a global variable, so that the compiler doesn't think it is really constant.
For the reflect issue, you might need to delve into unsafe to get the underlying value without conversion.
@fxamacker please keep in mind Go's unsafe package warns:
Packages that import unsafe may be non-portable and are not protected by the Go 1 compatibility guidelines.
Not using unsafe is also something your library mentions repeatedly as an advantage.
@randall77 Thanks again for spending time on this issue and suggesting workarounds.
- The constant propagation workaround is easy and I can use it right away.
- Is there any chance your fix for the
reflectpackage to be included in Go 1.14? I can't require Go 1.15 as a minimum requirement until Go 1.16+ which is 1+ years away.
I can't use unsafe as a workaround because it's been a design goal to avoid unsafe -- it's mentioned half-dozen times as a benefit and there's even a pretty gold medal for it my library's slideshow:
Early adopters of my fairly new CBOR library are primarily in the field of cryptography and security, so I think avoiding unsafe was a factor in their decision to choose my library.
I'll ask around and see what other people think. Don't get your hopes up. From https://github.com/golang/go/wiki/Go-Release-Cycle, about the freeze:
This part of the release cycle is focused on improving the quality of the release, by testing it and fixing bugs that are found. However, every fix must be evaluated to balance the benefit of a possible fix against the cost of now having not as well tested code (the fix) in the release. Early in the release cycle, the balance tends toward accepting a fix. Late in the release cycle, the balance tends toward rejecting a fix, unless a case can be made that the fix is both low risk and high reward.
The fix is pretty low risk. But I don't think it qualifies as high reward. Of course, reasonable people can disagree about that...
@randall77 Thanks so much! Please let me know.
In case it helps, CTAP2 Canonical CBOR (used by FIDO2, W3C WebAuthn) specifies:
The representations of any floating-point values are not changed.
W3C requires WebAuthn to use CTAP2 Canonical CBOR:
All CBOR encoding performed by the members of the above conformance classes MUST be done using the CTAP2 canonical CBOR encoding form.
https://www.w3.org/TR/webauthn/
Edit: added quote from WebAuthn and link to W3C WebAuthn
I asked around, and I don't think anyone is interested in adopting the reflect change for 1.14. It's just too late in the cycle. The release candidate will hopefully go out this week.
I did think of a possible workaround - if you can get an addressable reflect.Value containing the float32, you can get its value:
package main
import (
"fmt"
"math"
"reflect"
)
type myFloat32 float32
var qNaN32 = myFloat32(math.Float32frombits(0x7fc00000))
var sNaN32 = myFloat32(math.Float32frombits(0x7f800001))
func main() {
v := reflect.ValueOf(&sNaN32).Elem()
w := reflect.ValueOf(&qNaN32).Elem()
fmt.Printf("%x\n", bits(v))
fmt.Printf("%x\n", bits(w))
}
// v must be an addressable value with underlying type float32.
func bits(v reflect.Value) uint32 {
return math.Float32bits(v.Addr().Convert(reflect.TypeOf((*float32)(nil))).Elem().Interface().(float32))
}
Never mind, addressability is not required, as we can make an addressable copy. You can do this:
package main
import (
"fmt"
"math"
"reflect"
)
type myFloat32 float32
var qNaN32 = myFloat32(math.Float32frombits(0x7fc00000))
var sNaN32 = myFloat32(math.Float32frombits(0x7f800001))
func main() {
v := reflect.ValueOf(sNaN32)
w := reflect.ValueOf(qNaN32)
fmt.Printf("%x\n", bits(v))
fmt.Printf("%x\n", bits(w))
}
func bits(v reflect.Value) uint32 {
p := reflect.New(v.Type())
p.Elem().Set(v)
return math.Float32bits(p.Convert(reflect.TypeOf((*float32)(nil))).Elem().Interface().(float32))
}
@randall77 your workaround works great! 👍
I really appreciate your time and I don't know what to say except that you set the bar really high for open source projects.
CL reverted, reopening.
The reflect fix doesn't work on the 387 port. The 387 port can't even load a float into a register and store it right back to memory without squashing the signaling bit.
Others have run into this: samtools/hts-specs#145
I'm inclined to just punt on 387. I think all the other ports are ok in this regard.
To fix on 387, I think we'd have to keep parallel registers somehow. For example, a floating point load would have to be a fild (load integer) and then a fld (load float), to two different registers. Then we'd have to keep track of which one was canonical (the integer one, if no arithmetic had been done yet, or the float one, if arithmetic had been done) so we'd know which one to store.
Doesn't seem worth the effort.
Change https://golang.org/cl/221790 mentions this issue: cmd/compile: don't allow NaNs in floating-point constant ops
Change https://golang.org/cl/221792 mentions this issue: reflect: when Converting between float32s, don't lose signal NaNs
Change https://golang.org/cl/227860 mentions this issue: cmd/compile: prevent constant folding of +/- when result is NaN