[native] Replace the AndroidSystem path statics with a POD buffer - #12552
Open
simonrozsival wants to merge 6 commits into
Open
[native] Replace the AndroidSystem path statics with a POD buffer#12552simonrozsival 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 (3)
| Severity | Finding |
|---|---|
src/native/clr/runtime-base/android-system.cc — ❌ error (security): dir_length + 1uz can overflow before malloc, which would lead to a… |
|
src/native/clr/runtime-base/android-system.cc — ❌ error: Allocating app_lib_directories_size * sizeof (const char*) should use overflow-checked… |
|
src/native/clr/include/runtime-base/path-buffer.hh — path_buffer is easy to default-initialize on the stack (e.g. path_buffer<N> p;),… |
What changed in this PR
This PR reduces CoreCLR host libc++ dependencies and startup overhead by removing dynamically-initialized inline static std::string members from AndroidSystem, replacing them with a POD-style path buffer and const char* directory lists to avoid per-TU guard variables and atexit registrations.
Changes:
- Introduces
path_buffer<N>for constant-initialized, mostly stack-buffer path storage with heap fallback. - Converts
AndroidSystempath members and directory lists away fromstd::string/std::array<std::string,...>topath_buffer/const char*, and adjusts DSO path formatting APIs to usestd::string_view. - Updates CoreCLR host components to consume the new
AndroidSystemgetters returningconst char*.
| File | Description |
|---|---|
| src/native/clr/runtime-base/android-system.cc | Switches app/override directory storage and DSO load iteration to const char* + malloced storage; updates DSO path formatting to string_view. |
| src/native/clr/include/runtime-base/path-buffer.hh | Adds path_buffer<N> POD-style path storage abstraction with inline buffer + heap fallback. |
| src/native/clr/include/runtime-base/android-system.hh | Reworks AndroidSystem statics/getters/setters to use path_buffer and const char* directory arrays/spans. |
| src/native/clr/host/host.cc | Updates native library directory usage to const char* from AndroidSystem. |
| src/native/clr/host/fastdev-assemblies.cc | Updates override directory usage/logging to const char* from AndroidSystem. |
| src/native/clr/host/assembly-store.cc | Updates code-cache-dir empty check for const char* API. |
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 06:10
13c53e9 to
8707bd1
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 07:14
8707bd1 to
43bdbbb
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 07:54
43bdbbb to
cc2c3a8
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 08:47
cc2c3a8 to
6b74b72
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 08:56
6b74b72 to
f2a7c53
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 09:52
f2a7c53 to
7ee6aa9
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 10:29
7ee6aa9 to
8c831f8
Compare
`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
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 12:06
f9a1d0e to
d4409ce
Compare
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 (drop the
libc++dependency), stacked on #12551.AndroidSystemkept five of its members instd::string/std::array<std::string, 1>:primary_override_dir,native_libraries_dir,app_code_cache_dir,single_app_lib_directoryandoverride_dirs.Because they are
inline staticwith dynamic initialization, the compiler emits a guard variable and anatexitregistration for them in every translation unit that includesandroid-system.hh— even in ones that never touch them.logger.cc,internal-pinvokes-clr.cc,internal-pinvokes-shared.ccandandroid-system-shared.cceach paid four libc++ references (~basic_string,operator delete,__cxa_guard_acquire,__cxa_guard_release) without using a single one of these directories:What changed
All five become plain pointers. The three path members are
const char*initialized to""and assigned once, early during startup, with a copy made by a newUtil::duplicate_string()helper that aborts if the allocation fails. Pointers to a string literal are constant-initialized, so neither a guard variable nor anatexitregistration is emitted.The two directory arrays become plain
const char*arrays whose entries aremalloced, which also drops anoperator new[]from the non-split-APK path.Since there is no longer a fixed-size buffer anywhere, there is also no hard limit on the path length and no abort when it is exceeded — which is what NativeAOT's
char[SENSIBLE_PATH_MAX]primary_override_dirused to do. That letsprimary_override_dirbe shared by all three hosts, removing three#if defined (XA_HOST_NATIVEAOT)blocks anddetermine_primary_override_dir()entirely.Results
Undefined libc++ references in the three CoreCLR archives — 58 → 31:
assembly-store.cc.ohost.cc.oandroid-system.cc.otiming-internal.cc.ologger.cc.ointernal-pinvokes-shared.cc.ointernal-pinvokes-clr.cc.oandroid-system-shared.cc.otypemap.cc.oEvery
__cxa_guard_*reference coming from this header is gone; the only ones left arehost.cc's own function-local statics.libnet-android.release.so: 539,464 → 536,368 bytes (−3,096).The DEBUG-only code paths were compile-checked separately (there is no Debug ninja directory) and go from 12 to 7 references;
llvm-nmconfirmsadd_system_property,find_bundled_propertyandsetup_environment_from_override_fileare genuinely emitted rather than silently#if'd out.CoreCLR, NativeAOT and MonoVM all build clean.