Skip to content

[CoreCLR] Replace xxhash with crc32#12013

Open
simonrozsival wants to merge 28 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/coreclr-typemap-crc32
Open

[CoreCLR] Replace xxhash with crc32#12013
simonrozsival wants to merge 28 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/coreclr-typemap-crc32

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Jul 7, 2026

Copy link
Copy Markdown
Member

Goal

Remove the CoreCLR native dependency on xxHash/constexpr-xxh3 in the .NET for Android lookup paths while preserving runtime behavior and startup performance. The primary improvement is dependency removal; app size and startup time are unchanged within measurement noise.

Summary

  • Replace CoreCLR LLVM typemap xxHash lookups with 32-bit CRC-32 (System.IO.Hashing.Crc32 in the managed generators, matched by a small runtime-base/crc32.hh in the native runtime).
  • Replace CoreCLR assembly-store name hashes with 32-bit CRC-32 and bump the CoreCLR store format to v4 (MonoVM stays on v3/xxHash).
  • Replace CoreCLR DSO APK/cache hashes with 32-bit CRC-32.
  • Replace the CoreCLR preserved-p/invoke (find_pinvoke) generator hashes with 32-bit CRC-32.
  • Keep MonoVM assembly stores and runtime lookup data on xxHash; move the xxHash-specific Search helpers to MonoVM so CoreCLR no longer pulls in shared/xxhash.hh through the common search header.
  • Remove external/xxHash and constexpr-xxh3 include paths from the CoreCLR native CMake targets (both remain for MonoVM/NativeAOT, which still use xxHash).
  • Remove CoreCLR-only dead paths exposed by the cleanup: AOT DSO cache, debug typemap string-fallback switch, duplicate runtime-property hash index, and the runtime-property host callback.
  • Update generated-layout test parsing and the mk2 assembly-store reader for the new CoreCLR layouts.

Collision safety

CRC-32 is 32-bit, so collisions between two names in the same app — while extremely unlikely — are possible where 64-bit xxHash effectively never collided. Every CoreCLR lookup that matches on these hashes now verifies the actual name after a hash match:

  • Typemap — compares the original managed/Java name (same_string).
  • Assembly storefind_assembly_store_entry walks the run of entries sharing a hash and compares the requested name against the real assembly name from the store's ASSEMBLY_NAMES section.
  • DSO cachefind_dso_cache_entry re-derives the name mutations of the matched entry's real library name (mirroring managed AddNameMutations) and confirms the requested name is one of them.
  • DSO APK table (find_dso_apk_entry) intentionally stays hash-only: DSOApkEntry stores no name, and it's only reached with the real_name_hash of an already-name-verified DSO cache entry, so the residual risk is limited to a CRC-32 collision between two shared libraries in the same APK — a deliberate 64→32-bit trade-off.

The empty-input CRC-32 is a shared UINT32_MAX sentinel between the managed generators and the native runtime, documented in crc32.hh.

Relationship to #12010

Rebased on top of #12010 ([CoreCLR] Remove robin-map from p/invoke override), now in main. #12010 removed the CoreCLR p/invoke override's hand-maintained tables and cache entirely, so the earlier CRC-32 conversion of those tables here is obsolete and was dropped in the merge. CoreCLR p/invoke resolution now falls back through MonodroidDl::monodroid_dlopen, which makes the DSO-cache name verification above the load-bearing path.

Benchmark

CoreCLR MAUI Release/R2R sample, Android arm64, Samsung SM-A165F, interleaved runs, no reboot. These numbers were measured against #12010 as the xxHash baseline (before it merged) to isolate the CRC-32/dependency-removal change; they predate the collision-safety additions, which add only a name comparison on hash matches.

Size

Variant APK size libmonodroid.so uncompressed
#12010 baseline / xxHash 30,693,198 B 1,247,192 B
#12013 current / CRC32 30,709,582 B 1,250,616 B

Startup, 30x mixed cold + hot, IQR outliers removed

Mode #12010 baseline mean / median #12013 current mean / median Delta
Cold WaitTime 2005.0 / 1998 ms 1998.7 / 1995 ms -6.3 ms mean, -3 ms median
Hot WaitTime 148.2 / 146.5 ms 148.1 / 148 ms -0.1 ms mean, +1.5 ms median

