Skip to content

[native] Drop std::binary_semaphore and the last string in the DSO loader - #12560

Merged
simonrozsival merged 4 commits into
mainfrom
dev/simonrozsival/clr-drop-std-semaphore
Sep 3, 2026
Merged

simonrozsival merged 4 commits into
mainfrom
dev/simonrozsival/clr-drop-std-semaphore

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Aug 28, 2026 •

Copy link
Copy Markdown
Member

PR relationship: No code prerequisite; this PR is based directly on main. The absolute CoreCLR totals below were measured in the former serialized stack.

Part of the drop-libc++ effort.

This clears the remaining libc++ references from android-system.cc.o, taking that object from 5 to 0 and the CoreCLR total from 31 to 26.

None of the five references had anything to do with path handling — they came from two small header-only helpers:

std::binary_semaphore -> sem_t

try_acquire_for pulled in std::chrono::steady_clock::now and libc++'s __libcpp_timed_backoff_policy, and release pulled in __cxx_atomic_notify_all. (This was the unexplained Release-mode steady_clock reference noted earlier in the stack.)

The replacement is plain POSIX sem_t from <semaphore.h> — a real semaphore that lives in libc, not libc++, and needs no wrapper type.

An earlier revision of this PR hand-rolled a BinarySemaphore on top of pthread_mutex_t/pthread_cond_t so the timeout could use CLOCK_MONOTONIC. That was reimplementing a primitive libc already ships, so it is gone: sem_timedwait() supports CLOCK_REALTIME only until API 28 (sem_timedwait_monotonic_np() is __INTRODUCED_IN(28), sem_clockwait() is API 30, and we support API 24), which means a wall clock adjustment inside the window can cut the 3s wait short or stretch it. For a sanity timeout on loading a shared library that is a fine trade for deleting 99 lines of hand-written synchronization code.

sem_timedwait() takes an absolute deadline, so retrying after EINTR cannot extend the total wait, and no nanosecond normalization is needed because the timeout is a whole number of seconds.

The gratuitous virtual destructor

MainThreadDsoLoader is never derived from and only ever lives on the stack (dso-loader.hh:169), but its destructor was virtual. That made the compiler emit the deleting destructor D0Ev, which references operator delete.

The NUL-termination std::string

SystemLoadLibraryWrapper::load created a std::string purely to get a NUL-terminated copy of a std::string_view to hand to NewStringUTF. It now uses a stack buffer with a heap fallback, with the actual loading split into a const char* overload so there is a single place to free the copy.

Results

before after
android-system.cc.o refs 5 0
CoreCLR total refs 31 26
libnet-android.release.so 536,368 525,904 (−10,464)

NativeAOT stays at 0 refs; MonoVM and NativeAOT both still build clean (these are common/ headers, so MonoVM benefits too).

Copilot AI lite review requested due to automatic review settings August 28, 2026 06:33

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.

Copilot review overview

Review tier: Lite
Findings: 2 Medium severity

New issues introduced by this change (2)
Severity Finding
Medium severity src/​native/​common/​include/​runtime-base/​system-loadlibrary-wrapper.hh — ❌ error: java_lib_name is a local JNI reference created via NewStringUTF() and is never…
Medium severity src/​native/​common/​include/​shared/​binary-semaphore.hh — 💡 suggestion: clock_gettime() return value is currently ignored. If it ever fails, deadline…
What changed in this PR

This PR advances the “drop-libc++” effort by removing remaining libc++-pulling constructs from the DSO loading path: replacing std::binary_semaphore with a pthread-based BinarySemaphore, removing an unnecessary virtual destructor, and avoiding std::string allocation just to NUL-terminate a std::string_view for JNI.

Changes:

  • Introduces BinarySemaphore (pthread mutex/condvar) as a replacement for std::binary_semaphore in the main-thread DSO loader path.
  • Refactors SystemLoadLibraryWrapper::load to use a stack buffer with heap fallback instead of std::string for JNI NUL-terminated input.
  • Removes the virtual destructor from MainThreadDsoLoader to avoid emitting a deleting destructor (and operator delete).
