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

math: Sqrt() can generate invalid byte code for wasm #27961

Closed
justinclift opened this Issue Oct 2, 2018 · 15 comments

Comments

Projects
None yet
7 participants
@justinclift

justinclift commented Oct 2, 2018

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

$ go version
go version go1.11 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN="/home/jc/go/bin"
GOCACHE="/home/jc/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jc/git_repos:/home/jc/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=""
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-build142808779=/tmp/go-build -gno-record-gcc-switches"

What did you do?

From investigating a problem reported by @bobcob7 in the WebAssembly Slack channel, it's turned out to be a case of the Go compiler generating what seems to be wrong byte code for the math.Sqrt() function.

In that example code there are two lines that should do the same thing. One enabled (line 250), one commented out (line 251). In the repo as-is, the wasm runs ok.

However if line 250 is commented out, with the line after it then enabled, the byte code generated for wasm output seems wrong. Instead of running the same, Firefox gives an error in the javascript console:

CompileError: wasm validation error: at offset 1269295: type mismatch: expression has type i64 but expected f64

Chrome gives a different error, seemingly meaning the same thing:

CompileError: AsyncCompile: Compiling wasm function #1694 failed: f32.demote/f64[0] expected type f64, found get_local of type i64 @+1269409

After further drilling down, it's probably something to do with math.Sqrt()'s Go implementation using "unsafe" pieces:

But, no idea why the exact same values called from the same code (but done from a function) are making it go wrong.

Hopefully someone more familiar with the wasm byte code generation can take a look. It's probably not a hard fix. 😄

Note - As a thought, this seems like it might be a blocker for 1.11.x? Having any of the main math.* functions going wrong seems a tad non-optimal. 😉

What did you expect to see?

Working code running in the browser (eg at https://justinclift.github.io/wasmBug1/)

What did you see instead?

A compile error in the javascript console.

@agnivade agnivade changed the title from Math.Sqrt() can generate invalid byte code for wasm to math: Sqrt() can generate invalid byte code for wasm Oct 2, 2018

@agnivade agnivade added this to the Go1.12 milestone Oct 2, 2018

@agnivade

This comment has been minimized.

Show comment
Hide comment
@agnivade
Member

agnivade commented Oct 2, 2018

@justinclift

This comment has been minimized.

Show comment
Hide comment
@justinclift

justinclift Oct 2, 2018

Hmmm, looks like Sqrt is supposed to have some kind of direct wasm assembly output:

https://github.com/golang/go/blob/26957168c4c0cdcc7ca4f0b19d0eb19474d224ac/src/math/sqrt_wasm.s

So, really no idea what's going wrong here. 😄

justinclift commented Oct 2, 2018

Hmmm, looks like Sqrt is supposed to have some kind of direct wasm assembly output:

https://github.com/golang/go/blob/26957168c4c0cdcc7ca4f0b19d0eb19474d224ac/src/math/sqrt_wasm.s

So, really no idea what's going wrong here. 😄

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz
Member

bradfitz commented Oct 2, 2018

@neelance

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Oct 2, 2018

Member

I managed to reduce the test case to this:

package main

import "math"

type Vec2 [2]float64

func main() {
	var a Vec2
	a.A().B().C().D()
}

func (v Vec2) A() Vec2 {
	return Vec2{v[0], v[0]}
}

func (v Vec2) B() Vec2 {
	return Vec2{1.0 / v.D(), 0}
}

func (v Vec2) C() Vec2 {
	return Vec2{v[0], v[0]}
}

func (v Vec2) D() float64 {
	return math.Sqrt(v[0])
}

I couldn't reduce it any further. I'm still investigating. It seems to be related to inlining and autotmp values.

Member

neelance commented Oct 2, 2018

I managed to reduce the test case to this:

package main

import "math"

type Vec2 [2]float64

func main() {
	var a Vec2
	a.A().B().C().D()
}

func (v Vec2) A() Vec2 {
	return Vec2{v[0], v[0]}
}

func (v Vec2) B() Vec2 {
	return Vec2{1.0 / v.D(), 0}
}

func (v Vec2) C() Vec2 {
	return Vec2{v[0], v[0]}
}

func (v Vec2) D() float64 {
	return math.Sqrt(v[0])
}

I couldn't reduce it any further. I'm still investigating. It seems to be related to inlining and autotmp values.

@cherrymui

This comment has been minimized.

Show comment
Hide comment
@cherrymui

cherrymui Oct 3, 2018

Contributor

The offending instruction is

00027 (9) Get	R0
00028 (9) F64Store	""..autotmp_7-40(SP)

which tries to store an integer value (R0) as float64. This comes from spilling v86 (9) = I64AddConst <float64> [8] v2. This value looks problematic, since it is an integer operation with a float type.

This value is from lowering v86 (9) = OffPtr <float64> [8] v2, which is also problematic, since OffPtr should not be float typed. This seems coming from generic opt.

Contributor

cherrymui commented Oct 3, 2018

The offending instruction is

00027 (9) Get	R0
00028 (9) F64Store	""..autotmp_7-40(SP)

which tries to store an integer value (R0) as float64. This comes from spilling v86 (9) = I64AddConst <float64> [8] v2. This value looks problematic, since it is an integer operation with a float type.

This value is from lowering v86 (9) = OffPtr <float64> [8] v2, which is also problematic, since OffPtr should not be float typed. This seems coming from generic opt.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 3, 2018

Change https://golang.org/cl/139257 mentions this issue: cmd/compile: fix type of OffPtr in some optimization rules

gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139257 mentions this issue: cmd/compile: fix type of OffPtr in some optimization rules

@justinclift

This comment has been minimized.

Show comment
Hide comment
@justinclift

justinclift Oct 3, 2018

As a data point, that CL (patch set two) seems to fix the problem in testing here. When trying the original reported WebGL wasm code (that previously failed) Firefox now runs it instead.

Thanks heaps @neelance and @cherrymui. 😄

justinclift commented Oct 3, 2018

As a data point, that CL (patch set two) seems to fix the problem in testing here. When trying the original reported WebGL wasm code (that previously failed) Firefox now runs it instead.

Thanks heaps @neelance and @cherrymui. 😄

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 3, 2018

Member

@gopherbot, please cherry pick.

Member

bradfitz commented Oct 3, 2018

@gopherbot, please cherry pick.

@gopherbot gopherbot closed this in c96e3bc Oct 3, 2018

@justinclift

This comment has been minimized.

Show comment
Hide comment
@justinclift

justinclift Oct 3, 2018

Should we add this to 1.11.x too?

justinclift commented Oct 3, 2018

Should we add this to 1.11.x too?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 3, 2018

Member

@justinclift, yes, hence my comment immediately preceding yours. :)

