Repository navigation
[native] Replace the AndroidSystem path statics with a POD buffer - #12552
Conversation
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. |
13c53e9 to
8707bd1
Compare
8707bd1 to
43bdbbb
Compare
43bdbbb to
cc2c3a8
Compare
cc2c3a8 to
6b74b72
Compare
6b74b72 to
f2a7c53
Compare
f2a7c53 to
7ee6aa9
Compare
f9a1d0e to
d4409ce
Compare
56d80d9 to
a752d5a
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
The process-lifetime path storage is otherwise coherent: allocation sizes are overflow-checked, the application-library span is narrowed to initialized entries, and the new C-string consumers preserve the existing lifetime requirements. The remaining issue is that the shared duplication helper passes JNI-derived pointers to strdup() without enforcing its non-null precondition.
CI currently has 42 successful checks with no failures; the final macOS emulator lane and aggregate check are still in progress/queued.
Generated by Android PR Reviewer for #12552 · gpt56 · 203.1 AIC · ⌖ 15.8 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]>
0c3031c to
9a9906b
Compare
…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 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.