Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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)

3.1.67 - 09/17/24
-----------------
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 this function has been declared to only be callable from the main browser thread");`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about "but this function can only be called from the main thread"?

Using the term main browser thread I a little misleading because emscripten application can be run on workers where there is no main browser thread involved. We sometimes call this "main application thread", but I think just "main thread" here is clear enough.

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.

The proxying modes __proxy: 'sync' and __proxy: 'async' definitely mean to run the JS code in the main browser thread, and not in the thread that executes 'main'.

I.e. in PROXY_TO_PTHREAD builds, __proxy: 'sync' and __proxy: 'async' run the proxied JS code in the main browser thread, which is not the same thread as the main application thread (the main runtime thread)

The earlier PROXY_TO_WORKER build mode (which I presume is what you mean by "can be run on workers where there is no main browser thread involved") predates pthreads/SharedArrayBuffer, and does not interact with SAB, or the proxying architecture. So these proxying modes are not relevant there when pthreads are not used.

So I think saying "main browser thread" here is the correct thing to do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about "but this function can only be called from the main thread"?

Changed the message into form

"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"

I would like to keep the mention about the proxy declaration that prevents the call. Otherwise a developer might be surprised to mistakenly ponder "how does it know this function is not callable from a worker?"

Copy link
Collaborator

Choose a reason for hiding this comment

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

The proxying modes __proxy: 'sync' and __proxy: 'async' definitely mean to run the JS code in the main browser thread, and not in the thread that executes 'main'.

The thread on which the emscripten-generated code is first run I call the main application thread, and it might be running in worker, in which case there is no main browser thread at all in the picture. When you write MAIN_THREAD_EM_ASM or __proxy: 'sync' its is the main application thread that runs you code. It just happens that this is also normally the main browser thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, has there been some kind of change in configuration that I am not aware of?

My expectation:

Building with -sPROXY_TO_WORKER: main() is run in a Worker thread. __proxy architecture is not available there.
Building with -pthread: main() is run in main browser thread. main browser thread == main application thread, __proxy targets main browser thread.
Building with -pthread -sPROXY_TO_PTHREAD: main() is run in a separate Worker thread, i.e. main application thread is not main browser thread. __proxy: 'sync' and MAIN_THREAD_EM_ASM() proxy to the main browser thread, and not to the main application thread.

In each case, proxying does target the main browser thread. Has this changed somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have several users that do this.

Is this case a scenario where the user manually spawns a new Worker(), and inside that Worker, they load up the Emscripten-compiled pthread-enabled program?

Such use case was never supported originally, because of the problem that in such a manual "I loaded the app into a Worker" mode, Emscripten would still need to be able to load some stuff (a lot of stuff) into the JS context of the main application thread, in order for any kind of general proxying to work. And Emscripten runtime JS libraries were not developed with the impression that the target thread that proxying occurs to, would not be the main browser thread.

For example, if I start searching for __proxy: 'sync' in /src/, there are functions that assume that they are always the main browser thread:

emscripten_set_window_title(): document.title only exists in main browser thread
emscripten_get_screen_size(): screen.width / screen.height exist only in main browser thread
emscripten_hide_mouse(): document.styleSheets only in main browser thread
emscripten_set_canvas_size: Browser.setCanvasSize() works only in main browser thread
emscripten_get_canvas_size: Module['canvas'] only works in main browser thread
emscripten_create_worker: requires Subworkers support in order to work (https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers#spawning_subworkers)

library_html5.js: everything here that has __proxy: 'sync' expects that proxy target lands to the main browser thread
library_egl.js, library_glut.js: everything with __proxy: 'sync' expects that proxy target is the main browser thread
library_openal.js, library_html5_webgl.js, library_sdl.js: likewise

that is why I added the whole PROXY_TO_PTHREAD mode so that users would not need to figure out how to manually launch Emscripten content main() in a Worker - they could reuse the system-provided mechanism for that.

Although now that I think about it, iirc subworkers are supported widely, so that is no longer a problem (in 2015 Chrome did not support subworkers)

So maybe those users that do go and launch their pthread builds in a Worker are careful to not use any of the above APIs, and just instead do generic computation there? I.e. those people are not using Canvases or library_browser.js or library_html5.js or any other DOM accessing things that Emscripten provides?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I believe it was actually you who added those two functions back in 174a082)

Yeah, I added those, but that is not for supporting the "user manually launches a pthread-enabled build inside their manually created new Worker()".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this case a scenario where the user manually spawns a new Worker(), and inside that Worker, they load up the Emscripten-compiled pthread-enabled program?

Such use case was never supported originally, because of the problem that in such a manual "I loaded the app into a Worker" mode, Emscripten would still need to be able to load some stuff (a lot of stuff) into the JS context of the main application thread, in order for any kind of general proxying to work. And Emscripten runtime JS libraries were not developed with the impression that the target thread that proxying occurs to, would not be the main browser thread.

Yes exactly. It turns out that this configuration is supported, at least for some programs. Obviously such programs cannot use DOM or anything like that, but I believe we do at least some testing of this configuration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I added those, but that is not for supporting the "user manually launches a pthread-enabled build inside their manually created new Worker()".

Can you explain what you intended the difference between those two functions to be then? I seems that unless we support that configuration then then the main runtime thread and the main browser thread would always be the same? Unless I'm missing something?

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.

Very originally those functions intended to be a generic set to enable common handling of each of single-threaded, -pthread, and PROXY_TO_PTHREAD and PROXY_TO_WORKER build modes, so that users would be able to do multiple variants of builds from the same codebase and we'd be able to write system library code that would be aware of each of those modes.

So, for example, in a PROXY_TO_PTHREAD build, it would be expected to happen that emscripten_is_main_runtime_thread() == emscripten_is_main_browser_thread(). But then if one would make a PROXY_TO_WORKER build of the same, then those functions would change to return the different respective things. So that way one could have one codebase that was ready to mutate to any of those build modes.

But I did not originally intend for pthreads enabled builds to ever be loaded while inside a Worker(), since the __proxy: 'sync' semantics would proxy to the hosting Worker instead of going to the main browser thread, and we have never had any facilities to do proxying to the main browser thread in such a mode.

Although now that I think about it, yeah, it is probably the case that if user avoids all those library_x.js files that I listed above (that suffer from this problem), things will probably be pretty safe for them, even if they manually load pthread-enabled builds in their own Workers. And one can build "reasonable" use cases in the absence of relying on those library_x.js files.

}

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
53 changes: 53 additions & 0 deletions test/pthread/proxy_abort.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#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