Result: app size and startup times are unchanged within measurement noise. The meaningful win is removing CoreCLR's dependency on the xxHash/constexpr-xxh3 native dependency chain.

Validation

  • dotnet build of Xamarin.Android.Build.Tasks, the mk2 assembly-store reader, native-clr (arm64), native-mono (arm64), and the build test project.
  • Clean CoreCLR native arm64 configure/build after removing the xxHash include paths; no external/xxHash or external/constexpr-xxh3 paths remain in the CoreCLR compile output.
  • Full make all CONFIGURATION=Release after merging [CoreCLR] Remove robin-map from p/invoke override #12010.
  • Direct CreateAssemblyStore probe generated a CoreCLR arm64 v4 store with 9-byte index entries that the mk2 reader opened successfully.
  • CoreCLR MAUI benchmark APKs built and launched on Samsung SM-A165F.

simonrozsival and others added 2 commits July 8, 2026 00:02
Replace CoreCLR LLVM typemap xxHash lookups with CRC32 hashes from System.IO.Compression.Native. Keep collision-safe lookup by comparing the original managed or Java type name after matching the hash.

This is a first step toward removing the CoreCLR xxHash dependency to reduce size while preserving runtime lookup performance.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Emit CoreCLR assembly stores with 32-bit CRC32 name hashes and update the CoreCLR native assembly-store lookup to use the shared CRC32 helper.

Keep MonoVM assembly stores on the existing xxHash layout, bump the CoreCLR store format to v4, and update the mk2 store reader to infer index entry width from the header index size.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival simonrozsival changed the title [coreclr] Use CRC32 for LLVM typemap hashes [coreclr] attempt to replace xxhash with crc32 Jul 7, 2026
simonrozsival and others added 2 commits July 8, 2026 00:38
Replace CoreCLR runtime property, DSO APK, and DSO/AOT cache xxHash lookups with CRC32 hashes from the shared native CRC32 helper.

Update generated CoreCLR app config, desktop app stub data, and strict generated-layout test parsing for the smaller 32-bit hash fields.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch the CoreCLR dynamic p/invoke override and generated find_pinvoke tables to 32-bit CRC32 hashes.

Keep the precompiled p/invoke table on the existing xxHash layout while removing xxHash from the shared p/invoke override header and fallback cache path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival simonrozsival changed the title [coreclr] attempt to replace xxhash with crc32 [CoreCLR] Attempt to replace xxhash with crc32 Jul 7, 2026
simonrozsival and others added 10 commits July 8, 2026 01:44
Switch the CoreCLR precompiled p/invoke override table and generator to 32-bit CRC32 hashes.

This removes the remaining CoreCLR p/invoke override dependency on xxHash while keeping the table collision check in the generator.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the common Search helper generic-only and move the xxHash-specific overloads to a MonoVM-only search-xxhash.hh wrapper.

This prevents CoreCLR native code from including shared/xxhash.hh through the common search helper while preserving MonoVM's existing hash_t search callsites.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop adding external/xxHash and constexpr-xxh3 include paths to CoreCLR native targets now that CoreCLR no longer includes shared/xxhash.hh.

Keep the xxHash include support available for MonoVM and NativeAOT paths that still use it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduce a CoreCLR-local hash_t alias to uint32_t in the CRC32 helper and use it for CoreCLR hash fields, lookup signatures, and generated p/invoke tables.

This keeps the code readable without reintroducing the shared xxHash wrapper or changing MonoVM's hash_t layout.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a compile-time crc32_hash overload for string literals and use it in CoreCLR generated/default tables instead of hardcoded hex hash values.

This keeps the generated p/invoke table and app stub self-documenting while preserving the runtime CRC32 helper for dynamic strings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Allow generated-environment tests to parse decimal uint32 values above Int32.MaxValue and update CoreCLR APK size baselines after the CRC32 native size changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the CoreCLR debug typemap string fallback now that CRC32 lookups verify matching strings, and remove the unused CoreCLR AOT DSO cache path while failing fast if a CoreCLR config tries to use it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reuse the runtime property arrays passed to coreclr_initialize for the host runtime contract callback instead of emitting a duplicate CRC32-indexed lookup table.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Leave the host runtime contract in place for p/invoke and assembly probing, but set get_runtime_property to null because Android passes runtime configuration through coreclr_initialize and the callback only produced negative host-property probes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unused CoreCLR application config structure-info fields that were only kept for potential future host configuration data.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

