[Wasm64] Add support for threads #18705
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbc100
force-pushed
the
wasm64_pthreads
branch
from
ac08c6f to
2019e26
Compare
sbc100
added a commit
that referenced
this pull request
Feb 10, 2023
Logically the code make more sense here in processLibraryFunction, but its also important that the wasm64 wrapper is the outermost one. Without this the wasm64 adapter is applied to the inner, non-proxied function only. Split out from #18705
sbc100
force-pushed
the
wasm64_pthreads
branch
3 times, most recently
from
d828324 to
51a7fd7
Compare
sbc100
force-pushed
the
wasm64_pthreads
branch
from
51a7fd7 to
b9b2587
Compare
sbc100
added a commit
that referenced
this pull request
Feb 11, 2023
Split out from #18705 I noticed while working on #18705 that the exports returned by `instantiateWasm` (which is used in pthreads to load the wasm module) ere not the same (potentially wrapped/modified) exports that were created during `receiveInstance`. I guess someone else must have noticed this and added the extra call to `Asyncify.instrumentWasmExports`. This should not be necessary since `receiveInstance` already takes care of that.
sbc100
added a commit
that referenced
this pull request
Feb 11, 2023
Split out from #18705 I noticed while working on #18705 that the exports returned by `instantiateWasm` (which is used in pthreads to load the wasm module) ere not the same (potentially wrapped/modified) exports that were created during `receiveInstance`. I guess someone else must have noticed this and added the extra call to `Asyncify.instrumentWasmExports`. This should not be necessary since `receiveInstance` already takes care of that.
sbc100
added a commit
that referenced
this pull request
Feb 11, 2023
Split out from #18705 I noticed while working on #18705 that the exports returned by `instantiateWasm` (which is used in pthreads to load the wasm module) ere not the same (potentially wrapped/modified) exports that were created during `receiveInstance`. I guess someone else must have noticed this and added the extra call to `Asyncify.instrumentWasmExports`. This should not be necessary since `receiveInstance` already takes care of that.
sbc100
added a commit
that referenced
this pull request
Feb 13, 2023
Split out from #18705 I noticed while working on #18705 that the exports returned by `instantiateWasm` (which is used in pthreads to load the wasm module) ere not the same (potentially wrapped/modified) exports that were created during `receiveInstance`. I guess someone else must have noticed this and added the extra call to `Asyncify.instrumentWasmExports`. This should not be necessary since `receiveInstance` already takes care of that.
sbc100
added a commit
that referenced
this pull request
Feb 13, 2023
…18711) Split out from #18705 I noticed while working on #18705 that the exports returned by `instantiateWasm` (which is used in pthreads to load the wasm module) ere not the same (potentially wrapped/modified) exports that were created during `receiveInstance`. I guess someone else must have noticed this and added the extra call to `Asyncify.instrumentWasmExports`. This should not be necessary since `receiveInstance` already takes care of that.
sbc100
force-pushed
the
wasm64_pthreads
branch
from
b9b2587 to
1da0ceb
Compare
Testing wasm64 with threads requires a nightly build of node (since it depends on some very recent fixes on the v8 side).
sbc100
force-pushed
the
wasm64_pthreads
branch
from
1da0ceb to
a619e29
Compare
|
PTAL, this should be fairly straight forward review now! |
|
Reviewing. Since this is clearly a collection of several changes, can you add a commit description saying at a high level what those changes are? |
Funnily enough I already split out least 3 changes out of this one :) But yes I will add more color to the PR description. The one change I could still potentially split out is the testing under node canary. |
Done |
|
Yeah, splitting out changes when possible is good, but there will always be a place for more descriptive commit descriptions :) |
dschuff
approved these changes
Feb 15, 2023
|
Cool! What's still needed to support exceptions with wasm64? |
Its emscripten exceptions that are not supported.. so I think we decided no to prioritize it. Having said that, I think it would require changing something in the emscripten EH ABI from int32 to intptr width.. so it would require an llvm change IIRC. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing wasm64 with threads requires a nightly build of node (since it depends on some very recent fixes on the v8 side).
This change consists of the following:
__sigattributes to bunch of library functions that were missing them.create_wasm64_wrappers.makeDynCallused for thread entry pointI64_ADDopcode inwebassembly.pypthread_attr_tto uselongfor__smember (the__smember is used to hold pointer-sized things).