-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add new proxying modes foo__proxy: 'abort' and foo__proxy: 'abort_debug' #22648
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
Changes from all commits
d30d432
b2bac1e
bd6efe7
5f402cd
e753f21
382c420
1891f48
e6d7eb6
9f59913
1f2992a
5564586
725615c
5f0a9ef
5838805
bb7a5fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import { | |
| ATPRERUNS, | ||
| ATMAINS, | ||
| ATPOSTRUNS, | ||
| ENVIRONMENT_IS_WORKER_THREAD, | ||
| defineI64Param, | ||
| indentify, | ||
| makeReturn64, | ||
|
|
@@ -437,39 +438,38 @@ function(${args}) { | |
|
|
||
| const proxyingMode = LibraryManager.library[symbol + '__proxy']; | ||
| if (proxyingMode) { | ||
| if (proxyingMode !== 'sync' && proxyingMode !== 'async' && proxyingMode !== 'none') { | ||
| throw new Error(`Invalid proxyingMode ${symbol}__proxy: '${proxyingMode}' specified!`); | ||
| const possibleProxyingModes = ['sync', 'async', 'none', 'abort', 'abort_debug']; | ||
| if (!possibleProxyingModes.includes(proxyingMode)) { | ||
| throw new Error(`Invalid proxyingMode ${symbol}__proxy: '${proxyingMode}' specified! Possible modes: ${possibleProxyingModes.join(',')}`); | ||
| } | ||
| if (SHARED_MEMORY && proxyingMode != 'none') { | ||
| const sync = proxyingMode === 'sync'; | ||
| if (PTHREADS) { | ||
| snippet = modifyJSFunction(snippet, (args, body, async_, oneliner) => { | ||
| if (oneliner) { | ||
| body = `return ${body}`; | ||
| snippet = modifyJSFunction(snippet, (args, body, async_, oneliner) => { | ||
| if (oneliner) { | ||
| body = `return ${body}`; | ||
| } | ||
| const rtnType = sig && sig.length ? sig[0] : null; | ||
| const proxyFunc = | ||
| MEMORY64 && rtnType == 'p' ? 'proxyToMainThreadPtr' : 'proxyToMainThread'; | ||
| var prefix = ''; | ||
|
|
||
| if (proxyingMode == 'sync' || proxyingMode == 'async') { | ||
| if (PTHREADS) { | ||
| deps.push('$' + proxyFunc); | ||
| prefix = `if (ENVIRONMENT_IS_PTHREAD) return ${proxyFunc}(${proxiedFunctionTable.length}, 0, ${+(proxyingMode === 'sync')}${args ? ', ' : ''}${args});`; | ||
| } | ||
| const rtnType = sig && sig.length ? sig[0] : null; | ||
| const proxyFunc = | ||
| MEMORY64 && rtnType == 'p' ? 'proxyToMainThreadPtr' : 'proxyToMainThread'; | ||
| deps.push('$' + proxyFunc); | ||
| return ` | ||
| ${async_}function(${args}) { | ||
| if (ENVIRONMENT_IS_PTHREAD) | ||
| return ${proxyFunc}(${proxiedFunctionTable.length}, 0, ${+sync}${args ? ', ' : ''}${args}); | ||
| if (WASM_WORKERS && ASSERTIONS) { | ||
| prefix += `\nassert(!ENVIRONMENT_IS_WASM_WORKER, "Attempted to call proxied function '${mangled}' in a Wasm Worker, but in Wasm Worker enabled builds, proxied function architecture is not available!");`; | ||
sbc100 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } else if (proxyingMode == 'abort' || (proxyingMode == 'abort_debug' && ASSERTIONS)) { | ||
| const insideWorker = ENVIRONMENT_IS_WORKER_THREAD(); | ||
| prefix = `if (${insideWorker}) abort("Attempted to call function '${mangled}' inside a pthread/Wasm Worker, but by using the proxying directive '${proxyingMode}', this function has been declared to only be callable from the main browser thread");`; | ||
| } | ||
|
|
||
| return `${async_}function(${args}) { | ||
| ${prefix} | ||
| ${body} | ||
| }\n`; | ||
| }); | ||
| } else if (WASM_WORKERS && ASSERTIONS) { | ||
| // In ASSERTIONS builds add runtime checks that proxied functions are not attempted to be called in Wasm Workers | ||
| // (since there is no automatic proxying architecture available) | ||
| snippet = modifyJSFunction( | ||
| snippet, | ||
| (args, body) => ` | ||
| function(${args}) { | ||
| assert(!ENVIRONMENT_IS_WASM_WORKER, "Attempted to call proxied function '${mangled}' in a Wasm Worker, but in Wasm Worker enabled builds, proxied function architecture is not available!"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come the fdwrite syscall wasn't hitting this assertion already? |
||
| ${body} | ||
| }\n`, | ||
| ); | ||
| } | ||
| }); | ||
| proxiedFunctionTable.push(mangled); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -262,6 +262,10 @@ var WasiLibrary = { | |
| if (printCharBuffers[1].length) printChar(1, {{{ charCode("\n") }}}); | ||
| if (printCharBuffers[2].length) printChar(2, {{{ charCode("\n") }}}); | ||
| }, | ||
| // When we don't have a filesystem enabled, we do not need to proxy fd_write | ||
| // to the main thread, because all we do is log out() and err() calls. | ||
| // This enables printf() to work from Wasm Workers. | ||
| fd_write__proxy: 'none', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make this a separate PR? It seems like a good idea but unrelated to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put it back because it is needed by the correctness of this PR. Adding the new ASSERTIONS time check that functions are not attempted to be proxied in MINIMAL_RUNTIME mode exposed this function being proxied. It is correct here to declare this as proxy none, and otherwise the test suite won't pass in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see so there is a new assertion at play here, independent of the new I do think it would be nice to split that out so that the adding of the new From a casual reading of the diff its quite hard to see how/where this new assertion was added (it looks like that assertion is pre-existing, but maybe not in all cases?)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The previous code was if (PTHREADS) {
...
} else if (WASM_WORKERS && ASSERTIONS) {
// add check: "assert(!ENVIRONMENT_IS_WASM_WORKER, "Attempted to call proxied function '${mangled}' in a Wasm Worker, but in Wasm Worker enabled builds, proxied function architecture is not available!");"
}The refactored code is if (PTHREADS) {
...
}
if (WASM_WORKERS && ASSERTIONS) {
// add check: "assert(!ENVIRONMENT_IS_WASM_WORKER, "Attempted to call proxied function '${mangled}' in a Wasm Worker, but in Wasm Worker enabled builds, proxied function architecture is not available!");"
}When the original code was written PTHREADS vs WASM_WORKERS were mutually exclusive features - you couldn't build with both of them at the same time, so the two code blocks were effectively equivalent. But then at some point that restriction was lifted that you could build with both of them, so the new code now accounts for that. I agree it is a bit subtle, I am not sure if I did that fix knowingly at the time, but I am kind of wrangling a queue of more than 30 PRs here, so I'd prefer not to rewrite Git history PR commits just for the prose. I think this new check is more accurate to what we'll want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. It still seem odd though since I would have through that most of our tests are for either WASM_WORKERS or PTHREADS... so that assertion for fdwrite should be firing in that case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Figuring out exactly what is going on here seems like a good reason to split out the fdwrite change... but i get that you might be tired on splitting up changes at this point :) Personally I'd love to see the assertions fix + fdwrite fix land first, then the new modes. But I really appreciate all the PRs you've been landing recently and I don't want to discourage your too much so lgtm either way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll defer this feature altogether now for the time being, to avoid any chances. |
||
| fd_write__deps: ['$flush_NO_FILESYSTEM', '$printChar'], | ||
| fd_write__postset: () => addAtExit('flush_NO_FILESYSTEM()'), | ||
| #else | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| #include <emscripten.h> | ||
| #include <pthread.h> | ||
| #include <assert.h> | ||
|
|
||
| // Tests behavior of __proxy: 'abort' and __proxy: 'abort_debug' in pthreads. | ||
|
|
||
| void proxied_js_function(void); | ||
|
|
||
| int might_throw(void(*func)()) { | ||
| int threw = EM_ASM_INT({ | ||
| // Patch over abort() so that we can gracefully finish the test without | ||
| // an unhandled exception hanging the test. | ||
| abort = function(text) { | ||
| throw text; | ||
| }; | ||
|
|
||
| try { | ||
| getWasmTableEntry($0)(); | ||
| } catch(e) { | ||
| console.error('Threw an exception: ' + e); | ||
| return e.toString().includes('this function has been declared to only be callable from the main browser thread'); | ||
| } | ||
| console.error('Function did not throw'); | ||
| return 0; | ||
| }, func); | ||
| return threw; | ||
| } | ||
|
|
||
| void *thread_main(void *arg) { | ||
| REPORT_RESULT(might_throw(proxied_js_function)); | ||
| return 0; | ||
| } | ||
|
|
||
| char stack[1024]; | ||
|
|
||
| int main() { | ||
| proxied_js_function(); // Should be callable from main thread | ||
|
|
||
| pthread_t thread; | ||
| pthread_attr_t attr; | ||
| pthread_attr_init(&attr); | ||
| pthread_create(&thread, &attr, thread_main, 0); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| #include <emscripten.h> | ||
| #include <emscripten/wasm_worker.h> | ||
| #include <assert.h> | ||
|
|
||
| // Tests behavior of __proxy: 'abort' and __proxy: 'abort_debug' in Wasm Workers. | ||
|
|
||
| void proxied_js_function(void); | ||
|
|
||
| int might_throw(void(*func)()) { | ||
| int threw = EM_ASM_INT({ | ||
| // Patch over abort() so that we can gracefully finish the test without | ||
| // an unhandled exception hanging the test. | ||
| abort = function(text) { | ||
| throw text; | ||
| }; | ||
|
|
||
| try { | ||
| getWasmTableEntry($0)(); | ||
| } catch(e) { | ||
| console.error('Threw an exception: ' + e); | ||
| return e.toString().includes('this function has been declared to only be callable from the main browser thread'); | ||
| } | ||
| console.error('Function did not throw'); | ||
| return 0; | ||
| }, func); | ||
| return threw; | ||
| } | ||
|
|
||
| void worker_main() { | ||
| REPORT_RESULT(might_throw(proxied_js_function)); | ||
| } | ||
|
|
||
| char stack[1024]; | ||
|
|
||
| int main() { | ||
| proxied_js_function(); // Should be callable from main thread | ||
| emscripten_wasm_worker_post_function_v(emscripten_malloc_wasm_worker(1024), worker_main); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| addToLibrary({ | ||
| proxied_js_function__proxy: 'abort', | ||
| proxied_js_function: function() { | ||
| console.log('In proxied_js_function'); | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| addToLibrary({ | ||
| proxied_js_function__proxy: 'abort_debug', | ||
| proxied_js_function: function() { | ||
| console.log('In proxied_js_function'); | ||
| } | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.