Repository navigation
[native] Drop std::binary_semaphore and the last string in the DSO loader - #12560
Conversation
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
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… |
|
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 forstd::binary_semaphorein the main-thread DSO loader path. - Refactors
SystemLoadLibraryWrapper::loadto use a stack buffer with heap fallback instead ofstd::stringfor JNI NUL-terminated input. - Removes the
virtualdestructor fromMainThreadDsoLoaderto avoid emitting a deleting destructor (andoperator 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. |
8707bd1 to
43bdbbb
Compare
ed042fa to
4908d80
Compare
43bdbbb to
cc2c3a8
Compare
4908d80 to
9eaa808
Compare
cc2c3a8 to
6b74b72
Compare
9eaa808 to
7fa16a9
Compare
6b74b72 to
f2a7c53
Compare
7fa16a9 to
d28ecc6
Compare
f2a7c53 to
7ee6aa9
Compare
d28ecc6 to
b6471e9
Compare
7ee6aa9 to
8c831f8
Compare
04f2a89 to
d4d8482
Compare
f9a1d0e to
d4409ce
Compare
d4d8482 to
ba36ae9
Compare
f61c6e5 to
e808135
Compare
…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
56d80d9 to
a752d5a
Compare
e808135 to
54a7fa6
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
✅ 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
…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.
…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 the drop-libc++ effort.
This clears the remaining
libc++references fromandroid-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_ttry_acquire_forpulled instd::chrono::steady_clock::nowand libc++'s__libcpp_timed_backoff_policy, andreleasepulled in__cxx_atomic_notify_all. (This was the unexplained Release-modesteady_clockreference noted earlier in the stack.)The replacement is plain POSIX
sem_tfrom<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
BinarySemaphoreon top ofpthread_mutex_t/pthread_cond_tso the timeout could useCLOCK_MONOTONIC. That was reimplementing a primitive libc already ships, so it is gone:sem_timedwait()supportsCLOCK_REALTIMEonly 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 afterEINTRcannot extend the total wait, and no nanosecond normalization is needed because the timeout is a whole number of seconds.The gratuitous
virtualdestructorMainThreadDsoLoaderis never derived from and only ever lives on the stack (dso-loader.hh:169), but its destructor wasvirtual. That made the compiler emit the deleting destructorD0Ev, which referencesoperator delete.The NUL-termination
std::stringSystemLoadLibraryWrapper::loadcreated astd::stringpurely to get a NUL-terminated copy of astd::string_viewto hand toNewStringUTF. It now uses a stack buffer with a heap fallback, with the actual loading split into aconst char*overload so there is a single place to free the copy.Results
android-system.cc.orefslibnet-android.release.soNativeAOT stays at 0 refs; MonoVM and NativeAOT both still build clean (these are
common/headers, so MonoVM benefits too).