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

Cant generate typescript bindings using bazel for memory64 with embind #1494

Closed
sachitv opened this issue Nov 28, 2024 · 5 comments · Fixed by emscripten-core/emscripten#23051

Comments

@sachitv
Copy link

sachitv commented Nov 28, 2024

I was attempting to build typescript bindings in bazel for MEMORY64=2 when I noticed it didn't build since the current version of node in bazel (16.6.2 -> see below) didn't seem to work.

node_repositories(
node_version = "16.6.2",
)

I upgraded the dependencies for rules_nodejs and build_bazel_rules_nodejs to 5.8.5 and thereafer used the most recent version of node from there 20.14.0 and thereafter I was able to get unblocked.

The failure I noticed was something like this:

Aborted(CompileError: WebAssembly.instantiate(): Compiling function #79 failed: memory.copy[2] expected type i32, found local.get of type i64 @+5668)
/private/var/folders/85/bk0b_v_94y77_2jlmnybshlw0000gn/T/emscripten_temp/tsgen.js:531
  var e = new WebAssembly.RuntimeError(what);
          ^

RuntimeError: Aborted(CompileError: WebAssembly.instantiate(): Compiling function #79 failed: memory.copy[2] expected type i32, found local.get of type i64 @+5668)

Reproduction:

This is a commit / branch you can use to reproduce this error. After you cd into test-external please run bazel build //:hello-embind-wasm

Possible Fix:

Here's my possible fix for this issue: 6b76de3

Please let me know if it would make sense to make a pull request for this change. Given the scope reduction of rules_nodejs i'm uncertain if this is the right path to take given efforts underway in #1487. This is sort of a band aid for me at the moment to get my project to emit these definitions.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 2, 2024

It looks like the memory.copy instruction is not being correctly lowered by the binaryen --memory64-lowering pass.

@kripken
Copy link
Member

kripken commented Dec 2, 2024

@sbc100 That part of the pass looks valid to me:

https://github.com/WebAssembly/binaryen/blob/main/src/passes/Memory64Lowering.cpp#L120-L124

So I do think it might be the node.js version difference that explains this. Did the memory64 spec change on memory.copy between node 16 and 20, perhaps?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 2, 2024

This is -sMEMORY64=2 which means that result should not be using any memory64 features at all.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 2, 2024

There were some recent changes here: https://github.com/emscripten-core/emscripten/blob/b88d47915acd02adbd9bf438d2db714048d95b1b/system/lib/libc/emscripten_memcpy_bulkmem.S#L9-L13. Could there be an issue with the lowering pass?

@kripken
Copy link
Member

kripken commented Dec 2, 2024

@sbc100 Hmm, is that .S file linked in after memory64 lowering, somehow? But I don't think that's possible.

My current guess is that MEMORY64=2 + --emit-tsd is broken somehow, like perhaps --emit-tsd inhibits the memory64 lowering pass? @brendandahl does that ring a bell?

Otherwise @sachitv can you perhaps reduce this testcase to something simpler that does not use bazel? That would be easier for us to debug. Specifically, we need to see the link command in detail (and I don't know how to get bazel to output that). A small cpp file + a link command would be ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants