Repository navigation
[native] Remove the libc++ dependencies from the CoreCLR host's startup path - #12570
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/clr/host/fastdev-assemblies.cc — ERROR: FastDevAssemblies::tpa_in_use is set to true on success but never reset on failure. If… |
|
src/native/clr/host/host.cc — WARNING: This Debug-only FastDev path aborts the process if allocating the extended runtime… |
What changed in this PR
This PR refactors the CoreCLR native host FastDev/initialization code to remove dependencies on libc++/STL types (e.g., std::string, std::vector) by switching to C allocation patterns and existing in-tree path helpers.
Changes:
- Change FastDev TPA list construction to return a
malloc-ownedchar*instead of filling astd::string. - Replace STL-based path/property list handling in
host.ccwithUtil::join_paths,malloc/calloc, and explicit lifetime management. - Update FastDev assembly loading buffer allocation to use
malloc/freeinstead ofnew[]/delete[].
| File | Description |
|---|---|
| src/native/clr/include/host/fastdev-assemblies.hh | Updates the FastDev API to return an owned char* TPA list (no STL types). |
| src/native/clr/host/host.cc | Reworks CoreCLR init property handling and path building to avoid STL and manage memory explicitly. |
| src/native/clr/host/fastdev-assemblies.cc | Implements a growable C buffer for TPA list building and switches FastDev file buffer allocation to malloc. |
add6e9c to
6e8a14b
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
StackTraceContainsLineNumbers device test exercises the FastDev TPA behavior. The only feedback is a non-blocking utility-consolidation suggestion.
CI is not yet green: only license/cla has completed successfully, while no dotnet-android Azure DevOps checks are present for head 91690f72; the native ABI matrix therefore remains unvalidated.
Generated by Android PR Reviewer for #12570 · gpt56 · 99.4 AIC · ⌖ 9.54 AIC · ⊞ 25.7K
Comment /review to run again
`AndroidSystem` kept five of its members in `std::string`/`std::array<std::string>`: `primary_override_dir`, `native_libraries_dir`, `app_code_cache_dir`, `single_app_lib_directory` and `override_dirs`. Because they are `inline static` with dynamic initialization, the compiler emits a guard variable *and* an `atexit` registration for them in **every** translation unit that includes `android-system.hh` - even in ones that never touch them. `logger.cc`, `internal-pinvokes-clr.cc`, `internal-pinvokes-shared.cc` and `android-system-shared.cc` each paid four libc++ references (`~basic_string`, `operator delete`, `__cxa_guard_acquire`, `__cxa_guard_release`) without using a single one of these directories. Replace them with `path_buffer<N>`, a trivial aggregate holding an inline buffer plus an optional heap buffer. Being a POD, static instances are constant-initialized, so neither a guard variable nor an `atexit` registration is emitted. Paths that fit in `SENSIBLE_PATH_MAX` need no allocation at all and longer ones are moved to the heap, so - unlike the fixed `char[]` array NativeAOT used for `primary_override_dir` - there is no hard limit on the path length and no abort when it is exceeded. The directory arrays become plain `const char*` arrays whose entries are `malloc`ed, which also drops an `operator new[]` from the non-split-APK path. This lets `primary_override_dir` be shared by all three hosts, removing three `#if defined (XA_HOST_NATIVEAOT)` blocks and `determine_primary_override_dir()`. Undefined libc++ references in the CoreCLR archives: 58 -> 31. `libnet-android.release.so`: 539,464 -> 536,184 bytes (-3,280). Co-authored-by: Copilot <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The inline-buffer-plus-heap-fallback `path_buffer` was more machinery than these three values need. They are assigned exactly once, early during startup, and only read afterwards, so the inline buffer only ever saved a single `malloc` per value while costing 3 KB of `.bss`. Replace it with plain `const char*` members initialized to `""`. Pointers to a string literal are constant-initialized just like the aggregate was, so the guard variables and `atexit` registrations stay gone, which was the whole point of the change. The values are duplicated with a new `Util::duplicate_string()` helper, which aborts if the allocation fails. Also format the APK library directory with `snprintf` instead of open-coded `memcpy` calls - the exact length is computed up front, so the buffer is already known to be the right size. Undefined libc++ references are unchanged at 31. Co-authored-by: Copilot <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Addresses review feedback: - `app_lib_directories_size * sizeof (const char*)` is now computed with `Helpers::multiply_with_overflow_check`. - A zero-length array is handled explicitly. `malloc (0)` may legitimately return `nullptr`, which the previous code would have misreported as an allocation failure; `setup_apk_directories ()` already aborts with a more accurate message when no directory ends up being added. Co-authored-by: Copilot <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The view returned by `get_string_view ()` pointed at the UTF characters owned by the wrapper, so it dangled as soon as the wrapper released them. Nothing relied on the view being a view: two of the three callers immediately passed it to a path helper, and the third only needed a suffix comparison. Return the C string instead and let the callers build a view when they need one. `setup_apk_directories ()` used `std::string_view::ends_with ()`, so add a `Util::ends_with ()` that works on plain C strings. Co-authored-by: Copilot <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The hand-written copy existed to support a caller that passed a pointer and a length rather than a C string, but that caller formats its buffer with `snprintf ()` and only reaches the call when the result fits, so the buffer is already NUL terminated. With every caller passing a C string there is nothing left for `std::string_view` to do and the copy is just `strdup ()`. Keep the wrapper rather than calling `strdup ()` directly: it aborts on allocation failure, which saves each of the four callers from checking for null. Co-authored-by: Copilot <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The only caller of `get_full_dso_path ()` iterates over a container of `const char*` directories and wrapped each one in a `std::string_view` purely to satisfy the signature. Take a C string instead and measure it once inside `format_full_dso_path ()`. `dso_path` stays a view: it originates in the DSO cache lookup, which compares name mutations built with `substr ()`, so a view is the right type there. Co-authored-by: Copilot <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Part of #12533. This clears `host.cc` entirely, taking the CoreCLR host from 21 undefined libc++ references to 12 - all of which now live in `assembly-store.cc`. Three separate uses, two of which only compile in Release: * `scan_filesystem_for_assemblies_and_libraries` built the assembly store path with a `std::string`. It now uses `Util::join_paths`, added earlier in this stack, which keeps the path on the stack unless it does not fit and frees only when the returned pointer differs from the stack buffer. * `APP_CONTEXT_BASE_DIRECTORY` was held in a function-local `static std::string`. The static needs process lifetime because the value has to outlive `coreclr_initialize`, and a `std::string` has a non-trivial constructor, so it forced a guard variable - this was the only source of `__cxa_guard_acquire`/`__cxa_guard_release` in the file. A plain `char*` is constant-initialized, so the guard pair disappears. * The FastDev block used a `std::string` plus two `std::vector<const char*>`. It is inside `if constexpr (Constants::is_debug_build)`, so it contributes nothing to Release, but it has to go before libc++ can be dropped from Debug builds too. The property arrays are a fixed `prop_count + 1` entries, so they become `calloc`ed arrays. `FastDevAssemblies::build_tpa_list` consequently returns a `malloc`ed string the caller owns rather than filling a `std::string&` out-parameter. The TPA list has no useful upper bound - one absolute path per assembly in the override directory - so it is accumulated through a small growable buffer that doubles on demand. Allocation failure there falls back to the probe-only path rather than aborting, since FastDev is a debug convenience. `open_assembly` swaps `new uint8_t[]`/`delete[]` for `malloc`/`free`. That buffer is handed to CoreCLR and never freed (see the existing TODO), so this is not a behavioural change. It also gains a null check, which fixes a file descriptor leak on the allocation-failure path that `new` would have turned into a `std::bad_alloc` abort. `<string>` and `<vector>` are no longer included by `host.cc`, and `<string>` is gone from `fastdev-assemblies.hh`, which `host.cc` was picking it up from. Undefined libc++ references in the CoreCLR host, Release: | object | before | after | |---|---:|---:| | `host.cc.o` | 9 | **0** | | `assembly-store.cc.o` | 12 | 12 | | **total** | **21** | **12** | `libnet-android.release.so` goes from 523,224 to 520,112 bytes (-3,112). Debug is not built locally, so `host.cc` and `fastdev-assemblies.cc` were compiled standalone with `-DDEBUG -DDEBUG_BUILD` using the flags from `compile_commands.json`. Both compile clean and report 0 undefined libc++ references. * CoreCLR, MonoVM and NativeAOT all build clean. * Debug-mode compilation of both affected files verified as described above, which matters because `fastdev-assemblies.cc` is only added to the build under `if(DEBUG_BUILD)` and is therefore never compiled in Release. Co-authored-by: Copilot <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Reset FastDev TPA state on every rebuild and keep runtime property allocation failures on the probe-only path. Co-authored-by: Copilot App <[email protected]>
`FastDevAssemblies::tpa_in_use` only exists under `#if defined(DEBUG)`, but host.cc referenced it from an `if constexpr (Constants::is_debug_build)` branch. The discarded branch of an `if constexpr` in a non-template function is still name-looked-up, so Release builds failed with "no member named 'tpa_in_use'". Route the reset through a new `discard_tpa_list ()` which, like `open_assembly ()` and `build_tpa_list ()`, has a no-op Release stub. Co-authored-by: Copilot App <[email protected]>
Preserve empty-file handling without relying on malloc(0), close the override file descriptor after reading, and document the process-global lifetime assumed by the CoreCLR application base directory. Co-authored-by: Copilot App <[email protected]>
Force the existing path helper onto its heap-owned path so separator handling stays centralized while the CoreCLR property retains explicit process-lifetime ownership. Co-authored-by: Copilot App <[email protected]>
c968434 to
45e3d4e
Compare
Co-authored-by: Copilot App <[email protected]>
|
@jonathanpeppers please re-approve |
…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 #12533 (drop
libc++from the CoreCLR host).This clears
host.ccentirely. The CoreCLR host goes from 21 undefined libc++ references to 12, and every one that remains is now in a single file,assembly-store.cc.The three uses in
host.cc1. The assembly store path
scan_filesystem_for_assemblies_and_librariesjoined the native library directory and the store filename with astd::string. It now usesUtil::join_paths, added earlier in this stack, which keeps the path in a stack buffer unless it does not fit and frees only when the returned pointer differs from that buffer.2.
APP_CONTEXT_BASE_DIRECTORYstatic std::string app_context_base_directory;The static is needed because the value has to outlive
coreclr_initialize— CoreCLR keeps the pointer. Butstd::stringhas a non-trivial constructor, so a function-local static of that type forces a guard variable, and this was the only source of__cxa_guard_acquire/__cxa_guard_releasein the file. A plainchar*is constant-initialized, so the guard pair disappears along with the string. Runtime initialization is process-global, so replacing the stored value cannot invalidate a live CLR instance.3. The FastDev TPA list
This block is inside
if constexpr (Constants::is_debug_build), so it contributes nothing to the Release numbers below — but it has to go before libc++ can be dropped from Debug builds, which is why it is included here rather than left for later. The two property arrays are a fixedprop_count + 1entries, so they becomecalloced arrays.build_tpa_listnow returns the stringIt previously filled a
std::string&out-parameter and returnedbool; it now returns amalloced string the caller owns, ornullptr.The TPA list is the one place in this PR with no useful upper bound — it holds one absolute path per assembly in the override directory — so a fixed buffer is not appropriate. It is accumulated through a small growable buffer that doubles on demand.
FastDev allocation failures are non-fatal: both list construction and extended runtime-property allocation log a warning, free partial state, reset TPA mode and fall back to the probe-only path.
discard_tpa_list()owns that reset and has a no-op Release stub, avoiding references to Debug-only state from the discarded branch of the non-templateif constexpr.open_assemblynew uint8_t[]/delete[]becomemalloc/free. That buffer is handed to CoreCLR and never freed — see the existingTODOabove it about ownership — so this is not a behavioral change.It also gains a null check. That is a small bug fix:
new uint8_t[]would have thrownstd::bad_allocand aborted, whereas the new path logs, closes the file descriptor and returnsnullptr. The previous code would have leakedasm_fdhad it ever returned rather than aborted.Environment override buffer
The Debug environment override reader used
std::make_unique<char[]>. It now usesmalloc/free, preserves the previous non-fatal behavior for empty files without relying onmalloc(0), and closes the override file descriptor after reading. Read errors retain theirerrnovalue acrossclose()so diagnostics remain accurate.Header cleanup
<string>and<vector>are no longer included byhost.cc, and<string>is gone fromfastdev-assemblies.hh— which is wherehost.ccwas picking it up from in the first place.Results
Undefined libc++ references in the CoreCLR host, Release:
host.cc.oassembly-store.cc.oThe nine removed from
host.cc.o:__cxa_guard_acquire,__cxa_guard_release,operator new,operator delete,std::string::append,std::string::push_back,std::string::__grow_by_and_replace,~basic_stringand__libcpp_verbose_abort.libnet-android.release.soVerification
dotnet build src/native/native-clr.csproj --no-restore.fastdev-assemblies.ccis only added to the build underif(DEBUG_BUILD)(src/native/clr/host/CMakeLists.txt), so a Release build never compiles it and would not have caught a mistake in the FastDev work. There is no Debug CoreCLR build configured locally, so bothhost.ccandfastdev-assemblies.ccwere compiled standalone with-DDEBUG -DDEBUG_BUILDusing the exact flags fromcompile_commands.json. Both compile clean andllvm-nm --undefined-onlyreports 0 libc++ references for each.discard_tpa_list()was compiled explicitly both with and without-DDEBUG.Not covered
Only
android-arm64was built locally; the other ABIs rely on CI. The FastDev path is Debug-only and was not exercised on a device as part of this change.