[native] Remove libc++ dependencies from the CoreCLR assembly store - #12571
Open
simonrozsival wants to merge 6 commits into
Open
[native] Remove libc++ dependencies from the CoreCLR assembly store#12571simonrozsival wants to merge 6 commits into
simonrozsival wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/native/clr/host/assembly-store.cc — format_cache_path() currently fails silently. If cache_dir is near… |
What changed in this PR
This PR continues the CoreCLR-host libc++ removal work by refactoring assembly-store.cc to eliminate remaining std::string/std::deque/std::unique_ptr usage in the decompressed-assembly cache path and write-queue implementation, bringing the host to zero undefined libc++ references.
Changes:
- Replaces
std::deque<WriteRequest>with an intrusive FIFO (WriteRequest::next) and moves request payload inline (single allocation per request). - Converts cache path construction from
std::stringbuilding to boundedsnprintfintoUtil::LocalPathBufferSizestack buffers. - Replaces
new[]-allocated tracking/name tables withcalloc/free, and makescache_dirastrdup-owned immutable C string.
| File | Description |
|---|---|
| src/native/clr/host/assembly-store.cc | Removes remaining libc++-driven types from the decompressed-assembly cache (paths, write queue, tracking/name tables) using C allocation + fixed buffers. |
simonrozsival
added
the
drop-libcpp
simonrozsival
marked this pull request as draft
September 3, 2026 06:13
Base automatically changed from
dev/simonrozsival/clr-host-drop-strings
to
main
September 5, 2026 21:31
simonrozsival
marked this pull request as ready for review
September 7, 2026 11:22
Context: #12533 `assembly-store.cc` was the last file in the CoreCLR host referencing libc++. With it converted the host is at **zero** undefined libc++ references, which means the linker now pulls nothing out of `libc++_static.a` at all. The decompressed-assembly cache accounted for all of it: * `cache_dir` was a `std::string` built by repeated `append`. It is now a single `snprintf` into a stack buffer which the store ID is appended to in place, then `strdup`ed for the lifetime of the process. Both directory levels are still created and validated in turn. * `build_path` returned a `std::string` per call. It becomes `format_cache_path`, formatting into a caller-supplied buffer. * `WriteRequest` held a `std::string path` and a `std::unique_ptr<uint8_t[]> data`. The path is gone entirely - `cache_dir` is immutable once the cache is enabled, so the writer thread rebuilds the path from `descriptor_index` alone - and the payload now lives immediately after the structure, making a request and its bytes a single allocation instead of two. * `std::deque<WriteRequest>` becomes an intrusive FIFO threaded through `WriteRequest::next`. The queue is only ever pushed at the tail and popped at the head, so a singly linked list is a complete replacement; it also removes the per-request node allocation the deque made on top of the payload allocation. * `tracking` was a `std::unique_ptr<uint8_t*[]>` and `assembly_store_names` a `new std::string_view[]`. Both are now `calloc`/`free`. * The two `std::to_string` calls and the `.tmp.` scan in `remove_stale_temp_files` become `snprintf` and `strstr`. Both `snprintf`-formatted paths are bounded by `Util::LocalPathBufferSize` and checked for truncation, where the `std::string` versions grew without limit. A path that long could not be opened anyway, and the failure is logged and disables the cache rather than being silently ignored. Behaviour is otherwise unchanged. Allocation failure still disables the cache instead of taking the process down, except for `assembly_store_names`, which is required for correct assembly lookup and where `new[]` would previously have aborted anyway - exceptions are disabled, so its failure called `std::terminate`. Undefined libc++ references in the CoreCLR host, Release, arm64: | object | before | after | |---------------------|-------:|------:| | `assembly-store.cc.o` | 12 | **0** | | **host total** | **12** | **0** | | | before | after | delta | |-----------------------------|--------:|--------:|-----------:| | `libnet-android.release.so` | 520,112 | 203,776 | **-316,336 B** | The size drop is far larger than the source change because reaching zero references means the linker stops pulling members out of `libc++_static.a` entirely. * CoreCLR, MonoVM and NativeAOT all build clean. * `llvm-nm --undefined-only` over every CoreCLR host object reports 0 libc++ references; the 12 present before this change are gone. * The resulting `.so` still exports `JNI_OnLoad` and the `Java_mono_android_Runtime_*` entry points, and its `DT_NEEDED` list is unchanged. * Relinking with `-nostdlib++` in place of `-static-libstdc++` succeeds, confirming that the host no longer needs libc++ at link time. Actually dropping the flag is left to a follow-up. Only `android-arm64` was built locally; the other ABIs rely on CI. The cache itself is exercised by the existing assembly store tests. Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
force-pushed
the
dev/simonrozsival/clr-assembly-store-strings
branch
from
September 7, 2026 11:30
130d10e to
f3ab6cb
Compare
Keep WriteRequest and its payload in one allocation, but initialize an explicit payload pointer once instead of computing it through an accessor at each use. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
changed the title
Use separate malloc allocations for WriteRequest and its payload, remove the unnecessary over-alignment, and free both allocations on all cleanup paths. Release the request if payload allocation fails and log skipped writes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
ready-to-review
Member
Author
|
/review |
simonrozsival
removed
the
ready-to-review
Contributor
|
✅ Android PR Reviewer completed successfully!
|
Report bounded path formatting failures through the existing cache logger, using ENAMETOOLONG for truncation and errno for snprintf errors. Cover entry paths, temporary files, stale-file cleanup, and store directories while preserving cache enablement and failure handling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors, 1 warning.
The explicit request/payload ownership, FIFO queue accounting, bounded path handling, and failure cleanup are internally consistent. The remaining concern is validation of the PR’s defining invariant: normal linking does not prove that the CoreCLR host has no undefined libc++ references, so this needs an automated assertion or a current-head binary measurement.
CI for current head 739795f9dc (Azure DevOps build 1586402) has just started and is still in progress; the earlier run was green.
Generated by Android PR Reviewer for #12571 · gpt56 · 123.9 AIC · ⌖ 5.52 AIC · ⊞ 26.3K
Comment /review to run again
Keep ordinary paths on the stack and retry oversized paths in an exactly sized malloc buffer. Use scoped ownership so all early exits release temporary allocations, and keep formatting/allocation errors non-fatal for the optional cache rather than using the aborting Util helper. Apply the fallback to initialization, reads, writes, and stale-file cleanup. Extend the device cache regression test with a Java Application that supplies a code-cache directory longer than 1 KB, checking persistence, corruption recovery, mapping, and stale temporary-file removal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #12533: remove the remaining allocating C++ library types from the CoreCLR assembly store and its decompressed-assembly cache.
All prerequisites (#12541, #12545, #12551, #12552, #12560, #12568, and #12570) have merged. This branch was rebuilt on
mainat4b0cc8efa9. The runtime changes are confined tosrc/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-backedlock_guardis preserved.Changes
std::deque<WriteRequest>with an intrusive FIFO linked throughWriteRequest::next.std::unique_ptr<uint8_t[]>with an explicituint8_t *payload. Allocate the request and payload separately with twomalloccalls and release both with twofreecalls 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.std::stringpaths with a process-lifetime directory string and checkedsnprintfformatting. 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 withsnprintfandstrstr.CachePathbuffer for directory creation, reads, writes, and stale-file cleanup. Short paths remain on the stack; longer paths retry in an exactly sizedmallocallocation that is freed on every exit. Formatting and allocation failures are logged and remain non-fatal for the optional cache, unlike the abort-on-failureUtil::format_with_retryhelper.calloc, replacing theirunique_ptr/new[]allocations.The asynchronous writer, queue byte limit, cache footer validation, and scope-based pthread locking are retained.
Util::LocalPathBufferSizeis 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:
assembly-store.cc.olibnet-android.release.sosizeReaching 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 --checkpasses.A local
dotnet build src/native/native-clr.csprojrestored packages but could not reach native compilation because this worktree lacksxa-prep-tasks.dllandXamarin.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.