Uh oh!
There was an error while loading. Please reload this page.
[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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
add6e9c to
6e8a14bComparesimonrozsival
commented
Sep 3, 2026
/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
Uh oh!
There was an error while loading. Please reload this page.
`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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reset FastDev TPA state on every rebuild and keep runtime property allocation failures on the probe-only path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
c968434 to
45e3d4eCompareCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
commented
Sep 5, 2026
@jonathanpeppers please re-approve |
Uh oh!
There was an error while loading. Please reload this page.

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.