simonrozsival and others added 2 commits July 8, 2026 18:29
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival simonrozsival changed the title [CoreCLR] Attempt to replace xxhash with crc32 [CoreCLR] Replace xxhash with crc32 Jul 9, 2026
@simonrozsival simonrozsival marked this pull request as ready for review July 9, 2026 06:46
Copilot AI review requested due to automatic review settings July 9, 2026 06:46
@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jul 9, 2026

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.

Pull request overview

This PR removes CoreCLR’s native dependency on xxHash/constexpr-xxh3 by switching multiple CoreCLR lookup paths (typemaps, assembly store indices, DSO cache, and p/invoke override) to 32-bit CRC32, while keeping MonoVM on the existing xxHash-based layouts. It also bumps the CoreCLR assembly-store format to v4 and updates associated tooling/tests.

Changes:

  • Replace CoreCLR xxHash lookups with CRC32 across typemap, assembly store, DSO cache, and p/invoke override paths.
  • Update CoreCLR assembly-store format to v4 (including variable index entry sizes) and adjust readers/parsers accordingly.
  • Isolate xxHash-specific search helpers to MonoVM and remove CoreCLR’s xxHash include paths/dependencies.
Show a summary per file
File Description
tools/assembly-store-reader-mk2/AssemblyStore/StoreReader_V2.cs Accept CoreCLR v4 store versions and support variable index entry sizes when reading.
tools/assembly-store-reader-mk2/AssemblyStore/StoreReader_V2.Classes.cs Add explicit 32/64-bit index entry size constants for parsing.
src/Xamarin.Android.Build.Tasks/Utilities/TypeMappingReleaseNativeAssemblyGeneratorCLR.cs Emit CoreCLR typemap data using CRC32 + collision-safe secondary string comparisons.
src/Xamarin.Android.Build.Tasks/Utilities/TypeMappingDebugNativeAssemblyGeneratorCLR.cs Update debug typemap layout to CRC32 + module UUID array and simplify hash usage.
src/Xamarin.Android.Build.Tasks/Utilities/TypeMapHelper.cs Introduce CRC32 helpers for CoreCLR hashing (names/bytes).
src/Xamarin.Android.Build.Tasks/Utilities/PreservePinvokesNativeAssemblyGenerator.cs Switch preserved p/invoke hashing to CRC32 and simplify 32/64-bit handling.
src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreGenerator.cs Generate CoreCLR v4 assembly store indices using CRC32 (32-bit) and adjust index parsing.
src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreBuilder.cs Pass target runtime to store generator to select CRC32 vs xxHash.
src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGeneratorCLR.cs Convert CoreCLR DSO cache hashes to CRC32 and remove CoreCLR-only dead AOT/runtime-property paths.
src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigCLR.cs Reflect CoreCLR ApplicationConfig layout change (remove AOT cache entry count).
src/Xamarin.Android.Build.Tasks/Tasks/GenerateEmptyTypemapStub.cs Update stub LLVM IR struct layouts for CoreCLR typemap changes.
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsDotNet.CoreCLR.apkdesc Update expected APK description sizes after native/layout changes.
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.CoreCLR.apkdesc Update expected APK description sizes after native/layout changes.
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs Update layout parsing/constants for CoreCLR config + CRC32-sized fields.
src/native/mono/runtime-base/search-xxhash.hh New: MonoVM-only xxHash search wrapper to keep CoreCLR free of xxHash includes.
src/native/mono/runtime-base/monodroid-dl.hh Move MonoVM search usage to SearchXxHash wrapper.
src/native/mono/monodroid/mono-image-loader.hh Use SearchXxHash for MonoVM image cache lookups.
src/native/mono/monodroid/embedded-assemblies.cc Use SearchXxHash for MonoVM assembly store + typemap lookups.
src/native/common/include/runtime-base/search.hh Refactor search helpers: introduce lower_bound and rework binary_search templates.
src/native/CMakeLists.txt Remove CoreCLR include paths for external xxHash/constexpr-xxh3.
src/native/clr/xamarin-app-stub/application_dso_stub.cc Update CoreCLR stub to CRC32 and remove dead runtime-property/AOT-cache paths.
src/native/clr/shared/CMakeLists.txt Remove EXTERNAL include path from CoreCLR shared target options.
src/native/clr/pinvoke-override/precompiled.cc Switch precompiled p/invoke override hashing to CRC32 and adjust slow-path call.
src/native/clr/pinvoke-override/pinvoke-tables.include Regenerate internal p/invoke tables to use CRC32 hashes and constexpr crc32_hash calls.
src/native/clr/pinvoke-override/generate-pinvoke-tables.cc Update generator to emit CRC32-based tables and update constexpr validation.
src/native/clr/pinvoke-override/dynamic.cc Switch dynamic p/invoke override hashing to CRC32 and adjust slow-path call.
src/native/clr/include/xamarin-app.hh Bump CoreCLR store format to v4 and update public struct layouts for CRC32-based fields.
src/native/clr/include/runtime-base/monodroid-dl.hh Remove CoreCLR AOT cache path and switch DSO lookups to CRC32.
src/native/clr/include/runtime-base/crc32.hh New: constexpr CRC32 implementation for CoreCLR native hashing (compile-time + runtime).
src/native/clr/include/host/typemap.hh Update typemap lookup APIs to support collision-safe matching (hash + string).
src/native/clr/include/host/pinvoke-override.hh Switch map hashing to CRC32 and simplify APIs (remove explicit hash parameters).
src/native/clr/include/host/pinvoke-override-impl.hh Update override implementation to match new map APIs and loader calls.
src/native/clr/include/host/host.hh Remove unused runtime-property host callback wiring for CoreCLR.
src/native/clr/host/typemap.cc Implement CRC32 hashing and collision-safe typemap matching via lower_bound + string compare.
src/native/clr/host/host.cc Switch CoreCLR DSO APK hashing to CRC32 and remove unused runtime-property callback implementation.
src/native/clr/host/CMakeLists.txt Remove EXTERNAL include path from CoreCLR host target options.
src/native/clr/host/assembly-store.cc Switch CoreCLR assembly-store name hashing to CRC32.

