Uh oh!
There was an error while loading. Please reload this page.
[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. |
Uh oh!
There was an error while loading. Please reload this page.
ae48813 to
c5ad8c6Compare5bcebb9 to
ecc2418Compareecc2418 to
1f23c98Compare1f23c98 to
a3d78dcComparea3d78dc to
a1bee56Compared202466 to
a0ae195Comparea0ae195 to
5141cd0Compare5141cd0 to
43de481Compare`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 <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
43de481 to
10e7fd7Comparesimonrozsival
commented
Sep 3, 2026
/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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…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.

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.