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

Jump to bottom

misc/wasm: possible bug in InstanceOf #36561

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

misc/wasm: possible bug in InstanceOf #36561

albrow opened this issue Jan 15, 2020 · 5 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@albrow
Copy link
Contributor

albrow commented Jan 15, 2020 •
edited

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

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

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

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 WebAssembly issues label Jan 15, 2020
@albrow
Copy link
Contributor Author

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.

This was referenced Jan 15, 2020
Closed
Merged
@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 The path to resolution is known, but the work has not been done. label Jan 16, 2020
@bradfitz
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
5 participants
@bradfitz @neelance @albrow @ALTree @gopherbot