Set _mi_process_is_initialized before mi_process_setup_auto_thread_done (fixes 439dc27b with MI_TLS_RECURSE_GUARD) - #1399
Open
LongYinan (Brooooooklyn) wants to merge 1 commit into
Conversation
439dc27 moved `mi_process_setup_auto_thread_done` into `mi_process_init_once` so it runs after `mi_thread_init` and can associate the current theap with the thread-done key. It now runs before `_mi_process_is_initialized = true`, and with `MI_TLS_RECURSE_GUARD` (always on for `MI_TLS_MODEL_LOCAL` on macOS) `_mi_theap_default()` still returns `_mi_theap_empty` at that point. The new `mi_assert_internal(mi_theap_is_initialized(theap))` fires in debug builds; release builds skip the association, so the first thread's theap is again never passed to `_mi_thread_done`. Repro (macOS): `cmake -DCMAKE_BUILD_TYPE=Debug -DMI_TLS_MODEL=LOCAL` then `./mimalloc-test-api` aborts at process init on v3.5.2 and passes on v3.5.1. Same with `-DMI_TLS_RECURSE_GUARD=ON` on Linux. Set the flag before `mi_process_setup_auto_thread_done`; the theap is fully initialized by then. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016srh5svTkqfW21zGcU6ht9
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
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.
Follow-up to #1393 / 439dc27. Thanks for taking that fix; it works on the default TLS model (the #1393 repro passes on v3.5.2 on Linux). It regresses with
MI_TLS_RECURSE_GUARDthough, which is always on forMI_TLS_MODEL_LOCALon macOS (prim-tls.h):439dc27 moved
mi_process_setup_auto_thread_done()intomi_process_init_once()so it runs aftermi_thread_init(), but it now runs before_mi_process_is_initialized = true. With the guard,_mi_theap_default()returns_mi_theap_emptyuntil that flag is set:So in debug builds every process aborts at init:
and in release builds the first thread's theap is again never associated with the thread-done key, i.e. the #1393 leak is back for that configuration.
Repro (macOS arm64, v3.5.2):
Same on Linux with
-DMI_TLS_RECURSE_GUARD=ON(the #1393 repro aborts; with this patch it printsOK).Fix: set
_mi_process_is_initialized = truebeforemi_process_setup_auto_thread_done(). The theap is fully initialized by then (mi_thread_init()asserts that), and the pthread key creation inside can allocate through it if it needs to.Verified:
-DMI_TLS_MODEL=LOCAL,mimalloc-test-apimimalloc-test-stress-heapsfails therefcount == 1assert inmi_thread_theaps_donewith-DMI_TLS_MODEL=LOCALon macOS, but it does so on v3.5.1 too, so that is unrelated to this change.