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

Closed
fxamacker opened this issue Jan 6, 2020 · 19 comments
Milestone

Comments

@fxamacker
Copy link

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

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

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

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

@fxamacker
Copy link
Author

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

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

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

@randall77
Copy link
Contributor

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

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

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

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

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

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

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

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

gopherbot pushed a commit that referenced this issue Feb 25, 2020
When converting from float32->float64->float32, any signal NaNs
get converted to quiet NaNs. Avoid that so using reflect.Value.Convert
between two float32 types keeps the signal bit of NaNs.

Update #36400

Change-Id: Ic4dd04c4be7189d2171d12b7e4e8f7cf2fb22bb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/213497
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@randall77
Copy link
Contributor

CL reverted, reopening.

@randall77 randall77 reopened this Feb 25, 2020
@randall77
Copy link
Contributor

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.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 4, 2020
Trying this CL again, with a fixed test that allows platforms
to disagree on the exact behavior of converting NaNs.

We store 32-bit floating point constants in a 64-bit field, by
converting that 32-bit float to 64-bit float to store it, and convert
it back to use it.

That works for *almost* all floating-point constants. The exception is
signaling NaNs. The round trip described above means we can't represent
a 32-bit signaling NaN, because conversions strip the signaling bit.

To fix this issue, just forbid NaNs as floating-point constants in SSA
form. This shouldn't affect any real-world code, as people seldom
constant-propagate NaNs (except in test code).

Additionally, NaNs are somewhat underspecified (which of the many NaNs
do you get when dividing 0/0?), so when cross-compiling there's a
danger of using the compiler machine's NaN regime for some math, and
the target machine's NaN regime for other math. Better to use the
target machine's NaN regime always.

Update #36400

Change-Id: Idf203b688a15abceabbd66ba290d4e9f63619ecb
Reviewed-on: https://go-review.googlesource.com/c/go/+/221790
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@gopherbot
Copy link

Change https://golang.org/cl/227860 mentions this issue: cmd/compile: prevent constant folding of +/- when result is NaN

gopherbot pushed a commit that referenced this issue Apr 10, 2020
Missed as part of CL 221790. It isn't just * and / that can make NaNs.

Update #36400
Fixes #38359

Change-Id: I3fa562f772fe03b510793a6dc0cf6189c0c3e652
Reviewed-on: https://go-review.googlesource.com/c/go/+/227860
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alberto Donizetti <alb.donizetti@gmail.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@golang golang locked and limited conversation to collaborators Apr 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants