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: constant propagation in compiler converts signaling NaN to quiet NaN #36400

Open
fxamacker opened this issue Jan 6, 2020 · 14 comments
Open
Assignees
Milestone

Comments

@fxamacker
Copy link

@fxamacker fxamacker commented Jan 6, 2020

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.Value object 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.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 6, 2020

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.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 6, 2020

This has been an issue since 1.10, and there is an easy workaround, so not worth putting into 1.14.

@randall77 randall77 added this to the Go1.15 milestone Jan 6, 2020
@randall77 randall77 self-assigned this Jan 6, 2020
@randall77 randall77 changed the title reflect: Value.Float() turns float32 sNaN into float64 qNaN cmd/compile: constant propagation in compiler converts signaling NaN to quiet NaN Jan 6, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 6, 2020

Change https://golang.org/cl/213477 mentions this issue: cmd/compile: don't allow NaNs in floating-point constant ops

@fxamacker

This comment has been minimized.

Copy link
Author

@fxamacker fxamacker commented Jan 6, 2020

@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 in func 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
}
@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 6, 2020

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.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 6, 2020

Change https://golang.org/cl/213497 mentions this issue: reflect: when Converting between float32s, don't lose signal NaNs

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 6, 2020

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.

@x448

This comment has been minimized.

Copy link

@x448 x448 commented Jan 7, 2020

@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.

@fxamacker

This comment has been minimized.

Copy link
Author

@fxamacker fxamacker commented Jan 7, 2020

@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 reflect package 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:

image

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.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 7, 2020

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...

@fxamacker

This comment has been minimized.

Copy link
Author

@fxamacker fxamacker commented Jan 7, 2020

@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.

https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.pdf

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

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 7, 2020

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))
}
@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jan 7, 2020

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))
}
@fxamacker

This comment has been minimized.

Copy link
Author

@fxamacker fxamacker commented Jan 8, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.