Skip to content

[native] Replace the AndroidSystem path statics with a POD buffer - #12552

Merged
simonrozsival merged 9 commits into
mainfrom
dev/simonrozsival/clr-android-system-paths
Sep 4, 2026
Merged

simonrozsival merged 9 commits into
mainfrom
dev/simonrozsival/clr-android-system-paths

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Aug 27, 2026 •

Copy link
Copy Markdown
Member

PR relationship: Depends on #12551. This is the middle PR in stack #12654. The absolute symbol and size totals below were measured in the former serialized stack and include changes outside this smaller stack.

Part of #12533 (drop the libc++ dependency), stacked on #12551.

AndroidSystem kept five of its members in std::string / std::array<std::string, 1>: 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:

$ llvm-nm --undefined-only logger.cc.o | llvm-cxxfilt
std::__ndk1::basic_string<...>::~basic_string()
operator delete(void*)
__cxa_guard_acquire
__cxa_guard_release

$ llvm-objdump -r logger.cc.o | grep _ZGV | llvm-cxxfilt
guard variable for xamarin::android::AndroidSystem::override_dirs
guard variable for xamarin::android::AndroidSystem::app_code_cache_dir
guard variable for xamarin::android::AndroidSystem::native_libraries_dir
guard variable for xamarin::android::AndroidSystem::primary_override_dir
guard variable for xamarin::android::AndroidSystem::single_app_lib_directory

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 new Util::duplicate_string() helper that aborts if the allocation fails. Pointers to a string literal are constant-initialized, so neither a guard variable nor an atexit registration is emitted.

The two directory arrays become plain const char* arrays whose entries are malloced, which also drops an operator 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_dir used to do. That lets primary_override_dir be shared by all three hosts, removing three #if defined (XA_HOST_NATIVEAOT) blocks and determine_primary_override_dir() entirely.

Results

Undefined libc++ references in the three CoreCLR archives — 58 → 31:

object before after
assembly-store.cc.o 13 11
host.cc.o 11 11
android-system.cc.o 11 5
timing-internal.cc.o 5 2
logger.cc.o 4 0
internal-pinvokes-shared.cc.o 4 0
internal-pinvokes-clr.cc.o 4 0
android-system-shared.cc.o 4 0
typemap.cc.o 2 2

Every __cxa_guard_* reference coming from this header is gone; the only ones left are host.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-nm confirms add_system_property, find_bundled_property and setup_environment_from_override_file are genuinely emitted rather than silently #if'd out.

CoreCLR, NativeAOT and MonoVM all build clean.

Copilot AI lite review requested due to automatic review settings August 27, 2026 22:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: 1 High severity · 2 Medium severity

New issues introduced by this change (3)
Severity Finding
High severity src/​native/​clr/​runtime-base/​android-system.cc — ❌ error (security): dir_length + 1uz can overflow before malloc, which would lead to a…
Medium severity src/​native/​clr/​runtime-base/​android-system.cc — ❌ error: Allocating app_lib_directories_size * sizeof (const char*) should use overflow-checked…
Medium severity src/​native/​clr/​include/​runtime-base/​path-buffer.hh — ⚠️ warning: path_buffer is easy to default-initialize on the stack (e.g. path_buffer&lt;N&gt; 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 AndroidSystem path members and directory lists away from std::string/std::array<std::string,...> to path_buffer / const char*, and adjusts DSO path formatting APIs to use std::string_view.
  • Updates CoreCLR host components to consume the new AndroidSystem getters returning const 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.

Comment thread src/native/clr/runtime-base/android-system.cc
Comment thread src/native/clr/runtime-base/android-system.cc
Comment thread src/native/clr/include/runtime-base/path-buffer.hh Outdated
@simonrozsival simonrozsival added the drop-libcpp Work to remove the libc++ dependency from Android NativeAOT label Aug 28, 2026
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 13c53e9 to 8707bd1 Compare August 28, 2026 06:10
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 8707bd1 to 43bdbbb Compare August 28, 2026 07:14
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 43bdbbb to cc2c3a8 Compare August 28, 2026 07:54
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from cc2c3a8 to 6b74b72 Compare August 28, 2026 08:47
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 6b74b72 to f2a7c53 Compare August 28, 2026 08:56
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from f2a7c53 to 7ee6aa9 Compare August 28, 2026 09:52
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch 2 times, most recently from f9a1d0e to d4409ce Compare August 28, 2026 12:06
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 56d80d9 to a752d5a Compare September 3, 2026 06:15
@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Sep 3, 2026
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12552

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Needs Changes — 0 errors, 1 warning.

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

Comment thread src/native/clr/include/runtime-base/util.hh
Base automatically changed from dev/simonrozsival/clr-bundled-properties to main September 3, 2026 13:11
simonrozsival and others added 9 commits September 3, 2026 15:11
`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
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-android-system-paths branch from 0c3031c to 9a9906b Compare September 3, 2026 13:11
@simonrozsival
simonrozsival merged commit 73f4925 into main Sep 4, 2026
44 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/clr-android-system-paths branch September 4, 2026 22:19
simonrozsival added a commit that referenced this pull request Sep 8, 2026
…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.
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

drop-libcpp Work to remove the libc++ dependency from Android NativeAOT ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants