Skip to content

Warn before busy waiting on futex #15957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mmarczell-graphisoft wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Warn before busy waiting on futex #15957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d1884f
Warn before busy waiting on futex
mmarczell-graphisoft Jan 11, 2022
4a35256
Fix metadce tests?
mmarczell-graphisoft Jan 11, 2022
05932f5
Merge branch 'main' into futex-warn
mmarczell-graphisoft Jan 13, 2022
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
1 change: 1 addition & 0 deletions system/lib/pthread/emscripten_futex_wait.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ int emscripten_futex_wait(volatile void *addr, uint32_t val, double max_wait_ms)
// __builtin_wasm_memory_atomic_wait32 so we call out the JS function that
// will busy wait.
if (!_emscripten_thread_supports_atomics_wait()) {
emscripten_check_blocking_allowed();
ret = _emscripten_futex_wait_non_blocking(addr, val, max_wait_ms);

@kripken kripken Jan 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm confused. This line does a non-blocking wait, so why check for blocking before it?

@sbc100 sbc100 Jan 12, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The job of emscripten_check_blocking_allowed is to report the warning about busy waiting on main (e.g. Blocking on the main thread is very dangerous..).

However, this is normally called before emscripten_futex_wait is called. For example in pthread_cond_timedwait.c and pthread_join.c. I guess there is no harm in also calling it from here.. but generally I think it should have already been called before we get here.

@mmarczell-graphisoft mmarczell-graphisoft Jan 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sbc100 Here is the stack trace that displays the warning with this change, but hasn't done it before.
I'm not familiar enough with the architecture to know where the better place for this call would be...?
image

emscripten_conditional_set_current_thread_status(EM_THREAD_STATUS_WAITFUTEX, EM_THREAD_STATUS_RUNNING);
return ret;
Expand Down
2 changes: 1 addition & 1 deletion tests/other/metadce/minimal_main_Oz_USE_PTHREADS_PROXY_TO_PTHREAD.exports
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ G
H
I
J
p
K
q
r
s
Expand Down
1 change: 1 addition & 0 deletions tests/other/metadce/minimal_main_Oz_USE_PTHREADS_PROXY_TO_PTHREAD.imports
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ a.l
a.m
a.n
a.o
a.p
1 change: 1 addition & 0 deletions tests/other/metadce/minimal_main_Oz_USE_PTHREADS_PROXY_TO_PTHREAD.sent
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ l
m
n
o
p