Skip to content

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

Open
simonrozsival wants to merge 4 commits into
dev/simonrozsival/clr-android-system-pathsfrom
dev/simonrozsival/clr-drop-std-semaphore
Open

[native] Drop std::binary_semaphore and the last string in the DSO loader#12560
simonrozsival wants to merge 4 commits into
dev/simonrozsival/clr-android-system-pathsfrom
dev/simonrozsival/clr-drop-std-semaphore

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Part of the drop-libc++ effort. Stacked on #12552.

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 reviewed Aug 28, 2026
View reviewed changes

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/runtime-base/system-loadlibrary-wrapper.hh
Comment thread src/native/common/include/shared/binary-semaphore.hh Outdated
@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-drop-std-semaphore branch from 4908d80 to 9eaa808 Compare August 28, 2026 07:54
@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-drop-std-semaphore branch from 7fa16a9 to d28ecc6 Compare August 28, 2026 08:56
@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-drop-std-semaphore branch 4 times, most recently from 04f2a89 to d4d8482 Compare August 28, 2026 11:50
simonrozsival and others added 2 commits August 28, 2026 14:05
@simonrozsival
[native] Drop std::binary_semaphore and the last string in the DSO lo…
7373d89 
…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 <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
@simonrozsival
[native] Address the review feedback on the DSO loader
acafc80 
- `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 <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
@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 and others added 2 commits August 28, 2026 14:34
@simonrozsival
[native] Use a POSIX semaphore instead of a hand-rolled one
f3a6cb0 
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 <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
@simonrozsival
[native] Extract the DSO load wait into try_acquire_for()
e808135 
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 <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from f61c6e5 to e808135 Compare August 28, 2026 12:35
@simonrozsival simonrozsival mentioned this pull request Aug 28, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jonathanpeppers jonathanpeppers Awaiting requested review from jonathanpeppers jonathanpeppers is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

drop-libcpp Work to remove the libc++ dependency from Android NativeAOT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@simonrozsival