Repository navigation
[Java.Interop] Use CStruct and CUnion for JNI layouts - #12984
simonrozsival wants to merge 6 commits into
Conversation
simonrozsival
changed the title
simonrozsival
changed the title
simonrozsival
changed the title
simonrozsival
changed the title
f4e372a to
4a93518
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Foundational JNI ABI layout changes across CoreCLR and NativeAOT need maintainer assessment of validation requirements for other supported ABIs, since reported device and native-header checks cover only ARM64.
No open issues 🎉
What changed in this PR
This PR makes Java.Interop’s native JNI layout contracts explicit using CStruct and CUnion, while preserving field order, types, and existing initialization and lifetime behavior.
Changes:
- Replaces explicit union offsets and sequential struct layouts with extended C layouts.
- Adds field-level
safeannotations and updates the canonical JNI table generator. - Documents extended-layout requirements and corrects the standalone tooling framework reference to .NET 11.
| File | Description |
|---|---|
src/Mono.Android/Microsoft.Android.Runtime/HandleContext.cs |
Uses CStruct for handle context and its control-block mirror. |
src/Mono.Android/Android.Runtime/JNIEnvInit.cs |
Declares CStruct layout for runtime initialization arguments. |
src/Java.Interop/Java.Interop/JniRuntime.cs |
Applies CStruct to the JavaVM table and thread-attach arguments. |
src/Java.Interop/Java.Interop/JniObjectReferenceControlBlock.cs |
Converts the reference-control block to CStruct. |
src/Java.Interop/Java.Interop/JniNativeMethodRegistration.cs |
Declares CStruct layout for native method registration. |
src/Java.Interop/Java.Interop/JniArgumentValue.cs |
Replaces explicit field offsets with CUnion. |
external/Java.Interop/README.md |
Documents layout requirements and the tooling framework. |
build-tools/java-interop-jnienv-gen/Generator.cs |
Generates CStruct JNI tables with safe field annotations. |
Review effort: Balanced
Model Java.Interop.JniArgumentValue as a native C union instead of repeating explicit field offsets. Retain its fields, constructors, equality, and hashing behavior. Raise the shared Java.Interop target to .NET 11 and align Android reference-pack paths, since ExtendedLayout requires the new compiler and runtime support. This is a local PoC for the .NET 12 release. Link the ExtendedLayout and CUnion API documentation. Rely on existing binding and JNI coverage rather than adding declaration-only tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use ExtendedLayoutKind.CStruct for native method registration, JavaVM arguments and function tables, generated JNI tables, runtime startup arguments, and both managed GC-control-block mirrors. Keep instance fields and call sites unchanged. Compare actual product assembly sizes and offsets with native JNI/runtime headers, including array stride and unmanaged callback invocation. Existing binding and SDK smoke tests supply coverage; no repository tests are added. Document the blittable C representation and its distinction from Sequential marshalling, particularly the representation of bool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the memory-safety model enabled by current main when applying CStruct/CUnion. Extended-layout instance fields require an explicit safe or unsafe declaration; mark scalar/address storage safe while retaining native access and callback contracts at their existing sites. Update the canonical JNI generator to emit the same field annotations. Preserve upstream removal of the raw JNI backend and host-JVM harness. Validated JNI library builds with SDK 11 and the pinned SDK 12, 478 binding-generator tests, and native ABI/callback probes. Full product/device validation is paused for shared disk headroom. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The binary-blob remapping loader added NativeBinaryBlobPayload after the original extended-layout PoC. It directly overlays the native BinaryBlobPayload pointer/size descriptor and should use CStruct too. Keep both fields readonly and explicitly safe, preserving the existing initialization contract. Do not annotate span-backed binary readers or managed record views that are not native ABI overlays. Validated the full local SDK build, native-header size/offset/value comparison, and existing CoreCLR/NativeAOT remapping execution tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Condense the overview and build instructions. Replace the detailed layout inventory and release-planning text with a short explanation of CStruct/CUnion, retaining their documentation links and requirements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
009b5fa to
c2b03bf
Compare
Restore the unrelated overview and build wording from main. Keep only the correct .NET 11 target and a short linked extended-layout note. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
ready-to-review
|
@dalexsoto review |
|
In response to @simonrozsival's request: I cannot yet substantiate a complete review of this revision: the read-only review specialist returned a no-issues recommendation but did not retain the required changed-file, managed/native ABI, generator and compiler/framework coverage, or the separate final independent-blocker-pass evidence, even after one handoff-recovery attempt. The nine-file source boundary and immutable source are retained, but a recommendation alone is not being treated as complete coverage or approval. This is a review-evidence handoff limitation, not a reported defect in the PR. I am deferring the formal review until that precise coverage evidence is available; no builds, tests, generators or devices were run. |
|
@dalexsoto review |
|
In response to @simonrozsival's request: This renewed request is acknowledged, but the revision still has the same finite review-evidence gap: the retained specialist result does not substantiate all nine changed files, managed/native ABI consumers, generator and compiler/framework integration, or a separate final independent-blocker pass. One prior handoff-recovery attempt did not recover that evidence. The head is unchanged; the new request itself supplies no missing coverage. I am retaining the precise deferment rather than treating a no-issues recommendation as approval or repeatedly relaunching the unchanged failed investigation. No PR defect is asserted and no build, test, generator or device execution was performed. A formal review requires that missing coverage and current-target integration evidence. |
Context: dotnet/runtime#100896
Context: .NET 11 extended-layout announcement and release notes
Context:
ExtendedLayoutAttribute,ExtendedLayoutKind, and compiler supportJNI's
jvalueis a C union.Java.Interop.JniArgumentValuerepresents it with explicit layout and nine copies of[FieldOffset (0)]. The other native descriptors use sequential or implicit struct layout. These declarations describe field placement without expressing the native layout kind directly.Use
[ExtendedLayout (ExtendedLayoutKind.CUnion)]forJniArgumentValueandCStructfor native method registration, JavaVM/JNI function tables, thread-attach and runtime-initialization arguments, reference-control blocks, and the binary-blob payload descriptor. Update the canonical JNI generator and retain the explicitsafefield declarations required by the existing memory-safety opt-in. Field types/order, constructors, zero initialization, equality/hashing, and native lifetime/call contracts remain unchanged.The binary-blob descriptor was added on
mainafter the original PoC and is included because it directly overlays a native pointer/size pair. Span-backed remapping readers and managed record views are not native overlays and remain unchanged. No build-framework, CI, package-feed, or sample changes are included. This is native ABI layout, not discriminated unions; no performance improvement is claimed. The runtime proposal above is related context, not an issue closed by this PR.Validation
Local validation used macOS arm64, repository SDK
12.0.100-alpha.1.26523.103, and task-owned API 35 ARM64 emulators. Existing tests provide coverage; no new tests are added.make jenkins CONFIGURATION=Release— passed, including SDK preparation, native/managed builds, extra API levels, and local workload setup.make leeroy CONFIGURATION=Releasealso passed after adding the blob descriptor. Toolchain/cache locations used localMSBUILD_ARGSoverrides../dotnet-local.sh test tests/generator-Tests/generator-Tests.csproj -c Debug -v minimal— 478 passed../dotnet-local.sh test bin/TestRelease/net11.0/Xamarin.Android.Build.Tests.dll --filter 'TestCategory=SmokeTests'— 37 passed, 2 existing platform skips.../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Release -p:Device=emulator-5556 -p:RuntimeIdentifier=android-arm64 '-p:IncludeCategories=JniRegistrationSafety%3BPeerControlBlock' --report-trx— 34 passed.../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Release -p:Device=emulator-5556 -p:RuntimeIdentifier=android-arm64 -p:PublishAot=true -p:Optimize=true -p:IncludeCategories=PeerControlBlock --report-trx— 26 passed. The native APK contents and runtime property confirmed NativeAOT execution../dotnet-local.sh test bin/TestRelease/MSBuildDeviceIntegration/net11.0/MSBuildDeviceIntegration.dll --filter 'FullyQualifiedName~R8RuntimeRemappingTests.ObfuscatedMembersRun&Name~False'— both CoreCLR and NativeAOT cases passed, including remapping execution and deleted-output recovery.Device test commands ran from
tests/Mono.Android-Tests/Mono.Android-Tests, followingbuild -t:Installwith matching properties andANDROID_SERIAL=emulator-5556; integration tests also usedADB_TARGET='-s emulator-5556'. Native-header probes matched union bytes and struct sizes/offsets, registration-array stride/callbacks, and native-produced blob descriptor values. Other ABI execution was not run locally.