Block in dlopen until all threads have loaded the module #18376

sbc100 · 2022-12-14T23:35:19Z

system/lib/libc/dynlink.c

dschuff · 2022-12-15T01:08:49Z

I wonder if there is a way to test the case where multiple dlopens come from different threads before other threads are synced up. That could also happen coming from the same thread, when using the async dlopen.

sbc100 · 2022-12-15T01:11:03Z

I wonder if there is a way to test the case where multiple dlopens come from different threads before other threads are synced up. That could also happen coming from the same thread, when using the async dlopen.

Good idea. it might be hard to setup such a test but certainly worth having. Would be good to prove that such cases don't deadlock.

kripken

At a high level, why do we need different syncing code for the main thread vs others? (A dlopen can appear on any thread, so I'd think the situation is symmetric?)

src/library_pthread.js

system/lib/libc/dynlink.c

system/lib/pthread/emscripten_futex_wait.c

src/library_pthread.js

system/lib/libc/dynlink.c

src/library_pthread.js

system/lib/libc/dynlink.c

system/lib/pthread/library_pthread.c

system/lib/libc/dynlink.c

src/library_pthread.js

Prior to this change we were tracking the sequence of `dlopen` events and replaying them on each thread during `dlsync`. Now we also track `dlsym` events. A new data structure is used here to abstract over these two events (`dlevent`). During dlsync we then reply both the `dlopen` and the `dlsym` events. For `dlsym` events we serialized them as "dso+index", i.e. which dso did the symbol come from and what is the index of the symbol within that dso's symbol table. This is important for #18376.

…18590) Prior to this change we were tracking the sequence of `dlopen` events and replaying them on each thread during `dlsync`. Now we also track `dlsym` events. A new data structure is used here to abstract over these two events (`dlevent`). During dlsync we then reply both the `dlopen` and the `dlsym` events. For `dlsym` events we serialized them as "dso+index", i.e. which dso did the symbol come from and what is the index of the symbol within that dso's symbol table. This is important for #18376.

sbc100 · 2023-01-28T05:39:09Z

I wonder if there is a way to test the case where multiple dlopens come from different threads before other threads are synced up. That could also happen coming from the same thread, when using the async dlopen.

Since we have single write lock that is held my dlopen, we don't allow any other thread to begin a dlopen of dlsym, whicle there is one outstange. Even async dlopen will still block until is has the write lock. We could try to relax that restriction once we take a another look at the async dlopen interface.

system/lib/pthread/emscripten_futex_wait.c

system/lib/libc/dynlink.c

system/lib/pthread/emscripten_futex_wait.c

sbc100 · 2023-02-03T23:12:33Z

I've updated this PR with some extra documentation in Dynamic-Linking.rst.

There are two specific issues that came up when discussed possible deadlock risks.

Possible deadlock due to threads calling atomics.wait directly rather than using our futex wrapper. For this case I've simply updated the docs to describe this caveat, and how to deal with it. We can look into some kind of auto-detection or re-writing solution as a followup and before we move out of the experimental.
Possible deadlock due to malloc taking a inner lock which might block and trigger a dlsync, which itself might trigger a TLS allocation and recurse into malloc. I've convinced myself that, at least to dlmalloc and emmalloc this won't be an issue. The deadlock that @tlively found was due to the call to malloc that removed in dylink: Remove dynamic linking startup code and allocation #18643. The only remaining allocation that can happen during dlsync is the TLS allocation here:

emscripten/system/lib/pthread/emscripten_tls_init.c

Line 71 in 5a2a360

tls_block = emscripten_builtin_memalign(__builtin_wasm_tls_align(), tls_size);

. Note that this uses emscripten_builtin_malloc which bypasses things like asan and lsan. The deadlock that thomas found was becuase lsan was taking a lock internally. The non-lsan version of malloc that we have contains only a single outer lock. This means that once that outer malloc lock is taken we don't do any blocking operations until malloc is done. We could looking into trying to detect these possible deadlocks, but I think we can leave that to as followup, especially since this deadlock issue is not introduced by this PR, since Thomas noticed it already in the tree.

tlively

proofreading docs

tlively · 2023-02-03T23:24:48Z