Skip to content

[native] Replace std::mutex with pthread_mutex_t in the CoreCLR host - #12541

Merged
simonrozsival merged 13 commits into
mainfrom
dev/simonrozsival/clr-replace-std-mutex
Sep 3, 2026
Merged

simonrozsival merged 13 commits into
mainfrom
dev/simonrozsival/clr-replace-std-mutex

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Aug 27, 2026 •

Copy link
Copy Markdown
Member

PR relationship: Prerequisite: #12534 (merged). This is the bottom of stack #12653.

Part of #12533 (drop libc++ from the CoreCLR host). Stacked on top of #12534.

std::mutex is a thin wrapper over pthread_mutex_t, but using it pulls <mutex> and out-of-line libc++ symbols into the native host. This PR stores pthread_mutex_t directly and provides a small CoreCLR RAII guard so locked scopes retain automatic unlock behavior without libc++.

What

  • Replace CoreCLR's std::mutex instances with pthread_mutex_t, initialized using PTHREAD_MUTEX_INITIALIZER.
  • Add xamarin::android::lock_guard under the CoreCLR include tree. It takes pthread_mutex_t& and calls pthread_mutex_lock / pthread_mutex_unlock.
  • Update CoreCLR's StartupAwareLock to take pthread_mutex_t& directly.
  • Convert shared Timing to direct pthread calls and keep it explicitly non-copyable and non-movable.
  • Leave src/native/mono unchanged; MonoVM retains its existing mutex and templated lock_guard implementation in mono/shared/cppcompat.hh.

PTHREAD_MUTEX_INITIALIZER keeps 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:

files including <mutex> std::mutex / std::lock_guard uses
before 6 13
after 0 0

It also drops eight undefined libc++ references:

symbol before after
std::__ndk1::mutex::lock() 3 0
std::__ndk1::mutex::unlock() 3 0
std::__ndk1::mutex::~mutex() 2 0
refs __cxa_guard_*
#12534 (base) 55 10
this PR 47 10

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

  • CoreCLR, MonoVM and NativeAOT build clean.
  • fastdev-assemblies.cc is #if defined(DEBUG) and was additionally compiled with -DDEBUG.
  • The CoreCLR guard compiled with the runtime's no-C++-exceptions settings references only pthread_mutex_lock and pthread_mutex_unlock; it introduces no C++ exception-runtime or initialization-guard symbols.
  • Real 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 Mutex and MutexGuard wrappers around pthread_mutex_t in runtime-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

Comment thread src/native/common/include/runtime-base/mutex.hh Outdated
Comment thread src/native/clr/include/runtime-base/monodroid-dl.hh Outdated
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 9b98570 to 83ca915 Compare August 27, 2026 15:47
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 83ca915 to d43d7d7 Compare August 27, 2026 16:09
@simonrozsival simonrozsival changed the title [native] Replace std::mutex with a pthread wrapper in the CoreCLR host [native] Replace std::mutex with pthread_mutex_t in the CoreCLR host Aug 27, 2026
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 11cdb27 to 36f1cc4 Compare August 27, 2026 19:29
@simonrozsival
simonrozsival changed the base branch from dev/simonrozsival/clr-timing-raw-pointer to dev/simonrozsival/clr-remove-std-format August 27, 2026 19:34
@simonrozsival simonrozsival added the drop-libcpp Work to remove the libc++ dependency from Android NativeAOT label Aug 27, 2026
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 36f1cc4 to 85c0bf3 Compare August 27, 2026 21:42
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 85c0bf3 to 875ef58 Compare August 28, 2026 07:54
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 875ef58 to a3811d6 Compare August 28, 2026 08:47
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from a3811d6 to 82ea5c4 Compare August 28, 2026 08:55
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 82ea5c4 to ba25053 Compare August 28, 2026 09:51
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from ba25053 to 2507635 Compare August 28, 2026 10:29
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 2507635 to df1152c Compare August 28, 2026 12:06
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from a6019df to 2486f72 Compare September 2, 2026 13:20
Base automatically changed from dev/simonrozsival/clr-remove-std-format to main September 3, 2026 05:58
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-replace-std-mutex branch from 2486f72 to 74fc6f2 Compare September 3, 2026 05:58
simonrozsival and others added 7 commits September 3, 2026 08:13
`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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12541

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ 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

Comment thread src/native/common/include/runtime-base/timing.hh
simonrozsival and others added 6 commits September 3, 2026 12:43
Explicitly delete copy and move operations after replacing std::mutex with pthread_mutex_t.

Co-authored-by: Copilot App <[email protected]>
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 <[email protected]>
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 <[email protected]>
Rename MutexGuard to pthread_mutex_guard to make its direct pthread_mutex_t ownership explicit.

Co-authored-by: Copilot App <[email protected]>
Use xamarin::android::lock_guard for the concrete pthread_mutex_t RAII guard.

Co-authored-by: Copilot App <[email protected]>
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 <[email protected]>
@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Sep 3, 2026
@simonrozsival
simonrozsival merged commit 876c57e into main Sep 3, 2026
44 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/clr-replace-std-mutex branch September 3, 2026 16:41
simonrozsival added a commit that referenced this pull request Sep 7, 2026
#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.
simonrozsival added a commit that referenced this pull request Sep 8, 2026
…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.
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

drop-libcpp Work to remove the libc++ dependency from Android NativeAOT ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants