[native] Replace std::mutex with pthread_mutex_t in the CoreCLR host - #12541
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces the CoreCLR host’s dependence on libc++ by replacing std::mutex/std::lock_guard usage with a minimal pthread-backed Mutex/MutexGuard wrapper in the shared native runtime-base headers.
Changes:
- Add
MutexandMutexGuardwrappers aroundpthread_mutex_tinruntime-base/mutex.hh. - Update CoreCLR host code paths (assembly store, FastDev assemblies, DSO loader, startup-aware lock) to use the new wrapper instead of
<mutex>. - Update shared timing code to use the new wrapper (affecting other runtime lanes via common sources).
Show a summary per file
| File | Description |
|---|---|
| src/native/common/include/runtime-base/timing.hh | Switch timing sequence locking to Mutex/MutexGuard. |
| src/native/common/include/runtime-base/mutex.hh | Introduce pthread-backed Mutex and MutexGuard. |
| src/native/clr/include/runtime-base/startup-aware-lock.hh | Replace std::mutex reference with Mutex. |
| src/native/clr/include/runtime-base/monodroid-dl.hh | Replace static std::mutex with Mutex for DSO handle write lock. |
| src/native/clr/include/host/fastdev-assemblies.hh | Replace override directory lock from std::mutex to Mutex. |
| src/native/clr/include/host/assembly-store.hh | Replace assembly decompress lock from std::mutex to Mutex. |
| src/native/clr/host/fastdev-assemblies.cc | Replace std::lock_guard usage with MutexGuard. |
| src/native/clr/host/assembly-store.cc | Replace internal state_lock and std::lock_guard usage with Mutex/MutexGuard. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
9b98570 to
83ca915
Compare
83ca915 to
d43d7d7
Compare
11cdb27 to
36f1cc4
Compare
36f1cc4 to
85c0bf3
Compare
85c0bf3 to
875ef58
Compare
875ef58 to
a3811d6
Compare
a3811d6 to
82ea5c4
Compare
82ea5c4 to
ba25053
Compare
ba25053 to
2507635
Compare
2507635 to
df1152c
Compare
a6019df to
2486f72
Compare
2486f72 to
74fc6f2
Compare
`std::mutex` and `std::lock_guard` are thin wrappers over pthreads, but using them makes the runtime depend on libc++. Add `Mutex` and `MutexGuard` in `common/include/runtime-base/mutex.hh` and use them instead. `Mutex` uses `PTHREAD_MUTEX_INITIALIZER` as a default member initializer and has a `constexpr` default constructor, so the four static instances (`assembly_decompress_mutex`, `override_dir_lock`, `dso_handle_write_lock` and `Timing::sequence_lock`) are constant-initialized. That means they need neither dynamic initialization nor a thread-safe initialization guard - the number of `__cxa_guard_*` references is unchanged by this commit. This drops all eight `std::__ndk1::mutex` references from `libnet-android.release-static-release.a`, taking the host from 48 to 40 undefined libc++ symbols. `Timing` lives in the shared `common` sources, so this affects the MonoVM host too; it still links libc++ for other reasons, and the behaviour is unchanged either way. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Reuse a single pthread mutex wrapper instead of adding a second one. `mono/shared/cppcompat.hh` already contained `xamarin::android::mutex` and `xamarin::android::lock_guard`, added for exactly the same reason: `<mutex>` makes the runtime depend on libc++. Rather than maintain two wrappers with the same purpose, delete `cppcompat.hh` and move the MonoVM host over to the shared `Mutex`/`MutexGuard` in `common/include/runtime-base/mutex.hh`. `Mutex` is the stricter of the two: it deletes the copy and move operations, which the old `mutex` left implicitly defined even though copying a `pthread_mutex_t` is never correct. It is also explicitly `constexpr` default constructible, so static instances stay constant-initialized. Also refresh the two comments explaining why `NDEBUG` is defined before including `robin_map.h`. They claimed `<mutex>` "conflicts with our std::mutex definition in cppcompat.hh", which stopped being true once the wrapper moved into the `xamarin::android` namespace. The hack is still worth keeping, but the real reason is that `<iostream>` and `<mutex>` would both pull in libc++. Finally, value-initialize `dso_handle_write_lock` for consistency with the other static `Mutex` instances. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Drop the RAII guard in favour of calling pthread_mutex_lock/unlock through Mutex directly. Critical sections that used to return, break or continue while holding the lock now delegate to a `_locked` helper that holds the branching logic, so each locked region has exactly one entry and one exit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Drop the Mutex class and use pthread_mutex_t with pthread_mutex_lock/unlock at the call sites. PTHREAD_MUTEX_INITIALIZER keeps the static instances constant-initialized, so they still need no thread-safe initialization guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Keep cppcompat.hh and the MonoVM hosts's use of it as they were. The CoreCLR host no longer shares a mutex wrapper with MonoVM, so there is no reason for this change to reach into src/native/mono. Timing lives in the shared common sources, so it is still converted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The previous commit split every locked region that contained an early exit into an outer method (which locks) and an inner `_locked` method (which holds the logic). That kept the lock/unlock pairing obvious, but it introduced six new methods and a helper enum purely to work around `return` statements. Restructure the locked regions in place instead: use a result flag plus `break`/`if`-`else` so control always falls through to the unlock, and move the early `return` after it. This drops all six helpers along with the `ReserveResult` enum and keeps the diff against the original code much smaller. * `writer_loop` uses `have_request` / `write_failed` * `enqueue_write` restores the original `queue_full` bool and adds `writes_allowed` * `get_available_sequence` uses `ret == nullptr` + `break` * `open_assembly` inverts the `opendir` check and re-tests `override_dir_fd` after unlocking No behavioural change. libc++ references are unchanged at 40 (CoreCLR) and 0 (NativeAOT), and `__cxa_guard_*` stays at 8, confirming the statics are still constant-initialized. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The previous two commits reshaped the locked regions to avoid returning while the lock was held: first by splitting them into `_locked` helpers, then by threading result flags so control fell through to the unlock. Both worked, but both restructured code that did not otherwise need to change. Just call `pthread_mutex_unlock` immediately before the early `return` instead. The regions keep their original shape, so the diff against the pre-existing code is now purely mechanical -- a type change, a `std::lock_guard` turning into a `pthread_mutex_lock`, and an added unlock. Churn against the base drops from 145 changed lines to 62. This is safe because the native runtime is built with `-fno-exceptions` (verified in `compile_commands.json`), so there is no unwind path that `std::lock_guard` would have covered and a manual unlock would miss. Verified that every `return` inside a locked region is immediately preceded by an unlock of that mutex, across all 13 regions. libc++ references remain 40 (CoreCLR) and 0 (NativeAOT), and `__cxa_guard_*` stays at 8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
74fc6f2 to
4796b15
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors, 1 warning, 0 suggestions.
The direct pthread conversion is otherwise mechanically consistent, keeps the static mutexes constant-initialized, and preserves the startup-aware locking behavior. The remaining issue is that Timing no longer inherits std::mutex’s non-copyable semantics; its copy/move operations should be explicitly deleted before merging.
CI is still in progress: 41 of 44 checks had passed, with two running and one queued at review time; no failures were reported.
Generated by Android PR Reviewer for #12541 · gpt56 · 82.1 AIC · ⌖ 20.2 AIC · ⊞ 25.7K
Comment /review to run again
Explicitly delete copy and move operations after replacing std::mutex with pthread_mutex_t. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move the existing MonoVM pthread mutex abstraction into the shared runtime headers and use its RAII guard throughout CoreCLR. This keeps one non-copyable mutex implementation across runtime flavors while preserving constant initialization and avoiding libc++ symbols. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Store pthread_mutex_t directly at every call site and have the shared RAII guard lock and unlock it without an intermediate Mutex class. Keep Timing explicitly non-copyable because it owns mutex storage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rename MutexGuard to pthread_mutex_guard to make its direct pthread_mutex_t ownership explicit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use xamarin::android::lock_guard for the concrete pthread_mutex_t RAII guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Restore src/native/mono to the PR base, move the new lock_guard into the CoreCLR include tree, and keep shared Timing on direct pthread calls so MonoVM does not consume the CoreCLR helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
#12545) Part of #12533. Builds on the mutex changes from #12541, which is now merged into `main`. ## Why The CoreCLR host's timing support still reached into `libc++` for memory management: - `Timing::sequence_pool` was a `std::vector<managed_timing_sequence>`. It returned pointers to managed code as `IntPtr`; growing the vector could move its elements and invalidate every outstanding pointer. - `Host::_timing` held the single process-lifetime `Timing` instance in a `std::shared_ptr`, despite there being no shared ownership or dynamic lifetime. - `FastTiming` still used `new`/`delete`, `std::stack`, `std::string`, `std::function`, and `std::chrono` in paths that do not need those abstractions. ## Final implementation ### Stable managed timing sequences `Timing` allocates sequences in linked chunks of 16. Chunks never move or free, so every address handed to managed code remains valid for the process lifetime. Entries are recycled through `in_use`; a double release remains a harmless redundant store rather than corrupting a free list. Allocation failure aborts consistently with other required runtime allocations. With the vector gone, `Timing` contains only constant-initialized state. CoreCLR stores it directly as a `static inline` process-lifetime instance, and callers use `FastTiming::enabled()` rather than a nullable ownership pointer. ### Allocation-free open-event stack `FastTiming` event records already live at stable addresses in process-lifetime chunks. Each `TimingEvent` now carries a `previous_open_event` link, and the per-thread stack is a single `thread_local TimingEvent*`. Starting and ending an event performs no allocation, requires no lock, and needs no TLS destructor or `__cxa_thread_atexit` registration. An unbalanced sequence cannot leak a separately allocated stack node. Event chunks use `calloc`/`free`, preserving stable references while avoiding `operator new`/`operator delete`. ### Plain timing strings and file names `TimingEvent::more_info` is a NUL-terminated `char*` built with one `malloc` and direct copies. Because timing is diagnostic, allocation failure drops only that event's optional detail. Timing output file names use a 128-byte inline buffer for normal values and `malloc`-owned storage for longer bundled `debug.mono.timing` / `debug.dotnet.timing` values, which are not constrained by Android's system-property limit. If that fallback allocation fails, timing warns and writes to the default `timing.txt` instead of aborting the application. The inline buffer, null heap pointer, and configured flag remain constant-initialized. ### Removing unnecessary type erasure and `chrono` - `FastTiming::dump` uses a plain line-writer function pointer plus a typed `FILE*` context instead of `std::function`. - `AssemblyStore::configure_from_payload` accepts its diagnostic `const char*` directly instead of wrapping a lambda and allocating a temporary `std::string`. - Timestamps are plain `uint64_t` nanosecond counts from `clock_gettime(CLOCK_MONOTONIC)`. A shared `time_interval` reproduces the former `duration_cast` output exactly, including total milliseconds and nanoseconds within the final millisecond. MonoVM shares `Timing` and receives the allocator changes; `monodroid-glue.cc` also formats the new plain-nanosecond interval. ## Results | | libc++ refs | `__cxa_guard_*` | |---|---:|---:| | #12541 baseline | 47 | 10 | | this PR | **38** | **8** | The removed references include the `std::shared_ptr` control-block family, one `operator new`, one `__libcpp_verbose_abort`, and one pair of `__cxa_guard_acquire` / `__cxa_guard_release`. The `__cxa_thread_atexit` category is eliminated from the timing path. NativeAOT remains at 0. The simple ARM64 CoreCLR APK decreases by 24 KiB (0.36%); `libmonodroid.so` itself decreases by 27,144 bytes (5.05%). The four affected APK-size references are updated from CI output. ## Coverage - The timing duration contract is pinned at compile time: `1,500,000,123 ns` remains `1:1500::123`. - The device integration test grows the concurrent event store beyond one chunk and dumps it. - The same test covers both the short Android system-property filename path and a 144-byte bundled-property filename that requires heap fallback. - CoreCLR, MonoVM, and NativeAOT build paths are covered by the full CI matrix.
…12571) Part of #12533: remove the remaining allocating C++ library types from the CoreCLR assembly store and its decompressed-assembly cache. <!-- stack-prerequisites --> All prerequisites (#12541, #12545, #12551, #12552, #12560, #12568, and #12570) have merged. This branch was rebuilt on `main` at `4b0cc8efa9`. The runtime changes are confined to `src/native/clr/host/assembly-store.cc`, with cache regression coverage and APK size baseline updates alongside them. The already-merged startup, timing, DSO-loader, and other prerequisite changes are no longer included in this PR's diff. Upstream's pthread-backed `lock_guard` is preserved. <!-- /stack-prerequisites --> ### Changes - Replace `std::deque<WriteRequest>` with an intrusive FIFO linked through `WriteRequest::next`. - Replace the queued request's `std::unique_ptr<uint8_t[]>` with an explicit `uint8_t *payload`. Allocate the request and payload separately with two `malloc` calls and release both with two `free` calls on every completed or discarded write. If payload allocation fails, release the request and skip the write. No trailing-storage pointer arithmetic or explicit over-alignment is needed. - Replace the cache directory and per-request `std::string` paths with a process-lifetime directory string and checked `snprintf` formatting. Requests store the descriptor index instead of a path; the writer reconstructs the destination from the immutable cache directory. Replace temporary-file name construction and matching with `snprintf` and `strstr`. - Use a scoped `CachePath` buffer for directory creation, reads, writes, and stale-file cleanup. Short paths remain on the stack; longer paths retry in an exactly sized `malloc` allocation that is freed on every exit. Formatting and allocation failures are logged and remain non-fatal for the optional cache, unlike the abort-on-failure `Util::format_with_retry` helper. - Allocate the decompression tracking array and assembly-name table with `calloc`, replacing their `unique_ptr`/`new[]` allocations. - Extend the existing device cache regression test with a Java Application that supplies a code-cache directory longer than 1 KB. Cover persistence, mapping, corruption recovery, and stale temporary-file cleanup for both ordinary and long paths. The asynchronous writer, queue byte limit, cache footer validation, and scope-based pthread locking are retained. `Util::LocalPathBufferSize` is the stack fast-path size, not a hard path limit; truncated paths are never used for I/O. The change does not alter linker flags or other runtime hosts. ### Original measurements These results were recorded on the original pre-rebuild branch, **not on the current revision**: | Release, arm64 | Before | After | |---|---:|---:| | Undefined libc++ references in `assembly-store.cc.o` | 12 | 0 | | Undefined libc++ references across the CoreCLR host | 12 | 0 | | `libnet-android.release.so` size | 520,112 bytes | 203,776 bytes | Reaching zero references allowed the linker to stop pulling members from `libc++_static.a`. Relinking without libc++ also succeeded on that original branch; dropping the linker flag remains a follow-up. ### Current validation status The malloc fallback passed sanitizer-backed host cases covering stack/heap boundaries, nested lifetimes, allocation failure, and formatting/retry failures. The cache namespace extracted unchanged from the current source, using the real CRC32 and mutex helpers with runtime configuration/property stubs, compiled with the Android NDK for arm64, arm32, and x86_64. A standalone arm64 emulator harness passed cache initialization, asynchronous persistence, mmap reads, corruption recovery, and stale-file cleanup with both short and greater-than-1-KB paths. The generated Java regression-test application also compiled against the Android API. `git diff --check` passes. A local `dotnet build src/native/native-clr.csproj` restored packages but could not reach native compilation because this worktree lacks `xa-prep-tasks.dll` and `Xamarin.Android.Tools.BootstrapTasks.dll`. The complete native host build and repository device regression suite have **not** been run for this revision; the locally built SDK is unavailable. Binary measurements have not been repeated. The original branch's reported full native builds and binary measurements remain historical, not validation of this revision.
Part of #12533 (drop
libc++from the CoreCLR host). Stacked on top of #12534.std::mutexis a thin wrapper overpthread_mutex_t, but using it pulls<mutex>and out-of-line libc++ symbols into the native host. This PR storespthread_mutex_tdirectly and provides a small CoreCLR RAII guard so locked scopes retain automatic unlock behavior without libc++.What
std::mutexinstances withpthread_mutex_t, initialized usingPTHREAD_MUTEX_INITIALIZER.xamarin::android::lock_guardunder the CoreCLR include tree. It takespthread_mutex_t&and callspthread_mutex_lock/pthread_mutex_unlock.StartupAwareLockto takepthread_mutex_t&directly.Timingto direct pthread calls and keep it explicitly non-copyable and non-movable.src/native/monounchanged; MonoVM retains its existingmutexand templatedlock_guardimplementation inmono/shared/cppcompat.hh.PTHREAD_MUTEX_INITIALIZERkeeps static instances constant-initialized, so this adds no thread-safe initialization guards. The CoreCLR guard is header-only and compiles down to direct pthread calls.Effect
This removes the last
#include <mutex>in the repository:<mutex>std::mutex/std::lock_guardusesIt also drops eight undefined libc++ references:
std::__ndk1::mutex::lock()std::__ndk1::mutex::unlock()std::__ndk1::mutex::~mutex()__cxa_guard_*The link-time
libc++requirement only disappears when every cause reaches zero, so this is one of several prerequisites rather than a self-sufficient win. The remaining causes (operator new/delete[],__cxa_guard_*,std::string,__libcpp_verbose_abort) are tracked in #12533.Verification
fastdev-assemblies.ccis#if defined(DEBUG)and was additionally compiled with-DDEBUG.pthread_mutex_lockandpthread_mutex_unlock; it introduces no C++ exception-runtime or initialization-guard symbols.libc++references: CoreCLR 55 → 47, NativeAOT 0. The eight removed references are exactly the mutex members listed above.__cxa_guard_*undefined references remain at 10, confirming that the static mutexes remain constant-initialized.