Copilot's findings

Comments suppressed due to low confidence (1)

src/Xamarin.Android.Build.Tasks/Tasks/GenerateEmptyTypemapStub.cs:75

  • typemap_use_hashes was removed from the CoreCLR typemap implementation, but the debug stub LLVM IR still declares it. Keeping stale stub symbols makes the stub diverge from the real layout and can hide future regressions in layout-validation tests.
@type_map = dso_local constant %struct.TypeMap zeroinitializer, align 8
@typemap_use_hashes = dso_local constant i8 1, align 1
@type_map_managed_type_info = dso_local constant [0 x %struct.TypeMapManagedTypeInfo] zeroinitializer, align 8
  • Files reviewed: 37/37 changed files
  • Comments generated: 4

Comment thread tools/assembly-store-reader-mk2/AssemblyStore/StoreReader_V2.cs
Comment thread src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreGenerator.cs
Comment thread src/Xamarin.Android.Build.Tasks/Utilities/TypeMapHelper.cs Outdated
simonrozsival and others added 2 commits July 9, 2026 23:24
- Avoid per-call byte[] allocation in HashNameForCLR via stackalloc
- Replace unused switch expression with explicit arch guard
- Validate index_size divisibility in GetIndexEntrySize (both readers)
- Remove stale typemap_use_hashes symbol from debug typemap stub

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 13027389-e819-4a27-b98f-dbcd7586a1ab
…eclr-typemap-crc32

# Conflicts:
#	src/native/clr/pinvoke-override/dynamic.cc
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Android PR Reviewer completed successfully!

@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.

Review: [CoreCLR] Replace xxhash with crc32

Overall this is a clean, well-scoped refactor. I independently verified the core correctness invariant and the collision-safety story, and found one issue — in a test helper.

