Ensure static TLS region is free'd last - #15086
Merged
Merged
Conversation
sbc100
force-pushed
the
tls_free_last
branch
from
September 21, 2021 21:55
1485cf9 to
95b0a77
Compare
sbc100
force-pushed
the
tls_free_last
branch
from
September 22, 2021 00:15
95b0a77 to
3fabe01
Compare
sbc100
added a commit
that referenced
this pull request
Sep 22, 2021
This is needed by a followup PR (#15086) which improves our LSan support. LSan wants to be able to call pthread_key_create during its own startup and doesn't expect allocation to occur during this time: https://github.com/emscripten-core/emscripten/blob/6b53dedf3d2ace15bfd780e44f4a8ba34407fc27/system/lib/compiler-rt/lib/lsan/lsan.cpp#L113 This method of using a static array of keys is also the one used by musl so this seems like a reasonable implementation: https://github.com/emscripten-core/emscripten/blob/6b53dedf3d2ace15bfd780e44f4a8ba34407fc27/system/lib/libc/musl/src/thread/pthread_key_create.c#L8
sbc100
force-pushed
the
tls_free_last
branch
from
September 22, 2021 00:29
3fabe01 to
e56bba9
Compare
sbc100
force-pushed
the
tls_free_last
branch
from
September 22, 2021 00:30
e56bba9 to
5abe62e
Compare
Collaborator
Author
|
This change depends on #15087.. any chance you could take a look at that too? |
sbc100
added a commit
that referenced
this pull request
Sep 22, 2021
This is needed by a followup PR (#15086) which improves our LSan support. LSan wants to be able to call pthread_key_create during its own startup and doesn't expect allocation to occur during this time: https://github.com/emscripten-core/emscripten/blob/6b53dedf3d2ace15bfd780e44f4a8ba34407fc27/system/lib/compiler-rt/lib/lsan/lsan.cpp#L113 This method of using a static array of keys is also the one used by musl so this seems like a reasonable implementation: https://github.com/emscripten-core/emscripten/blob/6b53dedf3d2ace15bfd780e44f4a8ba34407fc27/system/lib/libc/musl/src/thread/pthread_key_create.c#L8
sbc100
force-pushed
the
stub_tls_refactor
branch
from
September 22, 2021 00:53
0f8f650 to
939427a
Compare
sbc100
force-pushed
the
tls_free_last
branch
from
September 22, 2021 00:54
5abe62e to
3498e6e
Compare
sbc100
added a commit
that referenced
this pull request
Sep 22, 2021
…15087) This is needed by a followup PR (#15086) which improves our LSan support. LSan wants to be able to call pthread_key_create during its own startup and doesn't expect allocation to occur during this time: https://github.com/emscripten-core/emscripten/blob/6b53dedf3d2ace15bfd780e44f4a8ba34407fc27/system/lib/compiler-rt/lib/lsan/lsan.cpp#L113 This method of using a static array of keys is also the one used by musl so this seems like a reasonable implementation: https://github.com/emscripten-core/emscripten/blob/6b53dedf3d2ace15bfd780e44f4a8ba34407fc27/system/lib/libc/musl/src/thread/pthread_key_create.c#L8
We were using `__cxa_thread_atexit` to register the funtion for free'ing TLS data. And because were were running the exit handlers `__pthread_exit_run_handlers` before `__pthread_tsd_run_dtors` it meant that any pthread_key descructor functions would not be able to access TLS variables. Instead we really need to delay free'ing of TLS data until all exit handlers and pthread_key descructors have been run. This was causing failure of `lsan.test_stdio_locking` due to the fact that lsan uses TLS during pthread key destruction. Specifically `thread_finalize` (a pthread_key desctructor function) calls `ThreadFinish` which in turn calls `GetCurrentThread` which uses a thread local variable to store the current thread ID. This change should allow #14489 to be dramatically simplified because it will allow the use of TLS variables to implement the atexit queue.
sbc100
force-pushed
the
tls_free_last
branch
from
September 22, 2021 04:02
3498e6e to
dc448d4
Compare
sbc100
enabled auto-merge (squash)
September 22, 2021 04:02
kripken
reviewed
Sep 22, 2021
| LSAN_MAYBE_INTERCEPT_PTHREAD_ATFORK; | ||
|
|
||
| LSAN_MAYBE_INTERCEPT_STRERROR; | ||
| #endif // !SANITIZER_FUCHSIA && !SANITIZER_EMSCRIPTEN |
Member
There was a problem hiding this comment.
is the movement of the fuscia ifdef here a divergence from upstream?
Collaborator
Author
There was a problem hiding this comment.
Oh yes, I hadn't realized I had effect fuchsia here. Will follow up.
sbc100
added a commit
that referenced
this pull request
Sep 22, 2021
…dling As well use being less code to maintain and more shared code this also enables thread exit handling which means ASan now tracks thread exit where it didn't before. With the old/existing code noone was ever calling the ASan thread destructor (passed as arg0 to AsanTSDInit). I imagine this approach might not have been possible in the past since there were some issues with using pthread_keys with destructors (e.g. #15086). Testing by running entire `asan` test suite.
sbc100
added a commit
that referenced
this pull request
Sep 22, 2021
In #15086 the intent was to enable `g_thread_finalize_key` under emscripten but I unwittingly modified the ifdef region used by fuchsia is well.
sbc100
added a commit
that referenced
this pull request
Sep 22, 2021
In #15086 the intent was to enable `g_thread_finalize_key` under emscripten but I unwittingly modified the ifdef region used by fuchsia is well.
sbc100
added a commit
that referenced
this pull request
Sep 22, 2021
…dling (#15095) As well use being less code to maintain and more shared code this also enables thread exit handling which means ASan now tracks thread exit where it didn't before. With the old/existing code noone was ever calling the ASan thread destructor (passed as arg0 to AsanTSDInit). I imagine this approach might not have been possible in the past since there were some issues with using pthread_keys with destructors (e.g. #15086). Testing by running entire `asan` test suite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We were using
__cxa_thread_atexitto register the funtion for free'ingTLS data. And because were were running the exit handlers
__pthread_exit_run_handlersbefore__pthread_tsd_run_dtorsit meantthat any pthread_key descructor functions would not be able to access
TLS variables.
Instead we really need to delay free'ing of TLS data until all exit
handlers and pthread_key descructors have been run.
This was causing failure of
lsan.test_stdio_lockingdue to the factthat lsan uses TLS during pthread key destruction. Specifically
thread_finalize(a pthread_key desctructor function) callsThreadFinishwhich in turn callsGetCurrentThreadwhich uses athread local variable to store the current thread ID.
This change should allow #14489 to be dramatically simplified because
it will allow the use of TLS variables to implement the atexit queue.