Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ See docs/process.md for more on how version tagging works.
- The number of arguments passed to Embind function calls is now only verified
with ASSERTIONS enabled. (#22591)
- Optional arguments can now be omitted from Embind function calls. (#22591)
- Added two new proxying directives. foo__proxy: 'abort' will abort program
execution if JS function foo is called from a pthread or a Wasm Worker.
foo__proxy: 'abort_debug' will do the same, but only in ASSERTIONS builds, and
in non-ASSERTIONS builds will be no-op.
Use these proxying directives to annotate JS functions that should not be
getting called from Workers. (#22648)
- Recent changes to Binaryen included in this version significantly improve
the speed at which the post-link optimizations run for some large programs.

Expand Down
57 changes: 29 additions & 28 deletions src/jsifier.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -319,39 +319,40 @@ 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) {
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 `
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!");`;
}
} else if (proxyingMode == 'abort' || (proxyingMode == 'abort_debug' && ASSERTIONS)) {
const insideWorker = (PTHREADS && WASM_WORKERS)
? '(ENVIRONMENT_IS_PTHREAD || ENVIRONMENT_IS_WASM_WORKER)'
: (PTHREADS ? 'ENVIRONMENT_IS_PTHREAD' : 'ENVIRONMENT_IS_WASM_WORKER');
prefix = `assert(!${insideWorker}, "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 `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!");
Copy link
Collaborator

Choose a reason for hiding this comment

The 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);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/library_wasi.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ var WasiLibrary = {
#else
fd_write__deps: ['$printChar'],
#endif
fd_write__proxy: 'none', // Each Worker can do stdout/stderr printing on its own, without needing to proxy to the main thread.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure doWritev won't work if you call it on a thread. And I think its not true for printChar either since you would end up with a different printCharBuffers on each thread.. which is not correct.

Copy link
Collaborator Author

@juj juj Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you would end up with a different printCharBuffers on each thread.. which is not correct.

I looked into this some decade ago, and from back then, I could not find defined semantics on how multiple threads writing on stdout/stderr should synchronize, when fflush is not called by the user. In most implementations, like ours, printing \n is an implicit fflush.

If multiple threads race to print to stdout/stderr, then it does seem to me it would be better semantics for each thread to construct full \n buffered lines that they would then race to print, rather than each thread mixing their individual character writes that could result in the data from multiple threads inter-mixing with each other? I could not find info to say such behavior would not be compliant.

Each Worker has a copy of MEMFS/library_tty.js in them, so they would each end up calling to default_tty_ops.put_char() and pushing out console.log() calls independently?

Having each thread synchronously block to perform stdout prints would seem like a rough performance overhead to have?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the function is not only used for stdout/stderr, its used for all file access.

If we do make this change it would be very large one (basically making the filesystem multi-threaded) and I would at least expect a separate RP for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, you're right of course. I somehow was interpreting this from the perspective that this function was already just for the stdout/stderr writing case. That would not work. I'll think of something here.

fd_write: (fd, iov, iovcnt, pnum) => {
#if SYSCALLS_REQUIRE_FILESYSTEM
var stream = SYSCALLS.getStreamFromFD(fd);
Expand All @@ -295,6 +296,7 @@ var WasiLibrary = {
fd_pwrite__deps: ['$doWritev'],
#endif
fd_pwrite__i53abi: true,
fd_pwrite__proxy: 'none', // Each Worker can do stdout/stderr printing on its own, without needing to proxy to the main thread.
fd_pwrite: (fd, iov, iovcnt, offset, pnum) => {
#if SYSCALLS_REQUIRE_FILESYSTEM
if (isNaN(offset)) return {{{ cDefs.EOVERFLOW }}};
Expand Down
49 changes: 49 additions & 0 deletions test/pthread/proxy_abort.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#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 assert() so that it does not abort execution on assert failure, but instead
// throws a catchable exception.
assert = function(condition, text) {
if (!condition) {
throw 'Assertion failed' + (text ? ": " + 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 test() {
proxied_js_function();
}

void *thread_main(void *arg) {
REPORT_RESULT(might_throw(test));
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);
}
12 changes: 12 additions & 0 deletions test/test_browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -5107,6 +5107,18 @@ def test_wasm_worker_no_proxied_js_functions(self):
self.btest('wasm_worker/no_proxied_js_functions.c', expected='0',
args=['--js-library', test_file('wasm_worker/no_proxied_js_functions.js')])

# Tests that the proxying directives foo__proxy: 'abort' and foo__proxy: 'abort_debug' work.
@parameterized({
'': ('1', ['--js-library', test_file('wasm_worker/proxy_abort.js')],),
'debug': ('1', ['--js-library', test_file('wasm_worker/proxy_abort_debug.js'), '-sASSERTIONS'],),
'debug_no_assertions': ('0', ['--js-library', test_file('wasm_worker/proxy_abort_debug.js'), '-sASSERTIONS=0'],),
})
def test_proxy_abort(self, expect_result, args):
self.btest('pthread/proxy_abort.c', expected=expect_result, args=args + ['-pthread'])

self.set_setting('WASM_WORKERS')
self.btest('wasm_worker/proxy_abort.c', expected=expect_result, args=args)

# Tests emscripten_semaphore_init(), emscripten_semaphore_waitinf_acquire() and emscripten_semaphore_release()
@also_with_minimal_runtime
def test_wasm_worker_semaphore_waitinf_acquire(self):
Expand Down
48 changes: 48 additions & 0 deletions test/wasm_worker/proxy_abort.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#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 assert() so that it does not abort execution on assert failure, but instead
// throws a catchable exception.
assert = function(condition, text) {
if (!condition) {
throw 'Assertion failed' + (text ? ": " + 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 test()
{
proxied_js_function();
}

void worker_main()
{
REPORT_RESULT(might_throw(test));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not just do might_throw(proxied_js_function) here? (i.e. do we need the test function at all?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since that will throw an unhandled exception, and the test run will then hang without calling out to REPORT_RESULT() at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean the test wrapper function:

void test() {
  proxied_js_function();
}

It seems like a pointless wrapper around proxied_js_function? Is it needed for some reason? Why can't the address of proxied_js_function be used in place of the address of test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that can be removed.

}

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);
}
6 changes: 6 additions & 0 deletions test/wasm_worker/proxy_abort.js
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');
}
});
6 changes: 6 additions & 0 deletions test/wasm_worker/proxy_abort_debug.js
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');
}
});
Loading