Add pthread mutex deadlock detection for debug builds #24607
Conversation
sbc100
force-pushed
the
deadlock_detection
branch
from
a57b588 to
0e10a5e
Compare
sbc100
force-pushed
the
deadlock_detection
branch
from
0e10a5e to
efb88c5
Compare
| if ((m->_m_type&15) == PTHREAD_MUTEX_NORMAL | ||
| && !a_cas(&m->_m_lock, 0, EBUSY)) | ||
| return 0; | ||
| #endif |
There was a problem hiding this comment.
Should upstream do the same for all platforms? I'm just trying to understand if this is a general improvement or if it works around a specific problem in wasm.
There was a problem hiding this comment.
I think its a general improvement for debug builds yes. I guess most libc's don't ship debug builds like we do (especially automatically selecting debug libs based on command line flags, which emcc does is quite unusual).
There was a problem hiding this comment.
Upstream musl doesn't really have any/many assertions AFAICT, so its not something they tend to consider I suppose?
There was a problem hiding this comment.
Indeed, it looks like upstream musl only has assertion in 2 very specific places:
src/regex/regcomp.c
src/malloc/mallocng/..
No other assertions in whole of libc.
This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565
sbc100
force-pushed
the
deadlock_detection
branch
from
efb88c5 to
a8d3d3f
Compare
| if ((m->_m_type&15) == PTHREAD_MUTEX_NORMAL | ||
| && !a_cas(&m->_m_lock, 0, EBUSY)) | ||
| return 0; | ||
| #endif |
| @@ -79,6 +86,9 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec | |||
| int own = r & 0x3fffffff; | |||
| if (!own && (!r || (type&4))) | |||
| continue; | |||
| #if defined(__EMSCRIPTEN__) && !defined(NDEBUG) | |||
| assert(own != __pthread_self()->tid && "pthread mutex deadlock detected"); | |||
| #endif | |||
| if ((type&3) == PTHREAD_MUTEX_ERRORCHECK | |||
There was a problem hiding this comment.
Can we perhaps enable this PTHREAD_MUTEX_ERRORCHECK globally somehow, rather than add these three lines? (It wouldn't assert though, just return EDEADLK)
| _emscripten_init_main_thread_js(&__main_pthread); | ||
|
|
||
| __main_pthread.stack = (void*)emscripten_stack_get_base(); | ||
| __main_pthread.stack_size = emscripten_stack_get_base() - emscripten_stack_get_end(); |
There was a problem hiding this comment.
Shouldn't the stack setup be as early as possible? Why move it down?
This would really have helped in at least detecting the deadlock that was found in #24570 / #24565