[TrimmableTypeMap] Add exception handling to UCO constructor callbacks (nctor_*_uco)#11178
Conversation
Wrap the activation code in EmitUcoConstructor with BeginMarshalMethod/ EndMarshalMethod + try/catch/finally to prevent exceptions from crossing the JNI boundary (SIGABRT). Mirrors the ManagedPeer.Construct pattern. - PEAssemblyBuilder: New EmitBody overload for exception regions via ControlFlowBuilder - TypeMapAssemblyEmitter: New type/member refs, EmitUcoConstructorBodyWithMarshal helper, updated EmitUcoConstructor to use exception handling - TypeMapAssemblyGeneratorTests: 4 new tests verifying exception regions Agent-Logs-Url: https://github.com/dotnet/android/sessions/c97aa44d-9497-4c2e-b1d1-1b8c382d1789 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
Pull request overview
This PR updates the TrimmableTypeMap generator so that generated UCO constructor callbacks (nctor_*_uco) wrap activation in the standard JniEnvironment.BeginMarshalMethod / EndMarshalMethod try/catch/finally pattern, preventing managed exceptions from crossing the JNI boundary and causing aborts.
Changes:
- Added a
PEAssemblyBuilder.EmitBodyoverload that passes aControlFlowBuilderto allow emitting IL and registering exception regions in one pass. - Updated
TypeMapAssemblyEmitter.EmitUcoConstructorto emit the marshal wrapper + catch/finally regions for both standard and JavaInterop activation styles. - Added new tests verifying exception regions are present for non-generic UCO constructors and absent for open-generic no-op constructors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Adds tests asserting exception regions (catch+finally) for generated nctor_*_uco methods across activation styles and inheritance cases. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs | Emits marshal-method wrapper (Begin/End + catch/finally) around UCO constructor activation and adds required type/member references and local signatures. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PEAssemblyBuilder.cs | Introduces a new EmitBody overload that exposes ControlFlowBuilder so callers can register exception regions while emitting IL. |
There was a problem hiding this comment.
✅ LGTM
Solid, well-structured change that fixes a real crash (SIGABRT from unhandled exceptions crossing the JNI boundary in UCO constructor callbacks). The implementation correctly mirrors the ManagedPeer.Construct marshal-method pattern.
What I verified:
- IL correctness: The exception region encoding follows ECMA-335 correctly — the finally region's try range
[tryStart, finallyStart)properly encompasses both the try body and catch handler, soleavefrom either will trigger the finally. - Early-return path:
BeginMarshalMethodreturning false branches toafterAllbeforetryStart, so no exception regions are entered —EndMarshalMethodis correctly skipped when no transition was started. - Local variable layouts: Standard (3 locals) and JavaInterop (4 locals) signatures encode the correct types (valuetype for
JniTransition/JniObjectReference, class forJniRuntime/Exception). Local indices in IL match the declared layouts. - Null-safety in catch: The
runtimenull-check beforecallvirt OnUserUnhandledExceptioncorrectly handles the case whereBeginMarshalMethodreturns true butruntimeis null. - Generic no-op: Open-generic UCO constructors are correctly left as simple
retstubs without exception handling. - Tests: 4 tests covering all activation paths (standard leaf, JavaInterop, inherited ctor, generic no-op).
- CI: Public CI checks are green.
By severity:
| Count | |
|---|---|
| 💡 Suggestion | 2 |
Two minor suggestions (missing invariant comment on the new EmitBody overload, and test helper extraction for repeated search pattern) — neither blocks merge.
Generated by Android PR Reviewer for issue #11178 · ● 4M
This comment was marked as outdated.
This comment was marked as outdated.
…invariant comment - Extract FindNctorUcoMethod helper into FixtureTestBase to eliminate duplication across 4 tests - Add ILContainsCallToken helper and verify nctor_*_uco IL actually calls BeginMarshalMethod/EndMarshalMethod/OnUserUnhandledException - Add sigBlobHandle capture ordering comment to new EmitBody overload - Keep generic no-op test conditional (GenericHolder doesn't emit nctor_*_uco) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
CI failure unrelated: |
UCO constructor callbacks (
nctor_0_uco,nctor_1_uco, …) emitted byEmitUcoConstructorhad no exception handling. An exception thrown by the activation constructor would cross the JNI boundary unhandled, causing a SIGABRT. This mirrors the same pattern already used byManagedPeer.Construct.Changes
PEAssemblyBuilderEmitBody(…, Action<InstructionEncoder, ControlFlowBuilder>, …)overload — lets callers both emit IL and register exception regions (catch/finally) in a single pass.TypeMapAssemblyEmitterJniTransition,JniRuntime,System.ExceptionJniEnvironment.BeginMarshalMethod,JniEnvironment.EndMarshalMethod,JniRuntime.OnUserUnhandledExceptionEmitUcoConstructorBodyWithMarshalhelper emits the standard marshal wrapper pattern:EmitUcoConstructorupdated for both standard ((IntPtr, JniHandleOwnership)) and JavaInterop ((ref JniObjectReference, JniObjectReferenceOptions)) activation styles. Open-generic (no-op) UCO constructors are unchanged.EncodeUcoConstructorLocals_Standard,_JavaInterop) for the expanded local layouts (JniTransition/JniRuntime/Exception + optional JniObjectReference).Tests (
TypeMapAssemblyGeneratorTests)