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

misc/wasm: possible bug in InstanceOf #36561

Open
albrow opened this issue Jan 15, 2020 · 5 comments
Open

misc/wasm: possible bug in InstanceOf #36561

albrow opened this issue Jan 15, 2020 · 5 comments
Assignees
Milestone

Comments

@albrow
Copy link
Contributor

@albrow albrow commented Jan 15, 2020

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

$ go version
go version go1.13.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes, judging by the contents of the master branch.

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

go env Output
$ go env
O111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/alex/Library/Caches/go-build"
GOENV="/Users/alex/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/alex/programming/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/alex/.go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/alex/.go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/alex/programming/go-mod/0x-mesh/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/h1/vhbpm31925nd0whbv4qd8mr00000gn/T/go-build550535634=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I am converting wasm_exec.js to TypeScript for bundling in a TypeScript library. I noticed this compiler error:

Screen Shot 2020-01-14 at 4 24 29 PM

What did you expect to see?

I don't expect this file to compile since it wasn't written with TypeScript in mind. I was surprised by this particular error because it looks like a type mismatch.

I think there might be a mistake on this line. I believe it is supposed to be written as:
(Update: Sorry, I realized my suggestion is probably not correct).

What did you see instead?

The code doesn't compile in TypeScript, I think due to a mismatched parentheses. If that's true, I believe this would result in a bug/incorrect behavior in syscall/js.InstanceOf.

@albrow

This comment has been minimized.

Copy link
Contributor Author

@albrow albrow commented Jan 15, 2020

It's also possible this is a quirk of the TypeScript compiler and the code is correct. The TypeScript compiler seems to think the signature for setUint8 is:

setUint8(byteOffset: number, value: number): void
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Jan 15, 2020

Can you just add a ternary to appease TypeScript?

diff --git a/misc/wasm/wasm_exec.js b/misc/wasm/wasm_exec.js
index bb66cf254d..5ac4032993 100644
--- a/misc/wasm/wasm_exec.js
+++ b/misc/wasm/wasm_exec.js
@@ -440,7 +440,7 @@
 
                                        // func valueInstanceOf(v ref, t ref) bool
                                        "syscall/js.valueInstanceOf": (sp) => {
-                                               this.mem.setUint8(sp + 24, loadValue(sp + 8) instanceof loadValue(sp + 16));
+                                               this.mem.setUint8(sp + 24, (loadValue(sp + 8) instanceof loadValue(sp + 16)) ? 1 : 0);
                                        },
 
                                        // func copyBytesToGo(dst []byte, src ref) (int, bool)
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 15, 2020

Change https://golang.org/cl/214944 mentions this issue: misc/wasm: avoid implicit boolean to number conversion

@bradfitz bradfitz changed the title Possible bug in InstanceOf in misc/wasm/wasm_exec.js misc/wasm: possible bug in InstanceOf Jan 15, 2020
@bradfitz bradfitz added the Arch-Wasm label Jan 15, 2020
@albrow

This comment has been minimized.

Copy link
Contributor Author

@albrow albrow commented Jan 15, 2020

@bradfitz Thanks for the reply. Ah I see. Seems like you were relying on type coercion to convert false to 0 and true to 1. So this is not a bug.

Still, it is generally considered bad practice in JavaScript to rely on this behavior, and there's a reason the TypeScript compiler complains about it. I was going to suggest a change, but I already see there's a proposed CL to fix it. Feel free to close this issue if you want.

@ALTree ALTree added this to the Backlog milestone Jan 16, 2020
@bradfitz bradfitz assigned bradfitz and unassigned neelance Jan 16, 2020
@bradfitz bradfitz modified the milestones: Backlog, Go1.15 Jan 16, 2020
@bradfitz bradfitz added the NeedsFix label Jan 16, 2020
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Jan 16, 2020

@albrow, I'll submit my change when the Go 1.15 tree opens.

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