Skip to content

[native] Replace the bundled properties map with a linked list - #12551

Merged
simonrozsival merged 1 commit into
mainfrom
dev/simonrozsival/clr-bundled-properties
Sep 3, 2026
Merged

simonrozsival merged 1 commit into
mainfrom
dev/simonrozsival/clr-bundled-properties

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Aug 27, 2026 •

Copy link
Copy Markdown
Member

PR relationship: No open PR prerequisite. This is the bottom of stack #12654.

Part of the drop-libc++ work for CoreCLR.

AndroidSystem::bundled_properties was an std::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:

struct BundledProperty
{
	BundledProperty *next;
	char            *name;
	char            *value;
	size_t           value_len;
};

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:

value_len = prop_iter->second.length ();   // the value's length
return prop_iter->first.c_str ();          // ...but the *name*

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.cc with the real build flags plus -DDEBUG:

before after
undefined libc++ symbols 13 12
object size 83,400 77,984 (−5,416)

The symbol that disappears is std::__ndk1::__next_prime() — the bucket-count helper that is unordered_map's only out-of-line dependency.

Testing

  • CoreCLR and MonoVM Release builds are clean.
  • The Debug path was compile-checked with the real build flags plus -DDEBUG, and I confirmed via llvm-nm that the new code is genuinely being compiled (find_bundled_property and add_system_property are present in the object) rather than silently skipped.

Copilot AI lite review requested due to automatic review settings August 27, 2026 21:51

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 Low severity

New issues introduced by this change (1)
Severity Finding
Low severity 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_properties from std::unordered_map<std::string, std::string> to a malloc’d BundledProperty singly 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.

Comment thread src/native/clr/runtime-base/android-system.cc
@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-bundled-properties branch from ae48813 to c5ad8c6 Compare August 28, 2026 06:10
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-bundled-properties branch 2 times, most recently from 5bcebb9 to ecc2418 Compare August 28, 2026 07:54
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-bundled-properties branch from ecc2418 to 1f23c98 Compare August 28, 2026 08:47
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-bundled-properties branch from 1f23c98 to a3d78dc Compare August 28, 2026 08:56
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-bundled-properties branch from a3d78dc to a1bee56 Compare August 28, 2026 09:51
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-bundled-properties branch 2 times, most recently from d202466 to a0ae195 Compare August 28, 2026 12:06
Base automatically changed from dev/simonrozsival/clr-drop-chrono to dev/simonrozsival/clr-timing-free-list August 28, 2026 12:42
@jonathanpeppers
jonathanpeppers force-pushed the dev/simonrozsival/clr-bundled-properties branch from a0ae195 to 5141cd0 Compare August 31, 2026 13:39
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-bundled-properties branch from 5141cd0 to 43de481 Compare September 1, 2026 10:14
`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
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-bundled-properties branch from 43de481 to 10e7fd7 Compare September 3, 2026 06:15
@simonrozsival
simonrozsival changed the base branch from dev/simonrozsival/clr-timing-free-list to main 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 #12551

@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

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

Comment thread src/native/clr/include/runtime-base/android-system.hh
Comment thread src/native/clr/runtime-base/android-system.cc
@simonrozsival
simonrozsival merged commit 404ca22 into main Sep 3, 2026
44 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/clr-bundled-properties branch September 3, 2026 13:11
simonrozsival added a commit that referenced this pull request Sep 4, 2026
…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.
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 4, 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.

3 participants