[native] Replace the AndroidSystem path statics with a POD buffer - #12552
Open
simonrozsival wants to merge 74 commits into
Open
[native] Replace the AndroidSystem path statics with a POD buffer#12552simonrozsival wants to merge 74 commits into
simonrozsival wants to merge 74 commits into
Conversation
Use fixed buffers for straightforward type-name, override-path, and system-property values, formatting composed strings with snprintf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the remaining logger, max-gref, and timing property consumers to explicit fixed buffers so the CLR dynamic-local-string property overload can be removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The logger interface no longer exposes local-string types, so keep the temporary include local to its remaining fallback-path implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Allocate managed type and timing strings to their exact sizes instead of treating the former local-string stack threshold as a maximum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep exact-size type and timing strings independent of libc++ ownership. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the existing NativeAOT fixed-storage limit while preserving unbounded CoreCLR path construction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use malloc only when typemap or override names exceed their sensible local buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve stack storage for typical managed type and override paths while allocating the exact required capacity for larger values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Route managed type and override path heap-buffer cleanup through Util. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rely on free(nullptr) and name stack-backed CLR string storage explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Eliminate separate heap pointers and free generated CLR strings only when they differ from their stack buffers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Narrowing the `strings.hh` include in `logger.hh` also removed two symbols that headers were picking up transitively through it: * `strings.hh` included `shared/helpers.hh`, which is where `os-bridge.hh` was getting `abort_unless` from. * `strings.hh` included `<unistd.h>`, which is where `bridge-processing.cc` was getting `gettid()` from. Include both explicitly at their point of use. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Narrowing the strings.hh include in logger.hh removed the transitive path that util.cc relied on for dynamic_local_string, breaking the CoreCLR and NativeAOT builds. Include the header where it is used. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Addresses review feedback: the fixed-buffer overload returned -1 when a bundled (build-time) property value did not fit into the caller's buffer, so long values were treated as if the property were not set at all. The `dynamic_local_string` based overload it replaced grew onto the heap and had no such limit. Bundled properties come from `@(AndroidEnvironment)` files and are stored as NUL-terminated strings in static application data, so they are neither subject to Android's 92 byte property limit nor in need of copying. Return a `std::string_view` instead of an `int`: for Android system properties it views the caller's scratch buffer, for bundled properties it points directly at the application data, which restores the previous behaviour and avoids a copy. `FastTiming::parse_options()` used to tokenize its argument in place, which is not safe for a view over static data, so it now parses without mutating. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Contributor
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. |
simonrozsival
added
the
drop-libcpp
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 06:10
13c53e9 to
8707bd1
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 07:14
8707bd1 to
43bdbbb
Compare
…perty
The previous commit changed `monodroid_get_system_property ()` to return a
`std::string_view` so that bundled properties, whose length is not limited by
`PROPERTY_VALUE_BUFFER_LEN`, could be returned without copying them into the
caller's scratch buffer.
That works, but `std::string_view` deliberately makes no promise about
NUL-termination, while every value this function can return happens to be
NUL-terminated: `__system_property_get ()` terminates what it writes, and
bundled properties are NUL-terminated strings in static application data. The
header had to document that invariant in a comment ("The returned value is
always NUL-terminated") precisely because the type denies it, and callers such
as `get_max_gref_count_from_system ()` silently relied on it by passing
`.data ()` to `strtol ()` and to a `%s` format specifier.
Return `const char*` instead (and `nullptr` when the property is not set). The
lifetime rule is unchanged and still uniform - the result is valid for at least
as long as the caller's buffer - but NUL-termination is now guaranteed by the
type rather than by a comment, so `.data ()` no longer has to be laundered
through a `std::string_view`. Callers that need to tokenize the value construct
a `std::string_view` explicitly, which is honest about what they are doing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 07:54
43bdbbb to
cc2c3a8
Compare
- `format_managed_type_name ()` builds the name with a single `snprintf ()` instead of three `memcpy ()` calls and hand-rolled length arithmetic. The negative-required-capacity retry contract is unchanged, and the helper is now guarded by `#if defined (DEBUG)` like its only caller, which removes the unused-function warning this pull request introduced. - `FastTiming::parse_options ()` takes a `const char*` again and tokenizes with `strchr ()`/`strncmp ()`/`strtoull ()`. It had been rewritten around `std::string_view`, which added a C++ layer to code that was already plain C. The parser still cannot NUL-terminate in place - the value may point at immortal bundled property data - so each parameter is bounded by its length instead. The `duration=` and `filename=` edge cases behave as they did before. - The property lookup chain (`monodroid_get_system_property ()`, `monodroid__system_property_get ()` and `lookup_system_property ()`) takes `const char *name`, matching the other overloads. Previously it took a `std::string_view` and immediately called `.data ()` on it, which is the same NUL-termination laundering that motivated changing the return type. This also lets `HostEnvironment::lookup_system_property ()` use `strcmp ()` directly and drops `<string_view>` from `android-system-shared.cc` entirely. - Shorten the comments added by this pull request, and drop the `strings.hh` include from `logger.cc`, which no longer uses `dynamic_local_string`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 08:47
cc2c3a8 to
6b74b72
Compare
Reuse a single pthread mutex wrapper instead of adding a second one. `mono/shared/cppcompat.hh` already contained `xamarin::android::mutex` and `xamarin::android::lock_guard`, added for exactly the same reason: `<mutex>` makes the runtime depend on libc++. Rather than maintain two wrappers with the same purpose, delete `cppcompat.hh` and move the MonoVM host over to the shared `Mutex`/`MutexGuard` in `common/include/runtime-base/mutex.hh`. `Mutex` is the stricter of the two: it deletes the copy and move operations, which the old `mutex` left implicitly defined even though copying a `pthread_mutex_t` is never correct. It is also explicitly `constexpr` default constructible, so static instances stay constant-initialized. Also refresh the two comments explaining why `NDEBUG` is defined before including `robin_map.h`. They claimed `<mutex>` "conflicts with our std::mutex definition in cppcompat.hh", which stopped being true once the wrapper moved into the `xamarin::android` namespace. The hack is still worth keeping, but the real reason is that `<iostream>` and `<mutex>` would both pull in libc++. Finally, value-initialize `dso_handle_write_lock` for consistency with the other static `Mutex` instances. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Drop the RAII guard in favour of calling pthread_mutex_lock/unlock through Mutex directly. Critical sections that used to return, break or continue while holding the lock now delegate to a `_locked` helper that holds the branching logic, so each locked region has exactly one entry and one exit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Drop the Mutex class and use pthread_mutex_t with pthread_mutex_lock/unlock at the call sites. PTHREAD_MUTEX_INITIALIZER keeps the static instances constant-initialized, so they still need no thread-safe initialization guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Keep cppcompat.hh and the MonoVM hosts's use of it as they were. The CoreCLR host no longer shares a mutex wrapper with MonoVM, so there is no reason for this change to reach into src/native/mono. Timing lives in the shared common sources, so it is still converted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The previous commit split every locked region that contained an early exit into an outer method (which locks) and an inner `_locked` method (which holds the logic). That kept the lock/unlock pairing obvious, but it introduced six new methods and a helper enum purely to work around `return` statements. Restructure the locked regions in place instead: use a result flag plus `break`/`if`-`else` so control always falls through to the unlock, and move the early `return` after it. This drops all six helpers along with the `ReserveResult` enum and keeps the diff against the original code much smaller. * `writer_loop` uses `have_request` / `write_failed` * `enqueue_write` restores the original `queue_full` bool and adds `writes_allowed` * `get_available_sequence` uses `ret == nullptr` + `break` * `open_assembly` inverts the `opendir` check and re-tests `override_dir_fd` after unlocking No behavioural change. libc++ references are unchanged at 40 (CoreCLR) and 0 (NativeAOT), and `__cxa_guard_*` stays at 8, confirming the statics are still constant-initialized. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The previous two commits reshaped the locked regions to avoid returning while the lock was held: first by splitting them into `_locked` helpers, then by threading result flags so control fell through to the unlock. Both worked, but both restructured code that did not otherwise need to change. Just call `pthread_mutex_unlock` immediately before the early `return` instead. The regions keep their original shape, so the diff against the pre-existing code is now purely mechanical -- a type change, a `std::lock_guard` turning into a `pthread_mutex_lock`, and an added unlock. Churn against the base drops from 145 changed lines to 62. This is safe because the native runtime is built with `-fno-exceptions` (verified in `compile_commands.json`), so there is no unwind path that `std::lock_guard` would have covered and a manual unlock would miss. Verified that every `return` inside a locked region is immediately preceded by an unlock of that mutex, across all 13 regions. libc++ references remain 40 (CoreCLR) and 0 (NativeAOT), and `__cxa_guard_*` stays at 8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`Timing::sequence_pool` was a `std::vector<managed_timing_sequence>` that `get_available_sequence` scanned for a free entry, growing it with `emplace_back` when every entry was in use. Returning pointers into a vector's buffer is unsound. The constructor does `resize (16)`, which leaves capacity at exactly 16, so the seventeenth concurrent sequence reallocates the buffer -- and every pointer already handed out to managed code (held as an `IntPtr` across the `TimingLogger.Start`/`Stop` window) is left dangling. `monodroid_timing_stop` then writes `sequence->end` and `in_use = false` into freed memory, and the measurement is silently lost. Because those entries are never marked free again, the pool also grows on every subsequent call. Replace the vector with an intrusive free list. Entries are allocated individually with `malloc`, so they never move, and `release_sequence` pushes them back onto the list instead of freeing them. Nothing is ever freed, so no pointer can dangle; the total allocation is bounded by the peak number of concurrent sequences. Acquire and release are now O(1) rather than an O(n) scan under the lock. `in_use` is kept purely as a guard: a double release would otherwise push an entry onto the list twice and hand it to two callers at once. Today a double release is harmless, and it stays harmless. `Timing` is left with two constant-initialized POD members, so it no longer needs a constructor and can be a plain `static inline` instance in BSS, removing the `new Timing ()` as well. This only pays off on top of the `pthread_mutex_t` change: while `sequence_lock` was a `std::mutex` its non-trivial destructor forced `__cxa_atexit` registration behind a guard variable, which cost two more symbols than the `operator new` it saved. Real libc++ references in the CoreCLR archive drop from 40 to 38 (one `operator new`, one `__libcpp_verbose_abort` from the vector's length check). `__cxa_guard_*` stays at 8 and NativeAOT stays at 0. MonoVM also uses this class and gets the same fix without any change to `src/native/mono/`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`Host::_timing` was a pointer whose only job was to encode "fast timing is disabled" as `nullptr`. `FastTiming::enabled ()` already answers that question, so the pointer was redundant indirection over a static instance that always exists. Keep just the object, have `get_timing ()` return a reference, and gate both P/Invokes on `FastTiming::enabled ()`. This also closes a window where `enabled ()` was true but the pointer had not been assigned yet. Also null-check `get_available_sequence ()` in `monodroid_timing_start ()`: it can now return `nullptr` when `malloc` fails, which the previous vector-backed implementation never did. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The free list gave every sequence its own `malloc`, and threaded the recycling through a `next_free` pointer inside the sequence itself. That works, but a double release would put an entry on the list twice and hand it to two callers at once, so `release_sequence ()` had to guard against it. Allocate in chunks of 16 instead and go back to recycling through `in_use`, the way the original vector-backed code did. `get_available_sequence ()` scans the chunks for an unused entry and chains on a new chunk when it finds none. Chunks are never freed, so every address handed to managed code stays valid for the lifetime of the process, and a double release is just a redundant store. MonoVM shares `Timing` and dereferenced `get_available_sequence ()` without checking it, which was safe while the pool was a vector but is not now that allocation can fail. Add the missing check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Returning `nullptr` on allocation failure pushed the problem onto every caller, and both `monodroid_timing_start ()` implementations had to grow a check they never needed while the pool was a `std::vector`. Abort instead, which is what the rest of the runtime does when it cannot allocate. `get_available_sequence ()` can no longer fail, so both checks go away again and `src/native/mono/` is untouched by this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The previous commits replaced the `std::vector` backing `Timing`'s sequence pool with chunks allocated by `calloc` and chained together. `FastTiming`'s `TimingEventChunk` is a structurally identical pool that was left using `new`/`delete`, so apply the same treatment to it. This does not change the `libc++` reference count on its own, because the same translation units still reference `operator new`/`operator delete` for the `std::string` that `TimingEvent::more_info` points to. Removing those strings is done in the next commit of the stack, and only then does the count actually drop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`FastTiming::open_sequences` was a `thread_local std::stack<TimingEvent*>`, which defaults to `std::deque` as its container. `std::deque` has both a non-trivial constructor and a non-trivial destructor, so every translation unit including `timing-internal.hh` emitted a guarded dynamic initializer plus a `__cxa_thread_atexit` registration for the thread-local instance. The stack only ever needs `push`, `top`, `pop` and `empty`, and its depth is bounded by how deeply the instrumented calls nest (currently 3) because every `start_event` is matched by exactly one `end_event` or `store_more_info`. Replace it with a fixed `TimingEvent*` array plus a depth counter, both of which are trivially constructible and destructible and therefore constant initialized. `open_sequences` is `thread_local`, so it is private to each thread and needs no locking - that remains true here, as no state is shared between threads. The depth counter is incremented even when the array is full, so a push past the bound only loses that one entry instead of misaligning the pairing of the events below it. Once the depth drops back within bounds the remaining entries are still correct. Removes all 4 `__cxa_thread_atexit` references and one `__libcpp_verbose_abort`, taking the CoreCLR host's libc++ references from 64 to 59. As a side effect, pushing a timing event no longer allocates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The fixed array capped the nesting depth of timing events, which is not a
limit the timing code should impose - any number of events may be open on a
thread at once. Replace it with a naive singly linked list used as a stack,
with one malloc'd node per open sequence:
struct OpenSequence
{
TimingEvent *event;
OpenSequence *next;
};
static inline thread_local OpenSequence *open_sequences = nullptr;
The head pointer is still a trivially destructible thread-local, so this keeps
the property that motivated the change: no guarded dynamic initializer and no
`__cxa_thread_atexit` registration.
Nodes are freed as they are popped rather than being recycled, so a thread
that balances its `start_event` and `end_event` calls leaves nothing behind
when it exits. That matters here because, unlike the process-wide timing
sequence pool, this list is per thread and threads come and go.
Allocation failure aborts, matching how the timing sequence chunks behave.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`FastTiming` kept two heap-allocated `std::string`s that the earlier pass over the timing code missed: the per-event `TimingEvent::more_info` and the output file name parsed out of the `debug.mono.timing` property. `more_info` becomes a plain NUL-terminated `char*`. It was always built from one or two `std::string_view`s whose total length is known up front, so a single `malloc` and one or two `memcpy`s replace the string entirely. When the allocation fails we simply drop the extra information instead of aborting - timing is a diagnostic facility and must not take the application down with it. The output file name comes from a system property, whose value is limited to `PROP_VALUE_MAX` (92) bytes, so it now lives in a fixed 128 byte buffer inside `FastTiming` rather than in a `std::unique_ptr<std::string>`. Keeping it inline also means the global `internal_timing` instance stays constant-initialized and needs no guard variable. Names that do not fit are rejected with a warning and the default is used. Together with the previous commit this removes the last `operator new` and `operator delete` references from `timing-internal.cc.o` and, as a side effect, all of them from `typemap.cc.o`, which had been inheriting them from the inlined `new TimingEventChunk` in `FastTiming::get_event`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`std::function` is a type-erasing wrapper which needs to store, copy and
destroy an arbitrary callable, and it pulls `<functional>` into every
translation unit that sees the declaration. Neither of the two uses in the
CoreCLR host needs any of that.
`FastTiming::dump` took its line writer as `std::function<void(std::string_view const&)>`
by value. Of its two callers one passes a captureless lambda and the other
captures a single `FILE*`, so a plain function pointer plus an opaque
`void *context` covers both:
using LineWriter = void (*) (void *context, std::string_view const& line);
`AssemblyStore::configure_from_payload` took a `const std::function<std::string()>&`
used only to produce a path for diagnostics. Its only caller wrapped a
`const char *` in a `std::string` just so that the callee could call
`c_str ()` on it again, and the callback is invoked unconditionally in the
success path, so this allocated a string on every startup. It now takes the
`const char *` directly.
This does not change the number of undefined libc++ references, since both
uses were fully inlined by the optimizer, but it removes the generated
machinery: `libnet-android.release.so` shrinks by 6,976 bytes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Both `dump` callers either write to a file or ignore the context entirely, so there is no need for the context to be `void*`. Typing it as `FILE*` removes the `static_cast` in the file line writer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The two line writers were captureless lambdas converted to function pointers at the call site. That conversion goes through a compiler generated static invoker, so making them plain functions in an anonymous namespace removes a level of indirection: `libnet-android.release.so` shrinks by a further 56 bytes. The remaining lambdas inside `dump` are called directly rather than converted to function pointers, so the optimizer already inlines them completely - replacing those measured 2 bytes *larger*, so they are left alone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Addresses review feedback: `configure_from_payload()` takes a raw `const char*` and every use of it goes through `optional_string ()`, so the header comment now says explicitly that passing `nullptr` is allowed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`FastTiming::get_time()` already read the clock with `clock_gettime()`; `std::chrono::steady_clock` was only used as the type tag of the `chrono::time_point` the result was wrapped in. Store the timestamps as a plain `uint64_t` nanosecond count instead and drop `<chrono>` from the four files that included it (it was entirely unused in mainthread-dso-loader.hh). All four places that formatted an interval repeated the same seconds/milliseconds/nanoseconds split, so they now share a `time_interval` helper. The split is reproduced exactly as `chrono::duration_cast` computed it, so the timing output is unchanged - this matters because the format after the first colon is parsed by our performance measuring utilities. Also read `CLOCK_MONOTONIC` rather than `CLOCK_MONOTONIC_RAW`, so that we keep using the same clock `steady_clock` was documented to use. The two differ only in that `CLOCK_MONOTONIC` is slewed by NTP, which is irrelevant at the granularity we measure. This does not remove any undefined libc++ symbols - `<chrono>` is header only - but it does shrink libnet-android.release.so by 80 bytes and removes one more libc++ header from the build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
…tals Addresses review feedback. Both fields are totals for the whole interval and both are printed, so `milliseconds` is not milliseconds-within-the-second. The output format is consumed by performance measuring utilities, so spell this out to keep a future change from "correcting" it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`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
`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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> 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 <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
simonrozsival
force-pushed
the
dev/simonrozsival/clr-android-system-paths
branch
from
August 28, 2026 12:06
f9a1d0e to
d4409ce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.