From 130d10e5177c2532d259966e5fa7c7589346949e Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 15:25:09 +0200 Subject: [PATCH] [native] Remove the last libc++ dependencies from the CoreCLR host Context: https://github.com/dotnet/android/issues/12533 `assembly-store.cc` was the last file in the CoreCLR host referencing libc++. With it converted the host is at **zero** undefined libc++ references, which means the linker now pulls nothing out of `libc++_static.a` at all. The decompressed-assembly cache accounted for all of it: * `cache_dir` was a `std::string` built by repeated `append`. It is now a single `snprintf` into a stack buffer which the store ID is appended to in place, then `strdup`ed for the lifetime of the process. Both directory levels are still created and validated in turn. * `build_path` returned a `std::string` per call. It becomes `format_cache_path`, formatting into a caller-supplied buffer. * `WriteRequest` held a `std::string path` and a `std::unique_ptr data`. The path is gone entirely - `cache_dir` is immutable once the cache is enabled, so the writer thread rebuilds the path from `descriptor_index` alone - and the payload now lives immediately after the structure, making a request and its bytes a single allocation instead of two. * `std::deque` becomes an intrusive FIFO threaded through `WriteRequest::next`. The queue is only ever pushed at the tail and popped at the head, so a singly linked list is a complete replacement; it also removes the per-request node allocation the deque made on top of the payload allocation. * `tracking` was a `std::unique_ptr` and `assembly_store_names` a `new std::string_view[]`. Both are now `calloc`/`free`. * The two `std::to_string` calls and the `.tmp.` scan in `remove_stale_temp_files` become `snprintf` and `strstr`. Both `snprintf`-formatted paths are bounded by `Util::LocalPathBufferSize` and checked for truncation, where the `std::string` versions grew without limit. A path that long could not be opened anyway, and the failure is logged and disables the cache rather than being silently ignored. Behaviour is otherwise unchanged. Allocation failure still disables the cache instead of taking the process down, except for `assembly_store_names`, which is required for correct assembly lookup and where `new[]` would previously have aborted anyway - exceptions are disabled, so its failure called `std::terminate`. ### Results Undefined libc++ references in the CoreCLR host, Release, arm64: | object | before | after | |---------------------|-------:|------:| | `assembly-store.cc.o` | 12 | **0** | | **host total** | **12** | **0** | | | before | after | delta | |-----------------------------|--------:|--------:|-----------:| | `libnet-android.release.so` | 520,112 | 203,776 | **-316,336 B** | The size drop is far larger than the source change because reaching zero references means the linker stops pulling members out of `libc++_static.a` entirely. ### Verification * CoreCLR, MonoVM and NativeAOT all build clean. * `llvm-nm --undefined-only` over every CoreCLR host object reports 0 libc++ references; the 12 present before this change are gone. * The resulting `.so` still exports `JNI_OnLoad` and the `Java_mono_android_Runtime_*` entry points, and its `DT_NEEDED` list is unchanged. * Relinking with `-nostdlib++` in place of `-static-libstdc++` succeeds, confirming that the host no longer needs libc++ at link time. Actually dropping the flag is left to a follow-up. Only `android-arm64` was built locally; the other ABIs rely on CI. The cache itself is exercised by the existing assembly store tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e --- src/native/clr/host/assembly-store.cc | 255 +++++++++++++++++--------- 1 file changed, 166 insertions(+), 89 deletions(-) diff --git a/src/native/clr/host/assembly-store.cc b/src/native/clr/host/assembly-store.cc index eed53bc53cd..1250558db40 100644 --- a/src/native/clr/host/assembly-store.cc +++ b/src/native/clr/host/assembly-store.cc @@ -1,10 +1,10 @@ #include #include +#include +#include #include #include -#include -#include -#include +#include #include #include @@ -65,13 +65,28 @@ namespace { static_assert (sizeof (CacheFileFooter) == 32uz); - struct WriteRequest final + // A queued cache write. The payload is stored immediately after the structure so that a + // request and the assembly bytes it carries are a single allocation, and `next` links the + // request into the intrusive FIFO drained by the writer thread. The destination path is + // not stored: `cache_dir` is immutable once the cache is enabled, so the writer thread can + // rebuild the path from `descriptor_index` alone. + // + // The over-alignment keeps `sizeof (WriteRequest)` a multiple of the maximum fundamental + // alignment, so that the payload is as aligned as the `malloc` block it lives in. + struct alignas (16) WriteRequest final { - std::string path; - std::unique_ptr data; - size_t size; + WriteRequest *next; + size_t size; + uint32_t descriptor_index; + + auto payload () noexcept -> uint8_t* + { + return reinterpret_cast(this) + sizeof (WriteRequest); + } }; + static_assert (sizeof (WriteRequest) % alignof (std::max_align_t) == 0uz); + enum class WriteResult { Succeeded, @@ -79,9 +94,10 @@ namespace { }; pthread_mutex_t state_lock = PTHREAD_MUTEX_INITIALIZER; - std::deque write_queue; - std::string cache_dir; - std::unique_ptr tracking; + WriteRequest *write_queue_head = nullptr; + WriteRequest *write_queue_tail = nullptr; + char *cache_dir = nullptr; + uint8_t **tracking = nullptr; size_t queued_bytes = 0; uint64_t store_id = 0; bool initialized = false; @@ -89,6 +105,20 @@ namespace { bool writes_enabled = false; bool writer_running = false; + auto allocate_write_request (size_t payload_size) noexcept -> WriteRequest* + { + if (payload_size > SIZE_MAX - sizeof (WriteRequest)) [[unlikely]] { + return nullptr; + } + + auto *request = static_cast(std::malloc (sizeof (WriteRequest) + payload_size)); + if (request != nullptr) { + request->next = nullptr; + } + + return request; + } + auto hash_payload (const uint8_t *data, size_t size) noexcept -> uint64_t { return static_cast(crc32_hash (reinterpret_cast(data), size)); @@ -114,27 +144,43 @@ namespace { return true; } - void log_file_error (std::string_view operation, std::string const& path, int error) noexcept + void log_file_error (const char *operation, const char *path, int error) noexcept { - log_debugf (LOG_ASSEMBLY, "Decompressed-assembly cache %.*s failed for '%s': %s", static_cast(operation.length ()), operation.data (), path.c_str (), std::strerror (error)); + log_debugf (LOG_ASSEMBLY, "Decompressed-assembly cache %s failed for '%s': %s", operation, path, std::strerror (error)); } - auto write_cache_file (WriteRequest const& req) noexcept -> WriteResult + // Formats the cache file path for `descriptor_index` into `buffer`, returning `false` if the + // path does not fit. `cache_dir` is only assigned once, before the cache is enabled, so this + // is safe to call from the writer thread without holding `state_lock`. + auto format_cache_path (char *buffer, size_t buffer_size, uint32_t descriptor_index) noexcept -> bool { - std::string tmp_path = req.path; - tmp_path.append (".tmp."sv); - tmp_path.append (std::to_string (getpid ())); + int length = snprintf (buffer, buffer_size, "%s/%u.bin", cache_dir, descriptor_index); + return length > 0 && static_cast(length) < buffer_size; + } + + auto write_cache_file (WriteRequest *req) noexcept -> WriteResult + { + char path[Util::LocalPathBufferSize]; + if (!format_cache_path (path, sizeof (path), req->descriptor_index)) [[unlikely]] { + return WriteResult::Failed; + } + + char tmp_path[Util::LocalPathBufferSize]; + int tmp_path_length = snprintf (tmp_path, sizeof (tmp_path), "%s.tmp.%d", path, getpid ()); + if (tmp_path_length <= 0 || static_cast(tmp_path_length) >= sizeof (tmp_path)) [[unlikely]] { + return WriteResult::Failed; + } int fd; do { - fd = open (tmp_path.c_str (), O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, 0600); + fd = open (tmp_path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, 0600); } while (fd < 0 && errno == EINTR); if (fd < 0) { - log_file_error ("temporary-file creation"sv, req.path, errno); + log_file_error ("temporary-file creation", path, errno); return WriteResult::Failed; } - bool ok = write_fully (fd, req.data.get (), req.size); + bool ok = write_fully (fd, req->payload (), req->size); int error = ok ? 0 : errno; if (close (fd) != 0 && ok) { ok = false; @@ -142,55 +188,65 @@ namespace { } if (!ok) { - log_file_error ("write"sv, req.path, error); - unlink (tmp_path.c_str ()); + log_file_error ("write", path, error); + unlink (tmp_path); return WriteResult::Failed; } int rename_result; do { - rename_result = rename (tmp_path.c_str (), req.path.c_str ()); + rename_result = rename (tmp_path, path); } while (rename_result != 0 && errno == EINTR); if (rename_result != 0) { error = errno; - log_file_error ("publish"sv, req.path, error); - unlink (tmp_path.c_str ()); + log_file_error ("publish", path, error); + unlink (tmp_path); return WriteResult::Failed; } return WriteResult::Succeeded; } + // Discards every queued request without writing it. Must be called with `state_lock` held. void clear_write_queue_locked () noexcept { - for (WriteRequest const& request : write_queue) { - queued_bytes -= request.size; + WriteRequest *request = write_queue_head; + write_queue_head = nullptr; + write_queue_tail = nullptr; + + while (request != nullptr) { + WriteRequest *next = request->next; + queued_bytes -= request->size; + std::free (request); + request = next; } - write_queue.clear (); } [[gnu::cold]] auto writer_loop ([[maybe_unused]] void *arg) noexcept -> void* { while (true) { - WriteRequest request; + WriteRequest *request; { pthread_mutex_lock (&state_lock); - if (write_queue.empty ()) { + request = write_queue_head; + if (request == nullptr) { writer_running = false; pthread_mutex_unlock (&state_lock); return nullptr; } - request = std::move (write_queue.front ()); - write_queue.pop_front (); + write_queue_head = request->next; + if (write_queue_head == nullptr) { + write_queue_tail = nullptr; + } pthread_mutex_unlock (&state_lock); } - size_t request_size = request.size; + size_t request_size = request->size; WriteResult write_result = write_cache_file (request); - request.data.reset (); + std::free (request); { pthread_mutex_lock (&state_lock); @@ -233,25 +289,25 @@ namespace { return true; } - bool ensure_directory (std::string const& path) noexcept + bool ensure_directory (const char *path) noexcept { - if (mkdir (path.c_str (), 0700) == 0) { + if (mkdir (path, 0700) == 0) { return true; } int error = errno; if (error != EEXIST) { - log_file_error ("directory creation"sv, path, error); + log_file_error ("directory creation", path, error); return false; } struct stat st {}; - if (lstat (path.c_str (), &st) != 0) { - log_file_error ("directory validation"sv, path, errno); + if (lstat (path, &st) != 0) { + log_file_error ("directory validation", path, errno); return false; } if (!S_ISDIR (st.st_mode)) { - log_file_error ("directory validation"sv, path, ENOTDIR); + log_file_error ("directory validation", path, ENOTDIR); return false; } @@ -261,24 +317,23 @@ namespace { // Best-effort removal of staging files left behind by a previous process whose writer was // killed (e.g. by Android) between creating a `.tmp.` file and renaming it into place. // Such files are never reclaimed otherwise and would accumulate outside the queue bound. - void remove_stale_temp_files (std::string const& dir) noexcept + void remove_stale_temp_files (const char *dir) noexcept { - DIR *handle = opendir (dir.c_str ()); + DIR *handle = opendir (dir); if (handle == nullptr) { return; } - std::string prefix = dir; - prefix.append ("/"); for (dirent *entry = readdir (handle); entry != nullptr; entry = readdir (handle)) { - std::string_view name { entry->d_name }; - if (name.find (".tmp."sv) == std::string_view::npos) { + if (strstr (entry->d_name, ".tmp.") == nullptr) { continue; } - std::string path = prefix; - path.append (name); - unlink (path.c_str ()); + char path[Util::LocalPathBufferSize]; + int length = snprintf (path, sizeof (path), "%s/%s", dir, entry->d_name); + if (length > 0 && static_cast(length) < sizeof (path)) { + unlink (path); + } } closedir (handle); @@ -320,33 +375,51 @@ namespace { return; } - cache_dir.assign (code_cache_dir); - cache_dir.append ("/"); - cache_dir.append (CACHE_DIR_NAME); - if (!ensure_directory (cache_dir)) { + // The cache lives at `//`, with both levels + // created in turn. The path is built up in place: the store ID is appended after the + // first directory has been validated. + char path[Util::LocalPathBufferSize]; + int length = snprintf (path, sizeof (path), "%s/%.*s", code_cache_dir, static_cast(CACHE_DIR_NAME.length ()), CACHE_DIR_NAME.data ()); + if (length <= 0 || static_cast(length) >= sizeof (path)) [[unlikely]] { + log_debugf (LOG_ASSEMBLY, "Decompressed-assembly cache path is too long for '%s'", code_cache_dir); + return; + } + + if (!ensure_directory (path)) { return; } store_id = assembly_store_id; - cache_dir.append ("/"); - char store_id_text[(sizeof (store_id) * 2) + 1]; - snprintf (store_id_text, sizeof (store_id_text), "%" PRIx64, store_id); - cache_dir.append (store_id_text); - if (!ensure_directory (cache_dir)) { + int store_id_length = snprintf (path + length, sizeof (path) - static_cast(length), "/%" PRIx64, store_id); + if (store_id_length <= 0 || static_cast(length + store_id_length) >= sizeof (path)) [[unlikely]] { + return; + } + + if (!ensure_directory (path)) { return; } - remove_stale_temp_files (cache_dir); + remove_stale_temp_files (path); - if (compressed_assembly_count > 0) { - tracking.reset (new (std::nothrow) uint8_t*[compressed_assembly_count]()); + if (compressed_assembly_count == 0) { + return; } - enabled = (tracking != nullptr); - if (!enabled) { + // Neither allocation is ever freed: both live for as long as the process does. + cache_dir = strdup (path); + if (cache_dir == nullptr) [[unlikely]] { return; } + tracking = static_cast(std::calloc (compressed_assembly_count, sizeof (uint8_t*))); + if (tracking == nullptr) [[unlikely]] { + std::free (cache_dir); + cache_dir = nullptr; + return; + } + + enabled = true; + { pthread_mutex_lock (&state_lock); writes_enabled = true; @@ -356,29 +429,24 @@ namespace { log_debugf ( LOG_ASSEMBLY, "Enabled decompressed-assembly cache at '%s'; store ID 0x%" PRIx64 "; write queue limit %zu bytes", - cache_dir.c_str (), + cache_dir, store_id, MAX_QUEUED_BYTES ); } - auto build_path (uint32_t descriptor_index) noexcept -> std::string - { - std::string path = cache_dir; - path.append ("/"); - path.append (std::to_string (descriptor_index)); - path.append (".bin"sv); - return path; - } - auto try_load (uint32_t descriptor_index, std::string_view name, uint32_t expected_size) noexcept -> uint8_t* { if (!enabled) { return nullptr; } - std::string path = build_path (descriptor_index); - int fd = open (path.c_str (), O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + char path[Util::LocalPathBufferSize]; + if (!format_cache_path (path, sizeof (path), descriptor_index)) [[unlikely]] { + return nullptr; + } + + int fd = open (path, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); if (fd < 0) { return nullptr; } @@ -467,42 +535,47 @@ namespace { return; } - auto snapshot = std::unique_ptr (new (std::nothrow) uint8_t[total]); - if (snapshot == nullptr) { + WriteRequest *req = allocate_write_request (total); + if (req == nullptr) [[unlikely]] { pthread_mutex_lock (&state_lock); queued_bytes -= total; pthread_mutex_unlock (&state_lock); return; } + + req->size = total; + req->descriptor_index = descriptor_index; + // The runtime can modify the shared decompression buffer after this // method returns, so the background writer needs an immutable copy. - memcpy (snapshot.get (), data, size); + memcpy (req->payload (), data, size); CacheFileFooter footer { .magic = CACHE_FILE_MAGIC, .version = CACHE_FILE_FORMAT_VERSION, .store_id = store_id, - .payload_hash = hash_payload (snapshot.get (), size), + .payload_hash = hash_payload (req->payload (), size), .descriptor_index = descriptor_index, .payload_size = static_cast(size), }; - memcpy (snapshot.get () + size, &footer, sizeof (footer)); - - WriteRequest req { - .path = build_path (descriptor_index), - .data = std::move (snapshot), - .size = total, - }; + memcpy (req->payload () + size, &footer, sizeof (footer)); { pthread_mutex_lock (&state_lock); if (!writes_enabled) { queued_bytes -= total; pthread_mutex_unlock (&state_lock); + std::free (req); return; } - write_queue.push_back (std::move (req)); + if (write_queue_tail == nullptr) { + write_queue_head = req; + } else { + write_queue_tail->next = req; + } + write_queue_tail = req; + if (!writer_running) { writer_running = true; if (!start_writer_locked ()) { @@ -827,12 +900,16 @@ void AssemblyStore::configure_from_payload (const void *payload_start, const cha // Build a lookup of assembly names indexed by descriptor index, used to disambiguate CRC32 hash // collisions during lookup. The names section follows the descriptor table and consists of // `entry_count` length-prefixed (uint32 length followed by the UTF-8 bytes) records, stored in - // descriptor-index order. `delete[]` guards against a leak should the (single) store ever be + // descriptor-index order. The `free` guards against a leak should the (single) store ever be // re-mapped; `assembly_store_names` is nullptr on first call, for which it is a no-op. const uint8_t *names_cursor = assembly_store.data_start + header_size + header->index_size + (static_cast(header->entry_count) * sizeof (AssemblyStoreEntryDescriptor)); - delete[] assembly_store_names; - assembly_store_names = new std::string_view[header->entry_count]; + std::free (assembly_store_names); + assembly_store_names = static_cast(std::calloc (header->entry_count, sizeof (std::string_view))); + if (assembly_store_names == nullptr) [[unlikely]] { + Helpers::abort_application (LOG_ASSEMBLY, "Unable to allocate memory for the assembly store name table"); + } + for (uint32_t i = 0; i < header->entry_count; i++) { uint32_t name_length; memcpy (&name_length, names_cursor, sizeof (name_length));