Member

bradfitz commented Oct 3, 2018

@justinclift, yes, hence my comment immediately preceding yours. :)

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 3, 2018

Change https://golang.org/cl/139104 mentions this issue: [release-branch.go1.11] cmd/compile: fix type of OffPtr in some optimization rules

gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139104 mentions this issue: [release-branch.go1.11] cmd/compile: fix type of OffPtr in some optimization rules

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 3, 2018

Member

@gopehrbot wasn't behaving, though, so I created https://go-review.googlesource.com/c/go/+/139104 manually.

/cc @andybons @dmitshur

Member

bradfitz commented Oct 3, 2018

@gopehrbot wasn't behaving, though, so I created https://go-review.googlesource.com/c/go/+/139104 manually.

/cc @andybons @dmitshur

@justinclift

This comment has been minimized.

Show comment
Hide comment
@justinclift

justinclift Oct 3, 2018

Awesome, thanks @bradfitz. 😄

justinclift commented Oct 3, 2018

Awesome, thanks @bradfitz. 😄

gopherbot pushed a commit that referenced this issue Oct 3, 2018

[release-branch.go1.11] cmd/compile: fix type of OffPtr in some optim…
…ization rules

In some optimization rules the type of generated OffPtr was
incorrectly set to the type of the pointee, instead of the
pointer. When the OffPtr value is spilled, this may generate
a spill of the wrong type, e.g. a floating point spill of an
integer (pointer) value. On Wasm, this leads to invalid
bytecode.

Fixes #27961.

Change-Id: I5d464847eb900ed90794105c0013a1a7330756cc
Reviewed-on: https://go-review.googlesource.com/c/139257
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Richard Musiol <neelance@gmail.com>
(cherry picked from commit c96e3bc)
Reviewed-on: https://go-review.googlesource.com/c/139104
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 4, 2018

Member

@gopehrbot wasn't behaving, though, so I created https://go-review.googlesource.com/c/go/+/139104 manually.

/cc @andybons @dmitshur

@bradfitz, did you mean to say "backport" instead of "cherry pick"? See https://golang.org/wiki/gopherbot#backporting-issues and source code.

Member

dmitshur commented Oct 4, 2018

@gopehrbot wasn't behaving, though, so I created https://go-review.googlesource.com/c/go/+/139104 manually.

/cc @andybons @dmitshur

@bradfitz, did you mean to say "backport" instead of "cherry pick"? See https://golang.org/wiki/gopherbot#backporting-issues and source code.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 4, 2018

Member

Probably. Perhaps @gopherbot should reply when it doesn't understand something.

Member

bradfitz commented Oct 4, 2018

Probably. Perhaps @gopherbot should reply when it doesn't understand something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment