Repository navigation
[native] Replace the bundled properties map with a linked list - #12551
Conversation
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/native/clr/runtime-base/android-system.cc — 💡 suggestion (performance/alloc churn) — add_system_property() always allocates a new value… |
What changed in this PR
This PR updates the CoreCLR host’s debug-only bundled system property storage to remove the last std::unordered_map usage (and its <unordered_map> dependency), aligning the implementation with MonoVM’s long-standing BundledProperty singly linked list approach. It also corrects a pre-existing debug-only lookup bug where the property name was returned instead of the value, which could cause buffer over-reads.
Changes:
- Replace
AndroidSystem::bundled_propertiesfromstd::unordered_map<std::string, std::string>to a malloc’dBundledPropertysingly linked list (DEBUG-only). - Add
AndroidSystem::find_bundled_property()helper for linear lookup in the linked list. - Fix debug-only property lookup to return the correct value pointer and length.
| File | Description |
|---|---|
| src/native/clr/runtime-base/android-system.cc | Implements linked-list storage and correct lookup of bundled properties in DEBUG builds. |
| src/native/clr/include/runtime-base/android-system.hh | Removes <unordered_map> and introduces the DEBUG-only BundledProperty definition + storage pointer. |
ae48813 to
c5ad8c6
Compare
5bcebb9 to
ecc2418
Compare
ecc2418 to
1f23c98
Compare
1f23c98 to
a3d78dc
Compare
a3d78dc to
a1bee56
Compare
d202466 to
a0ae195
Compare
a0ae195 to
5141cd0
Compare
5141cd0 to
43de481
Compare
`AndroidSystem::bundled_properties` was an
`std::unordered_map<std::string, std::string>`, which is the only user of
`<unordered_map>` in the CoreCLR host. The properties are read from the
environment override files at run time, so the set is not known at build time
and cannot be a static sorted array - but the map buys us nothing either: the
entries are added once at startup, looked up a handful of times and there are
only a few of them.
Use the same malloc'd singly linked list MonoVM has always used for this
(`BundledProperty`), with the name allocated together with the node and the
value allocated separately so that setting a property twice can replace it.
This also fixes a real bug. The lookup returned the map key rather than the
value:
value_len = prop_iter->second.length ();
return prop_iter->first.c_str ();
so every bundled property resolved to its own *name*, reported with the
*value's* length - which over-reads past the end of the name whenever the value
is longer than the name.
Release builds are unaffected, this code is `#if defined (DEBUG)` only. In a
Debug build of android-system.cc it removes the last reference to
`std::__next_prime()` (12 undefined libc++ symbols instead of 13) and shrinks
the object file from 83,400 to 77,984 bytes.
Co-authored-by: Copilot <[email protected]>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
43de481 to
10e7fd7
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 1 warning · 1 suggestion
The linked-list implementation correctly mirrors the established MonoVM ownership model and fixes the Debug-only lookup bug by returning the stored value with its matching length. All 44 reported checks passed. Before merging, please add regression coverage for the pointer/length bug; the stale map reference in the API documentation should also be corrected.
Generated by Android PR Reviewer for #12551 · gpt56 · 85.5 AIC · ⌖ 14.1 AIC · ⊞ 25.7K
Comment /review to run again
…2552) 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 `malloc`ed, 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.
…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 the drop-libc++ work for CoreCLR.
AndroidSystem::bundled_propertieswas anstd::unordered_map<std::string, std::string>and the only user of<unordered_map>in the CoreCLR host.A sorted static array + binary search isn't an option here: bundled properties are read from the environment override files at run time, so the set isn't known at build time. But the map isn't buying us anything either — the entries are added once during startup, looked up a handful of times, and there are only a few of them.
So this uses the same malloc'd singly linked list that MonoVM has always used for exactly this purpose (
BundledProperty), keeping the two runtimes consistent:The name is allocated together with the node (one allocation covers both); the value is allocated separately so that setting the same property twice can replace it. Allocation failure aborts, as elsewhere in this stack.
This also fixes a real bug
The lookup returned the map key instead of the value:
So in Debug builds every bundled property resolved to its own name, reported with the value's length. When the value is longer than the name that's an over-read past the end of the name's buffer. This is pre-existing on
main, and falls out of the rewrite for free.Results
This code is
#if defined (DEBUG)only, so Release builds are completely unaffected — I verified the undefined-libc++-symbol list per object file is byte-for-byte identical before and after (58 both ways).The win is in Debug builds. Compiling
android-system.ccwith the real build flags plus-DDEBUG:The symbol that disappears is
std::__ndk1::__next_prime()— the bucket-count helper that isunordered_map's only out-of-line dependency.Testing
-DDEBUG, and I confirmed viallvm-nmthat the new code is genuinely being compiled (find_bundled_propertyandadd_system_propertyare present in the object) rather than silently skipped.