File Description
src/​native/​common/​include/​shared/​binary-semaphore.hh Adds pthread-based binary semaphore with monotonic-clock timeout logic.
src/​native/​common/​include/​runtime-base/​system-loadlibrary-wrapper.hh Replaces std::string copy with stack/heap buffer and splits out const char* overload for JNI loading.
src/​native/​common/​include/​runtime-base/​mainthread-dso-loader.hh Switches to BinarySemaphore and removes unnecessary virtual destructor.

Comment thread src/native/common/include/shared/binary-semaphore.hh Outdated
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 8707bd1 to 43bdbbb Compare August 28, 2026 07:14
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from ed042fa to 4908d80 Compare August 28, 2026 07:14
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 43bdbbb to cc2c3a8 Compare August 28, 2026 07:54
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from 4908d80 to 9eaa808 Compare August 28, 2026 07:54
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from cc2c3a8 to 6b74b72 Compare August 28, 2026 08:47
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from 9eaa808 to 7fa16a9 Compare August 28, 2026 08:47
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 6b74b72 to f2a7c53 Compare August 28, 2026 08:56
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from 7fa16a9 to d28ecc6 Compare August 28, 2026 08:56
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from f2a7c53 to 7ee6aa9 Compare August 28, 2026 09:52
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from d28ecc6 to b6471e9 Compare August 28, 2026 09:52
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 7ee6aa9 to 8c831f8 Compare August 28, 2026 10:29
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch 4 times, most recently from 04f2a89 to d4d8482 Compare August 28, 2026 11:50
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from f9a1d0e to d4409ce Compare August 28, 2026 12:06
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from d4d8482 to ba36ae9 Compare August 28, 2026 12:06
@simonrozsival simonrozsival added the drop-libcpp Work to remove the libc++ dependency from Android NativeAOT label Aug 28, 2026
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from f61c6e5 to e808135 Compare August 28, 2026 12:35
simonrozsival and others added 4 commits September 3, 2026 08:15
…ader

This clears the remaining `libc++` references from `android-system.cc.o`,
bringing that object down from 5 to 0 and the CoreCLR total from 31 to 26.

None of the five references had anything to do with path handling:

  * `std::binary_semaphore::try_acquire_for` pulled in
    `std::chrono::steady_clock::now` and libc++'s timed backoff policy, and
    `release` pulled in `__cxx_atomic_notify_all`. Replace it with a small
    `BinarySemaphore` built directly on `pthread_mutex_t`/`pthread_cond_t`,
    which is the primitive already used elsewhere in the tree. As a bonus it
    waits on `CLOCK_MONOTONIC`, so the timeout is no longer affected by wall
    clock adjustments.

  * `MainThreadDsoLoader`'s destructor was `virtual` even though the class is
    never derived from and only ever lives on the stack. That made the
    compiler emit the deleting destructor, which references `operator delete`.

  * `SystemLoadLibraryWrapper::load` created a `std::string` purely to get a
    NUL-terminated copy of a `std::string_view`. Use a stack buffer with a
    heap fallback instead, and split the actual loading into an overload
    taking a `const char*` so there is a single place to free the copy.

`libnet-android.release.so` shrinks by 10,464 bytes (536,368 -> 525,904).

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
- `SystemLoadLibraryWrapper::load ()` never released the local reference
  returned by `NewStringUTF ()`. This runs while loading the application's
  shared libraries, before control returns to Java, so nothing reclaims the
  references in between and the local reference table can fill up. Delete the
  reference once `CallStaticVoidMethod ()` returns. `DeleteLocalRef ()` is safe
  to call with a pending exception, so it can happen before the exception check.

- `BinarySemaphore::try_acquire_for ()` ignored the return value of
  `clock_gettime ()`. On failure `deadline` stayed zero, which makes
  `pthread_cond_timedwait ()` return `ETIMEDOUT` right away and turns the wait
  into a silent spurious timeout. Abort instead.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The previous commit replaced `std::binary_semaphore` with a `BinarySemaphore`
class built on `pthread_mutex_t`/`pthread_cond_t`. That was reimplementing a
primitive libc already provides: `sem_t` from `<semaphore.h>` is a POSIX
semaphore, lives in libc rather than libc++, and needs no wrapper at all.

Delete the 99-line header and use `sem_init()`/`sem_post()`/`sem_timedwait()`
directly.

The only reason to prefer a condition variable here was that
`sem_timedwait()` supports `CLOCK_REALTIME` only until API 28
(`sem_timedwait_monotonic_np()` is `__INTRODUCED_IN(28)` and `sem_clockwait()`
is API 30, while we support API 24), so a wall clock adjustment inside the
window can cut the 3s wait short or stretch it. For a sanity timeout on
loading a shared library that is an acceptable trade for deleting a
hand-written synchronization primitive.

`sem_timedwait()` also takes an *absolute* deadline, so unlike the relative
timeout it replaces, retrying after `EINTR` cannot extend the total wait, and
the deadline needs no nanosecond normalization because the timeout is a whole
number of seconds.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The deadline arithmetic and the `EINTR` retry loop obscured what `load()` is
actually doing. Move them into a small `try_acquire_for()` helper so the wait
reads as a single line again, as it did when this was a
`std::binary_semaphore`.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 56d80d9 to a752d5a Compare September 3, 2026 06:15
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from e808135 to 54a7fa6 Compare September 3, 2026 06:15
@simonrozsival
simonrozsival changed the base branch from dev/simonrozsival/clr-android-system-paths to main September 3, 2026 06:15
@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

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 #12560

@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.

✅ LGTM

Findings: 0 errors · 0 warnings · 1 suggestion

The POSIX semaphore replacement preserves the one-shot synchronization semantics while removing the libc++ dependency, and the JNI local reference is now released on both success and exception paths. I left one non-blocking inline suggestion to retain explicit clock_gettime() failure handling. All 44 reported CI checks completed successfully.

Generated by Android PR Reviewer for #12560 · gpt56 · 73.9 AIC · ⌖ 20.2 AIC · ⊞ 25.7K
Comment /review to run again

Comment thread src/native/common/include/runtime-base/mainthread-dso-loader.hh
simonrozsival added a commit that referenced this pull request Sep 3, 2026
…rapper (#12568)

Part of the [drop-libc++](#12533) effort.

Split out of #12560, where it had been sitting alongside the DSO loader changes despite being unrelated to them.

`jstring_array_wrapper` had two problems.

### A JNI local reference leak

`jstring_wrapper::release ()` bailed out early when it had no UTF chars to release:

```cpp
if (cstr == nullptr) {
    return;
}
```

But `jstring_array_wrapper::operator[]` fetches the array element's reference eagerly, while `cstr` is only populated on the first `get_cstr ()` call. So any element that was indexed but never read kept its local reference until the frame was popped. The reference and the UTF chars have independent lifetimes, so they are now released independently.

### `new[]` / `delete[]`

The wrapper allocated its elements with `new jstring_wrapper[len]`, which is where `_Znam` and `_ZdaPv` in `host.cc.o` came from. It now uses `malloc()` with explicit placement construction and destruction.

`jstring_wrapper` is not an implicit-lifetime type — it has a user-provided destructor — so `malloc()` alone cannot begin its lifetime; placement new is required. Its default constructor is private, with `jstring_array_wrapper` as a friend, which is what makes that legal here.

## Results

| | before | after |
|---|---|---|
| `host.cc.o` refs | 11 | **9** |
| CoreCLR total refs | 23 | **21** |

All three runtime lanes build clean.
@simonrozsival
simonrozsival merged commit 411cc92 into main Sep 3, 2026
44 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/clr-drop-std-semaphore branch September 3, 2026 16:37
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.

4 participants