Avoid proxying print handlers back to the main thread

[kleisauke]

This partially reverts commit 97b352e.

Resolves: #19683.

[juj]

Fantastic, thanks. I have been wondering about how to address the use case that motivated the original PR #17925 in the first place.

Reverting that PR will fix the immediate sequential consistency problem, although will there be any problems with existing users..(?)

Could they migrate code that was injecting Module.print at runtime, to use a sequentially consistent safe way to override Module.print "statically", i.e. using a pre-run file or some other codegenned file, so that the assignment to Module.print will also be present in the code that is loaded into each pthread at startup?

Or if the code pattern for injecting a Module.print() function at startup from the main thread to apply to all Workers still seems worthwhile (even with the performance caveat it would bring), then it would seem that we'd have to figure out a way to proxy logging prints via the WebAssembly proxying queue over to the main thread.

Would we need some kind of way (assert/warning) to help users discover that dynamically assigning Module.print() from the main thread will only apply to main thread, and not to pthreads? Or would that be something to expect to be implicitly given.

[sbc100]

For any kind of complex program that uses a real FS we already proxy stdout and stderr anyway so I think continuing to proxy these calls seems good/fine, but we should switch to using the normal in-memory proxying mechanism.

The main problem with that is that its not supported with MINIMAL_RUNTIME, but maybe these print and printErr overides are already not supported here?

[sbc100]

I don't think its reasonable to land this change as is since it breaking anyone doing print or printErr overrides with pthreads (which includes anyone using the default shell.html file).

[kleisauke]

It seems that marking the emscripten_out* and emscripten_err* functions for sync proxying is a possible alternative (as noted in #17688 (comment)).

Hopefully that will resolve the test flakiness, I've removed the flaky decorators to see if it helps.

[kleisauke]

... ah, looking at https://circleci.com/gh/emscripten-core/emscripten/1017413, I guess that should only be done for #if PTHREADS.

Uncaught RuntimeError: Aborted(Assertion failed: Attempted to call proxied function '_emscripten_out' in a Wasm Worker, but in Wasm Worker enabled builds, proxied function architecture is not available!)

[sbc100]

This works for native code calls to emscripten_err but the err and out functions (which are not currently library functions) have many calls sites and those should really be the ones that get proxied.

I'm looking into moving err and out to library code, but its a little complex since they used very early on.

[kleisauke]

Dropped this commit. It also failed for test_pthread_cancel and test_pthread_busy_wait_atexit, which depend on emscripten_out() not being proxied back to the main thread.

emscripten/test/pthread/test_pthread_busy_wait_atexit.cpp

Lines 15 to 17 in a4d0dbc

	// Avoid using printf here since stdio is proxied back to the
	// main thread which is busy looping
	emscripten_out("in thread");

[kleisauke]

Looking at #17688 (comment), I realized why I designed other.test_pthread_print_override_modularize to use emscripten_out() instead of printf(). With the legacy filesystem, the issue didn't appear when using printf(), because printf() is proxied through fd_write():

emscripten/src/lib/libwasi.js

Line 270 in 873dc2e

fd_write: (fd, iov, iovcnt, pnum) => {

emscripten/src/lib/libwasi.js

Line 614 in 873dc2e

wrapSyscallFunction(x, WasiLibrary, true);

emscripten/src/lib/libcore.js

Lines 2672 to 2685 in 873dc2e

	#if PTHREADS
	// Most syscalls need to happen on the main JS thread (e.g. because the
	// filesystem is in JS and on that thread). Proxy synchronously to there.
	// There are some exceptions, syscalls that we know are ok to just run in
	// any thread; those are marked as not being proxied with
	// __proxy: false
	// A syscall without a return value could perhaps be proxied asynchronously
	// instead of synchronously, and marked with
	// __proxy: 'async'
	// (but essentially all syscalls do have return values).
	if (library[x + '__proxy'] === undefined) {
	library[x + '__proxy'] = 'sync';
	}
	#endif

But WasmFS uses emscripten_out() (and emscripten_err()) for its printf() writing.

emscripten/system/lib/wasmfs/special_files.cpp

Line 123 in 873dc2e

return writeToJS(buf, len, &emscripten_out, writeBuffer);

emscripten/system/lib/wasmfs/special_files.cpp

Line 137 in 873dc2e

return writeToJS(buf, len, &emscripten_err, writeBuffer);

which bypasses the proxying system entirely. Commit 96da2f2 revises the test to make its purpose clearer.

[kleisauke]

Marking this as a draft since I'm not sure of the best way to proceed.

[sbc100]

My proposal is as follows:

1. Move out and err to library functions (this is a fairly involved change on its own).
2. Mark out and err as proxy: sync (except on MINIMAL_RUNTIME) and remove the postMessage for these events.