🔎 Verified correct

  • C#/native CRC32 agreement (non-empty): System.IO.Hashing.Crc32.HashToUInt32 (build) matches the hand-rolled crc32_hash (runtime) — both are CRC-32/ISO-HDLC (poly 0xEDB88320). Confirmed numerically: "test"0xd87f7e0c, "java-interop"0x33b98009, matching the native static_asserts.
  • Empty-input handling is consistent: HashNameForCLR/HashBytesForCLR and native crc32_hash both special-case empty → 0xFFFFFFFF, so they agree (they intentionally diverge from raw CRC32's 0).
  • Collision safety with 32-bit hashes:
    • typemap lookups now do lower_bound + linear scan with an actual string compare (same_string) after the hash match — safe against CRC32 collisions.
    • pinvoke table generation hard-fails the build on collision (generate-pinvoke-tables.cc returns 1), so the pure-hash find_pinvoke_address is safe.
    • assembly store / DSO cache keep pure-hash binary search, but 32-bit hashing already applied to 32-bit ABIs before this PR and the entry counts are small — consistent with the documented trade-off.
  • Struct layout: AssemblyStoreIndexEntry is [[gnu::packed]], so the 32-bit hash_t gives a 9-byte on-disk entry that matches the mk2 reader's NativeSize32 and the generator's packed writes.
  • Assembly store v4 + mk2 reader: reader derives entry size from index_size / index_entry_count (with a divisibility guard) and validates it against NativeSize32/64 — correctly handles the new CoreCLR 32-bit-on-64-bit case.
  • Removal of dead CoreCLR paths (AOT DSO cache, get_runtime_property, debug typemap string fallback) looks self-consistent, with a guard added if a libaot- DSO unexpectedly appears in CoreCLR config.

⚠️ Issue (see inline comment)

  • EnvironmentHelper.TryParseInteger (test helper): the Int32/Int64 fallback uses NumberStyles.None, which rejects a leading -. That makes the signed fallback dead code and drops negative decimals that llc can emit — a regression from the prior NumberStyles.Integer behavior, and it contradicts the method's own updated comment. Fix: use NumberStyles.AllowLeadingSign.

CI

At review time: Java.Interop tests (Windows/Mac) and CLA are green; the Linux/Mac/Windows build+test legs are still in progress with no failures yet.

Automated review — surfacing observations only, not a blocking gate.

Generated by Android PR Reviewer for #12013 · 412.7 AIC · ⌖ 23.7 AIC · ⊞ 6.8K
Comment /review to run again

Generated uint32_t and uint64_t values are emitted using unsigned decimal representations, so remove the unnecessary signed parsing fallback.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 11570f57-4494-4f92-977b-47f37db2e81b
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Android PR Reviewer completed successfully!

@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.

🤖 Code Review — [CoreCLR] Replace xxhash with crc32

Overall this is a clean, internally-consistent change. CI is green across the sampled Azure DevOps checks. The migration from xxHash/constexpr-xxh3 to CRC32 (reusing System.IO.Compression.Native) is well executed, the search.hh refactor to lower_bound primitives is tidy, and removing the AOT DSO cache / runtime-property host callback / CI_TYPEMAP_DEBUG_USE_STRINGS dead paths is a nice cleanup. The store format bump (v3→v4) and the matching EnvironmentHelper test updates (field count 20→19, DSOCacheEntry size 32→24, unsigned parsing) line up correctly with the struct changes.

Strongest positive: the typemap lookups were made genuinely collision-safe — after matching the 32-bit hash you now compare the actual strings via the new same_string() helper, and the C# generators sort by hash then StringComparer.Ordinal so grouped collisions stay consistent. Native crc32_hash (reflected poly 0xedb88320, init/final 0xffffffff) matches System.IO.Hashing.Crc32, and pinvoke-tables.include now emits crc32_hash("...") consteval calls so build/runtime hashes can't drift.

Main thing to consider (see inline): the same collision-safety was not applied to the assembly-store index, the DSO cache, or the DSO apk lookups — those still match on 32-bit hash only, with no runtime name verification and no build-time duplicate-hash detection. Since CoreCLR was previously 64-bit-only here, this is a real (if small) reduction in collision resistance for those tables. Internal p/invoke tables are fine — generate-pinvoke-tables.cc already fails the build on collisions. Worth either adding a cheap build-time collision check for the store index or documenting the intentional trade-off.

Minor: a one-line comment on the crc32_hash empty-string → UINT32_MAX sentinel (mirrored in TypeMapHelper) would protect against a future well-meaning "fix" to 0.

Nothing here looks like a blocker — the inline notes are a safety consideration and a doc nit.

Generated by Android PR Reviewer for #12013 · 259.6 AIC · ⌖ 19 AIC · ⊞ 6.8K
Comment /review to run again

Comment thread src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreGenerator.cs
Comment thread src/native/clr/include/runtime-base/crc32.hh
Comment thread src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreGenerator.cs
Comment thread src/Xamarin.Android.Build.Tasks/Utilities/AssemblyStoreGenerator.cs
simonrozsival and others added 6 commits July 13, 2026 16:52
CoreCLR assembly stores now key their name index with a 32-bit CRC32
instead of the previous 64-bit xxHash, so a hash collision between two
assembly names - while extremely unlikely - is possible. The store
lookup previously matched on hash equality alone and would silently
resolve such a collision to the wrong assembly, with no build-time
signal.

Walk the whole run of index entries sharing a hash and compare the
requested name against the actual assembly name (recovered from the
store's ASSEMBLY_NAMES section) before accepting an entry, mirroring the
collision-safe approach already used by the CoreCLR typemap lookups. The
names are exposed through a lookup array indexed by descriptor index,
built once when the store is mapped.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a09ac6c0-bbe5-4abe-bce5-55cc631256a4
The DSO cache is also keyed by 32-bit CRC32 hashes, and
find_dso_cache_entry previously matched on hash equality alone. The
cache stores one entry per name mutation but keeps only the hash of each
mutation - every entry points at the library's real name - so a
collision is disambiguated by re-deriving the mutations of a matched
entry's real name and confirming the requested name is one of them.
name_is_mutation_of mirrors the managed AddNameMutations and must be
kept in sync with it (a note was added on both sides).

find_dso_apk_entry is intentionally left matching on the CRC32 hash
alone: a DSOApkEntry stores no name to verify against, and the lookup is
only reached with the real_name_hash of a DSOCacheEntry that has already
been name-verified, so the residual risk is limited to a CRC32 collision
between two shared libraries packaged in the same APK. This is a
deliberate 64->32-bit hash-width trade-off.

Also document the empty-input CRC32 sentinel (UINT32_MAX) that is shared
with the managed TypeMapHelper hashes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a09ac6c0-bbe5-4abe-bce5-55cc631256a4
…eclr-typemap-crc32

# Conflicts:
#	src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.CoreCLR.apkdesc
#	src/native/clr/host/CMakeLists.txt
#	src/native/clr/include/host/pinvoke-override-impl.hh
#	src/native/clr/include/host/pinvoke-override.hh
#	src/native/clr/pinvoke-override/generate-pinvoke-tables.cc
#	src/native/clr/pinvoke-override/pinvoke-tables.include
#	src/native/clr/pinvoke-override/precompiled.cc
The NativeAOT native target listed ${CONSTEXPR_XXH3_DIR} in its include
path, but no NativeAOT translation unit includes constexpr-xxh3.h - the
CoreCLR (clr/) headers it shares are xxHash-free after this PR, and the
nativeaot/ sources never used it. Remove the now-unused include path.

CONSTEXPR_XXH3_DIR and its MonoVM consumer (mono/shared) remain, since
MonoVM still uses xxHash via shared/xxhash.hh.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a09ac6c0-bbe5-4abe-bce5-55cc631256a4
The BuildReleaseArm64SimpleDotNet.CoreCLR.apkdesc sizes for the CoreCLR
native libraries this PR changes (libassembly-store.so, libmonodroid.so,
libxamarin-app.so) and the package size were set from an additive
estimate during the origin/main merge, because a local Release build
couldn't be run to regenerate them. The estimate for libmonodroid.so was
~5.07% low, which tripped the 5% per-file threshold in BuildReleaseArm64.

Replace the estimates with the sizes measured by CI:
- libassembly-store.so: 2731744 -> 2725520
- libmonodroid.so:      1174656 -> 1237416
- libxamarin-app.so:      18376 -> 17896
- PackageSize:          7202235 -> 7230907

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a09ac6c0-bbe5-4abe-bce5-55cc631256a4
@simonrozsival simonrozsival removed the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jul 14, 2026
Resolve the dlopen-based assembly store conflicts by removing the obsolete CoreCLR APK DSO hash table while retaining CRC32 for assembly-store and runtime lookups.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: f6fda5dd-cc30-4aa3-8666-d11212ff9cac
Read have_assembly_store from field 19 after the obsolete AOT cache field was removed from the CoreCLR application configuration.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: f6fda5dd-cc30-4aa3-8666-d11212ff9cac
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Android PR Reviewer completed successfully!

@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.

🤖 Code Review — ⚠️ Minor suggestions (no blockers)

Thorough, well-documented change. I reviewed the CRC32 conversion end-to-end and the collision-safety design holds up.

✅ Verified correct

  • Managed ↔ native hash agreement. TypeMapHelper.HashNameForCLR/HashBytesForCLR (System.IO.Hashing CRC-32, poly 0xEDB88320, empty→UInt32.MaxValue) matches the native crc32_hash in crc32.hh bit-for-bit, including the shared UINT32_MAX empty-input sentinel — and the sentinel is documented on both sides.
  • Collision verification paths all compare real names after a hash match:
    • Typemap (find_index_by_hash, find_managed_to_java_map_entry, find_java_to_managed_entry) now walks the run of equal hashes and confirms via same_string, using new managed_type_name_length/java_name_length fields. Those lengths are UTF-8 byte counts (LlvmIrStringBlob.AddUtf8StringToBytes), consistent with the native strlen-based comparison. ✔
    • find_assembly_store_entry walks the hash run and validates against the new assembly_store_names table; name_matches correctly handles the with/without-extension index duplication.
    • find_dso_cache_entryname_is_mutation_of faithfully re-derives AddNameMutations (.dll.so double-strip, .so stem variant, recursive lib prefix stripping, OrdinalIgnoreCase). I traced several names through both sides and they agree.
  • Sort ordering parity. Both managed comparators (JavaNameHashComparer, module-entry sort, debug SequenceCompareTo on module_uuid) now tie-break by name, matching the native lower_bound run-walk. byte unsigned ordering ↔ memcmp is consistent.
  • Store format v4 bump is CoreCLR-only; MonoVM stays on v3/xxHash. Search::lower_bound is correctly introduced and binary_search is re-expressed on top of it. GetIndexEntrySize derives entry size from the header rather than a bitness flag (needed now that CoreCLR is always 32-bit hashes) and guards divisibility. Removed dead paths (AOT DSO cache, typemap_use_hashes, runtime-property index) are cleanly excised with the C# structs kept in sync.

💡 Suggestions (both posted inline, non-blocking)

  1. configure_from_payload names-section parser advances by an untrusted name_length with no bounds check — cheap to harden.
  2. binary_search_branchless relocated into the new search-xxhash.hh is dead (only a commented-out caller) and still carries commented-out lines.

Notes

  • find_dso_apk_entry intentionally staying hash-only is a reasonable, clearly-documented 64→32-bit trade-off.
  • CI shows no completed required checks yet on this commit (status pending) — not evaluated here; ensure the full Azure DevOps pipeline is green before merge.

Nice work on the parity documentation between the generators and the runtime — the cross-referencing comments make this reviewable.

Generated by Android PR Reviewer for #12013 · 188.8 AIC · ⌖ 13.4 AIC · ⊞ 6.8K
Comment /review to run again

Comment thread src/native/clr/host/assembly-store.cc
Comment thread src/native/mono/runtime-base/search-xxhash.hh Outdated
Remove dead bitness plumbing, redundant CoreCLR DSO hash data, the retired string-typemap test mode, and the unused branchless xxHash search.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 33bd77c7-b6de-4c97-a662-4825faa3068a
@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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