wasm2c runtime: add per-thread initialization/free entrypoint #2332

keithw · 2023-11-19T05:59:02Z

This helps wasm2c-generated modules be usable in a multithreaded context on POSIX, by giving each thread a runtime API entrypoint to initialize and free the altstack, which is used to catch segfaults from stack exhaustion. On Windows, explicit counting of stack depth is used to control stack exhaustion.

Closes #2189. Closes #2046, in the sense that this finishes getting rid of global state in our distributed runtime that doesn't have a thread-safe way of using it.

sbc100 · 2023-11-21T00:22:57Z

Maybe wait for @keithw who might have more feedback..

keithw · 2023-11-22T23:02:31Z

@shravanrn, any concerns before this goes in? I know you were the one initially concerned about thread safety in the runtime via #2046.

The semantics are basically:

wasm_rt_init() initializes the runtime and is basically unchanged.
- By default, it installs the signal handler, allocates an altstack in g_alt_stack, and installs it
- But if WASM_RT_SKIP_SIGNAL_RECOVERY was set, it doesn't do anything.
wasm_rt_init_thread() initializes any subsequent thread.
- By default, it allocates an altstack (not in the global g_alt_stack -- the point is thread-safety), installs it, and returns its location.
- But if WASM_RT_SKIP_SIGNAL_RECOVERY was set, it doesn't do anything.
Same as before, wasm_rt_is_initialized() gets asserted by the generated code in module instantiation (so it's called unless NDEBUG is set)
If signal handling is being used to catch stack exhaustion (i.e. if WASM_RT_USE_STACK_DEPTH_COUNT isn't set), wasm_rt_is_initialized() will now check to make sure that the current thread has been initialized by checking for the existence of an altstack (whether it was the wasm2c runtime that installed it, or the embedder).

So what's new here is that wasm_rt_is_initialized() is going to start enforcing that there's an altstack in place if we're using signals to catch stack exhaustion, and people who run wasm2c-generated code with asserts on will hit that. If the embedder is installing its own signal handler, I'm assuming it's also installing its own altstack on each thread if it wants to catch stack-exhaustion segfaults. Hopefully that's acceptable to Firefox and anybody else using WASM_RT_SKIP_SIGNAL_RECOVERY...?

shravanrn · 2023-11-25T19:14:06Z

Looking today. Sorry for the delay

wasm2c/wasm-rt-impl.c

shravanrn · 2023-11-25T19:48:56Z

wasm2c/wasm-rt-impl.c

+#if WASM_RT_USE_STACK_DEPTH_COUNT || (!WASM_RT_INSTALL_SIGNAL_HANDLER)
+  return NULL;
+#else
+  return os_allocate_and_install_altstack();


A minor future proofing point: could we return a struct with this pointer instead of the pointer directly. This would allow us to easily add fields to thread specific context.

I think we can always do this when we have more fields to add; it's an opaque void* either way.

On further review... I just made wasm_rt_init_thread return void for now. The OS is already keeping track of the altstack.

shravanrn · 2023-11-25T19:52:03Z

wasm2c/wasm-rt-impl.c

+static bool os_has_altstack_installed() {
+  /* check for altstack already in place */
+  stack_t ss;
+  if (sigaltstack(NULL, &ss) != 0) {
+    perror("sigaltstack failed");
+    abort();
+  }
+
+  return !(ss.ss_flags & SS_DISABLE);
+}


I wasn't aware you could do this. Neat! One worry I had was that the man page (which is a bit tricky to follow) says this indicates whether the altstack is "disabled". Unfortunately, I am not clear on whether "disabled" means not installed. I did run a quick test and saw that this worked as expected on my Ubuntu machine. Out of curiosity, did you have a chance to test this on other POSIX flavors and Mac?

Well, we do CI on Mac (with asserts enabled, at least I'm pretty sure)... AFAIK this is sigaltstack's only way of indicating a lack of alternate stack. But that's all the non-Linux testing I've done. I think if there are other POSIXy platforms that care, it would be great to get them in the CI (not necessarily using GitHub Actions to do it).

wasm2c/wasm-rt-impl.c

shravanrn · 2023-11-25T20:00:47Z

wasm2c/wasm-rt-impl.c

-  }
+#if !WASM_RT_USE_STACK_DEPTH_COUNT
+  assert(!g_alt_stack);
+  g_alt_stack = os_allocate_and_install_altstack();


Nit: It may make sense to call wasm_rt_thread_init within here, as logically what we are saying is that the main thread gets has any thread-specific instantiation performed by default. Same with wasm_rt_free

Yeah... I think in a world where wasm_rt_init returns the pointer (and wasm_rt_free takes it), that makes total sense. But if g_alt_stack only exists if certain preprocessor macros are set a certain way, then wasm_rt_init would have to condition the call to wasm_rt_thread_init similarly, and that feels weird -- why aren't we doing a thread init if WASM_RT_USE_STACK_DEPTH_COUNT is set?

So I think as long as g_alt_stack exists, wasm_rt_init has to take responsibility for knowing the details itself; it can't take advantage of the abstraction barrier.

On further review... I got rid of g_alt_stack and just let the OS keep track of it.

On further further view... I think I found a way to do this to satisfy everybody. wasm_rt_init() calls wasm_rt_init_thread() as you propose. g_alt_stack is still there but it's a thread-local mostly just used for safety (it gets cross-checked against the actual altstack).

shravanrn · 2023-11-25T20:05:12Z

Looks reasonable % comments. I am still thinking through the design choice of "main thread is special and the user doesn't have to call wasm_rt_thread_init. I expect that on balance, this makes sense from a backward compatibility standpoint, even though it makes the internal code a bit messier.

keithw · 2023-11-26T01:00:21Z

Looks reasonable % comments. I am still thinking through the design choice of "main thread is special and the user doesn't have to call wasm_rt_thread_init. I expect that on balance, this makes sense from a backward compatibility standpoint, even though it makes the internal code a bit messier.

I was able to clean up the internals a little by letting the OS keep track of the altstack.

sbc100 · 2023-11-27T17:53:41Z

wasm2c/wasm-rt-impl.c

+    return;
+  }
+
+  fprintf(stderr, "Warning: wasm2c altstack was modified unexpectedly\n");


Can you invert the check above and put this error in an early return.

Also, do we want this error string in release builds?

sbc100 · 2023-11-27T17:55:12Z

wasm2c/wasm-rt-impl.c

@@ -42,8 +42,6 @@
 static bool g_signal_handler_installed = false;
 #ifdef _WIN32
 static void* g_sig_handler_handle = 0;
-#else
-static char* g_alt_stack = 0;
 #endif


Maybe too much of a pain but removing g_alt_stack now seems like a separable change, along with SS_DISABLE stuff?

Okay, let me try to split this out into a new PR... (And maybe we should keep track of it in a thread-local just for safety anyway.)

keithw · 2023-11-28T19:26:58Z

This is ready for review again and is a lot smaller now that #2338 has been merged.

sbc100

I love how much smaller this change is now! Thanks for splitting it up.

lgtm from my POV but maybe wait for @shravanrn too

keithw requested review from sbc100 and shravanrn November 19, 2023 05:59

keithw force-pushed the w2c-multithread branch from abaad17 to d2c5c41 Compare November 19, 2023 05:59

sbc100 approved these changes Nov 21, 2023

View reviewed changes

keithw force-pushed the w2c-multithread branch 2 times, most recently from 93d0f69 to f4dabb3 Compare November 22, 2023 22:55

shravanrn requested changes Nov 25, 2023

View reviewed changes

keithw force-pushed the w2c-multithread branch 3 times, most recently from 9cfeb80 to 5c1cbd6 Compare November 26, 2023 04:54

sbc100 reviewed Nov 27, 2023

View reviewed changes

keithw force-pushed the w2c-multithread branch from 5c1cbd6 to fe5780a Compare November 28, 2023 15:58

keithw mentioned this pull request Nov 28, 2023

wasm2c runtime: refactor handling of alternate stack (NFC) #2338

Merged

keithw force-pushed the w2c-multithread branch from fe5780a to a7a02d6 Compare November 28, 2023 16:29

keithw changed the base branch from main to w2c-rt-altstack November 28, 2023 16:29

keithw force-pushed the w2c-rt-altstack branch from ced0cfe to da016f8 Compare November 28, 2023 18:57

Base automatically changed from w2c-rt-altstack to main November 28, 2023 19:13

keithw force-pushed the w2c-multithread branch from a7a02d6 to fe77127 Compare November 28, 2023 19:23

sbc100 approved these changes Nov 28, 2023

View reviewed changes

keithw mentioned this pull request Nov 30, 2023

Fix wasm-rt-impl defines nesting #2339

Closed

shravanrn approved these changes Dec 2, 2023

View reviewed changes

keithw force-pushed the w2c-multithread branch from fe77127 to 60c2c7e Compare December 3, 2023 05:03

w2c runtime: add per-thread init/free API

2009930

keithw force-pushed the w2c-multithread branch from 65eb03a to 321122d Compare December 3, 2023 07:08

keithw force-pushed the w2c-multithread branch from 321122d to 2009930 Compare December 4, 2023 02:13

keithw enabled auto-merge (squash) December 4, 2023 02:14

keithw merged commit 0362847 into main Dec 4, 2023
15 checks passed

keithw deleted the w2c-multithread branch December 4, 2023 02:23

keithw mentioned this pull request Dec 14, 2023

wasm2c: Cleanup/separate code for heap and stack signal handlers #2354

Merged

jonathanmarvens mentioned this pull request Jan 25, 2024

wasm2c: Are module instances truly thread-safe? #2372

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasm2c runtime: add per-thread initialization/free entrypoint #2332

wasm2c runtime: add per-thread initialization/free entrypoint #2332

keithw commented Nov 19, 2023 •

edited

Loading

sbc100 commented Nov 21, 2023

keithw commented Nov 22, 2023 •

edited

Loading

shravanrn commented Nov 25, 2023

shravanrn Nov 25, 2023

keithw Nov 26, 2023

keithw Nov 26, 2023

shravanrn Nov 25, 2023

keithw Nov 26, 2023

shravanrn Nov 25, 2023

keithw Nov 26, 2023

keithw Nov 26, 2023

keithw Nov 28, 2023

shravanrn commented Nov 25, 2023

keithw commented Nov 26, 2023 •

edited

Loading

sbc100 Nov 27, 2023

keithw Nov 28, 2023

sbc100 Nov 27, 2023

keithw Nov 28, 2023

keithw Nov 28, 2023

keithw commented Nov 28, 2023

sbc100 left a comment

wasm2c runtime: add per-thread initialization/free entrypoint #2332

wasm2c runtime: add per-thread initialization/free entrypoint #2332

Conversation

keithw commented Nov 19, 2023 • edited Loading

sbc100 commented Nov 21, 2023

keithw commented Nov 22, 2023 • edited Loading

shravanrn commented Nov 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shravanrn commented Nov 25, 2023

keithw commented Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithw commented Nov 28, 2023

sbc100 left a comment

Choose a reason for hiding this comment

keithw commented Nov 19, 2023 •

edited

Loading

keithw commented Nov 22, 2023 •

edited

Loading

keithw commented Nov 26, 2023 •

edited

Loading