From 01a7fe190add09b41236c30535ffceafdc366cb7 Mon Sep 17 00:00:00 2001 From: David Nguyen <87228593+davidnguyen-tech@users.noreply.github.com> Date: Tue, 17 Mar 2026 13:17:30 +0100 Subject: [PATCH 01/10] Add ObjectiveCMarshal.ThrowPendingExceptionObject helper Add a CoreLib helper that retrieves and throws any pending Objective-C exception. This will be called from R2R-precompiled P/Invoke IL stubs after objc_msgSend calls, mirroring the existing NativeAOT approach. Uses ExceptionDispatchInfo.Throw to preserve the original stack trace and is marked [StackTraceHidden] to keep the throw site clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../InteropServices/ObjectiveCMarshal.CoreCLR.cs | 12 ++++++++++++ src/coreclr/vm/corelib.h | 1 + 2 files changed, 13 insertions(+) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs index 101978434bd612..bb27891a8c6627 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs @@ -1,7 +1,9 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; using System.Runtime.Versioning; namespace System.Runtime.InteropServices.ObjectiveC @@ -39,6 +41,16 @@ private static partial IntPtr CreateReferenceTrackingHandleInternal( out int memInSizeT, out IntPtr mem); + [StackTraceHidden] + internal static void ThrowPendingExceptionObject() + { + Exception? ex = System.StubHelpers.StubHelpers.GetPendingExceptionObject(); + if (ex is not null) + { + ExceptionDispatchInfo.Throw(ex); + } + } + [UnmanagedCallersOnly] internal static unsafe void* InvokeUnhandledExceptionPropagation(Exception* pExceptionArg, IntPtr methodDesc, IntPtr* pContext, Exception* pException) { diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 04042375bb9496..fdae9778316191 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -451,6 +451,7 @@ DEFINE_FIELD_U(_buckets, GCHandleSetObject, _buckets) #ifdef FEATURE_OBJCMARSHAL DEFINE_CLASS(OBJCMARSHAL, ObjectiveC, ObjectiveCMarshal) +DEFINE_METHOD(OBJCMARSHAL, THROW_PENDING_EXCEPTION_OBJECT, ThrowPendingExceptionObject, SM_RetVoid) DEFINE_METHOD(OBJCMARSHAL, INVOKEUNHANDLEDEXCEPTIONPROPAGATION, InvokeUnhandledExceptionPropagation, SM_PtrException_IntPtr_PtrIntPtr_PtrException_RetVoidPtr) #endif // FEATURE_OBJCMARSHAL From b41aa09deb6abb5617b2c3261bfbc86985f9fff1 Mon Sep 17 00:00:00 2001 From: David Nguyen <87228593+davidnguyen-tech@users.noreply.github.com> Date: Tue, 17 Mar 2026 13:17:54 +0100 Subject: [PATCH 02/10] Emit ThrowPendingExceptionObject in R2R P/Invoke IL stubs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of throwing NotSupportedException for ObjC P/Invokes (which blocked R2R precompilation entirely), emit a call to ObjectiveCMarshal.ThrowPendingExceptionObject() after the native call in the IL stub. This mirrors the NativeAOT PInvokeILEmitter. The generated stub IL for a blittable objc_msgSend now looks like: marshal args → call objc_msgSend → call ThrowPendingExceptionObject → ret On iOS (no JIT), this eliminates 82 interpreter fallbacks at startup for a dotnet new maui app. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../IL/Stubs/PInvokeILEmitter.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs index ed05deb5f23581..319e39dda7b3d5 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs @@ -56,6 +56,14 @@ private void EmitPInvokeCall(PInvokeILCodeStreams ilCodeStreams) callsiteSetupCodeStream.Emit(ILOpcode.call, emitter.NewToken(rawTargetMethod)); + if (MarshalHelpers.ShouldCheckForPendingException(context.Target, _importMetadata)) + { + MetadataType objcMarshalType = context.SystemModule.GetKnownType( + "System.Runtime.InteropServices.ObjectiveC"u8, "ObjectiveCMarshal"u8); + callsiteSetupCodeStream.Emit(ILOpcode.call, emitter.NewToken( + objcMarshalType.GetKnownMethod("ThrowPendingExceptionObject"u8, null))); + } + static PInvokeTargetNativeMethod AllocateTargetNativeMethod(MethodDesc targetMethod, MethodSignature nativeSigArg) { var contextMethods = s_contexts.GetOrCreateValue(targetMethod.Context); @@ -72,9 +80,6 @@ private MethodIL EmitIL() if (!_importMetadata.Flags.PreserveSig) throw new NotSupportedException(); - if (MarshalHelpers.ShouldCheckForPendingException(_targetMethod.Context.Target, _importMetadata)) - throw new NotSupportedException(); - if (_targetMethod.IsUnmanagedCallersOnly) throw new NotSupportedException(); From e04bc858067e606f62b4ceec0b42e1e5941e0948 Mon Sep 17 00:00:00 2001 From: David Nguyen <87228593+davidnguyen-tech@users.noreply.github.com> Date: Tue, 17 Mar 2026 13:18:22 +0100 Subject: [PATCH 03/10] Move ObjC pending exception check to PInvokeILStubMethodIL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove ShouldCheckForPendingException from Marshaller.IsMarshallingRequired — ObjC pending exceptions are a platform concern, not a marshalling concern. return false for all ObjC P/Invokes, blocking R2R stub generation entirely. Add the check to PInvokeILStubMethodIL.IsMarshallingRequired instead. This property is read by pInvokeMarshalingRequired (the JIT-EE interface) to decide whether the JIT should use the precompiled stub or inline a raw native call. Without this check, the JIT would inline blittable ObjC P/Invokes with GTF_CALL_UNMANAGED — a raw call with no pending exception handling — silently skipping the ThrowPendingExceptionObject call that the stub contains. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs | 4 +++- .../ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs index 319e39dda7b3d5..58d71b9cc7d128 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs @@ -147,7 +147,9 @@ public sealed class PInvokeILStubMethodIL : ILStubMethodIL public PInvokeILStubMethodIL(ILStubMethodIL methodIL) : base(methodIL) { - IsMarshallingRequired = Marshaller.IsMarshallingRequired(methodIL.OwningMethod); + MethodDesc method = methodIL.OwningMethod; + IsMarshallingRequired = Marshaller.IsMarshallingRequired(method) + || MarshalHelpers.ShouldCheckForPendingException(method.Context.Target, method.GetPInvokeMethodMetadata()); } } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs index d827aed4673c78..186ee7fba2a3ba 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs @@ -117,9 +117,6 @@ public static bool IsMarshallingRequired(MethodDesc targetMethod) if (!flags.PreserveSig) return true; - if (MarshalHelpers.ShouldCheckForPendingException(targetMethod.Context.Target, metadata)) - return true; - var marshallers = GetMarshallersForMethod(targetMethod); for (int i = 0; i < marshallers.Length; i++) { From d289c6cd295b5b6386477611a8c9b23ae76519b3 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 11 Mar 2026 14:08:12 -0500 Subject: [PATCH 04/10] Add review assessment and implementation plan for PR #124770 Analyzes reviewer feedback from BrzVlad, AaronRobinsonMSFT, and jkoritzinsky. Includes recommended code changes and test coverage plan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pr-124770-plan.md | 169 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 pr-124770-plan.md diff --git a/pr-124770-plan.md b/pr-124770-plan.md new file mode 100644 index 00000000000000..fc34ca1b36e15d --- /dev/null +++ b/pr-124770-plan.md @@ -0,0 +1,169 @@ +# PR #124770 — Final Review Assessment + +## Summary + +Analysis of 3 reviewer concerns on PR #124770 ("Enable R2R precompilation of objc_msgSend P/Invoke stubs"), validated against the actual code. + +### Historical Context + +The `ShouldCheckForPendingException` check was originally added to `IsMarshallingRequired` by Aaron Robinson in commit 4a782d58ac4 (PR #52849, May 2021). At that time, R2R had **no ability** to emit the pending exception check at all. The check served as a two-layer safety net: +1. `IsMarshallingRequired` returns true → `GeneratesPInvoke` returns false → R2R won't try +2. Even if it did try, `EmitIL()` threw `NotSupportedException` → caught in CorInfoImpl + +Now that R2R **can** emit the check (via this PR's `ThrowPendingExceptionObject`), the safety net is no longer needed for ObjC P/Invokes — but the original design explains why it was there. + +--- + +## Concern 1: Logic Error in `GeneratesPInvoke` (BrzVlad — inline review) + +### BrzVlad's Claim + +> "This check here looks wrong. This check is already done as part of `Marshaller.IsMarshallingRequired` and I believe it should be removed from there. In your case, this method is returning true for pinvokes with non-blittable types that require check for pending exception. It should be returning false instead and I believe this is the reason you are forcefully catching the requires jit exception above." + +### Verdict: ✅ BrzVlad is correct — the root cause is a conflation of concerns + +**The code evidence:** + +`Marshaller.IsMarshallingRequired` (Marshaller.ReadyToRun.cs:104-131) contains: +```csharp +// line 120-121: +if (MarshalHelpers.ShouldCheckForPendingException(targetMethod.Context.Target, metadata)) + return true; +// line 123-128: then checks each marshaller for type conversion needs +``` + +This means `IsMarshallingRequired` returns `true` for ANY ObjC P/Invoke **before even checking parameter types**. It conflates "needs ObjC exception check" with "needs type marshalling." + +`GeneratesPInvoke` (ReadyToRunCompilationModuleGroupBase.cs:712-726): +```csharp +if (!Marshaller.IsMarshallingRequired(method)) // line 719 + return true; // line 720 — blittable non-ObjC +if (MarshalHelpers.ShouldCheckForPendingException(...)) // line 722 + return true; // line 723 — ANY ObjC +return false; // line 725 +``` + +**Traced scenarios:** + +| Scenario | `IsMarshallingRequired` | `GeneratesPInvoke` | Actually compilable? | +|----------|------------------------|--------------------|--------------------| +| Blittable non-ObjC (`Sleep(int)`) | false | true (line 720) | ✅ Yes | +| Blittable ObjC (`objc_msgSend(IntPtr, IntPtr)`) | true (line 120) | true (line 723) | ✅ Yes | +| **Non-blittable ObjC** (`objc_msgSend(IntPtr, IntPtr, string)`) | true (line 120) | **true (line 723)** | ❌ **No!** | + +For the non-blittable ObjC case, `GeneratesPInvoke` promises "yes, we'll compile this," but then `PInvokeILEmitter.EmitIL()` hits `NotSupportedMarshaller` for the `string` parameter → throws `NotSupportedException` → converted to `RequiresRuntimeJitException` (line 133-135) → caught by the try-catch in `ReadyToRunCodegenCompilation.cs` (line 228-233). + +**The try-catch IS a workaround** for `GeneratesPInvoke` returning an incorrect `true`. + +### BrzVlad's suggested fix (remove from both places) + +BrzVlad is saying remove `ShouldCheckForPendingException` from **both**: +1. **`IsMarshallingRequired`** (Marshaller.ReadyToRun.cs line 120-121) — "needs ObjC exception check" ≠ "needs marshalling" +2. **`GeneratesPInvoke` line 722-723** (the PR's addition) — "this check here looks wrong" + +With both removed, `GeneratesPInvoke` simplifies back to just `!IsMarshallingRequired`: + +| Scenario | New `IsMarshallingRequired` | New `GeneratesPInvoke` | Correct? | +|----------|---------------------------|----------------------|----------| +| Blittable non-ObjC (`Sleep(int)`) | false | true (line 720) | ✅ | +| Blittable ObjC (`objc_msgSend(IntPtr, IntPtr)`) | **false** (all params blittable) | true (line 720) | ✅ | +| Non-blittable ObjC (`objc_msgSend(IntPtr, string)`) | true (string marshaller) | false (line 725) | ✅ | + +This works because: +- The pending exception check doesn't belong in the "is marshalling required?" gate — it belongs in `PInvokeILEmitter.EmitPInvokeCall()` where it already independently calls `ShouldCheckForPendingException` and emits `ThrowPendingExceptionObject`. +- With `GeneratesPInvoke` now accurate, the try-catch in `ReadyToRunCodegenCompilation.cs` (line 224-233) also becomes unnecessary and should be removed. + +**Three things to change per BrzVlad:** +1. Remove lines 120-121 in `Marshaller.ReadyToRun.cs` (`ShouldCheckForPendingException` inside `IsMarshallingRequired`) +2. Remove lines 722-723 in `ReadyToRunCompilationModuleGroupBase.cs` (`ShouldCheckForPendingException` in `GeneratesPInvoke`), reverting it back to its original `return !Marshaller.IsMarshallingRequired(method)` form + +**⚠️ Keep the try-catch in `ReadyToRunCodegenCompilation.cs`** — contrary to BrzVlad's implication, this safety net is still needed. While the ObjC non-blittable case no longer triggers it, `EmitIL()` can still throw for `LCIDConversionAttribute` (line 86-87 of PInvokeILEmitter.cs), which is NOT checked by `IsMarshallingRequired`. The original CorInfoImpl try-catch (removed by the PR) handled this. Since the PR keeps the CorInfoImpl simplification, the CreateValueFromKey try-catch serves as the replacement. Update its comment to reflect the broader purpose rather than being ObjC-specific. + +**What stays in the PR (the actual feature):** +- `ThrowPendingExceptionObject()` method in `ObjectiveCMarshal.CoreCLR.cs` ✅ +- The corelib.h binder entry ✅ +- The `ShouldCheckForPendingException` + emit call in `PInvokeILEmitter.EmitPInvokeCall()` ✅ +- The simplified `CorInfoImpl.ReadyToRun.cs` cleanup ✅ + +### Impact: Should fix — eliminates unnecessary complexity + +The current code is **functionally correct** (the try-catch catches failures), but it: +1. Introduces an unnecessary try-catch workaround +2. Causes wasted compilation attempts for non-blittable ObjC P/Invokes +3. Makes the code harder to reason about +4. BrzVlad's fix is simpler AND more correct + +**Severity: 🟡 Should fix** — the PR is cleaner and more correct with BrzVlad's approach. + +--- + +## Concern 2: Non-Blittable ObjC Scope (BrzVlad — general comment) + +### BrzVlad's Claim + +> "Were there also any interpreted pinvokes with non-blittable types? In case we might need additional fixes, related to the original plan of using `LibraryImport`." + +### Verdict: ✅ Valid concern, but not a blocker for this PR + +- In practice, `objc_msgSend` is almost always called with **blittable** params (IntPtr handles, selectors, primitives). This covers the vast majority of real MAUI/iOS interop. +- Non-blittable ObjC P/Invokes would still fall back to JIT/interpreter — **same behavior as before this PR**. No regression. +- The `LibraryImport` source generator approach would handle non-blittable cases at compile time, but that's a separate effort. + +**Severity: 🟢 Low** — informational / future work, not a PR blocker. + +--- + +## Concern 3: Missing Test Coverage (AaronRobinsonMSFT + jkoritzinsky) + +### The Claims + +**AaronRobinsonMSFT:** "Do we need to re-enable or create new tests? We have tests at `src/tests/Interop/ObjectiveC/ObjectiveCMarshalAPI`, but are we missing them during R2R testing?" + +**jkoritzinsky:** "The tests there are falling back to JIT/Interpreter. We could add testing to validate that the methods were actually generated (use `--map` option when running cg2 and validate expected entries exist)." + +### Verdict: ✅ Fully agree — this is the most critical concern + +**Evidence:** +1. `ObjectiveCMarshalAPI` tests have NO `CrossGenTest` property — they never exercise R2R stubs +2. The PR adds NO new test files (`git diff main...HEAD --stat` shows 0 test files) +3. **If this PR were reverted, zero tests would fail** — the feature has no regression protection +4. Existing R2R tests in `src/tests/readytorun/tests/` use an established `--map` validation pattern that could be adopted + +**What a proper test would look like** (per jkoritzinsky's suggestion): +1. Define a test assembly with blittable `objc_msgSend` P/Invoke declarations +2. Compile with crossgen2 using `--map` flag +3. At runtime, load the `.map` file and assert the ObjC P/Invoke method entries exist +4. This proves the stubs were actually precompiled into R2R, not falling back to JIT + +**Severity: 🔴 High** — merge blocker. Without tests, this feature could silently regress. + +--- + +## Final Recommendations + +| Priority | Action | Reviewer | +|----------|--------|----------| +| 🔴 **Must fix** | Add R2R map-validation test proving ObjC stubs are precompiled | jkoritzinsky | +| 🟡 **Should fix** | Per BrzVlad: remove `ShouldCheckForPendingException` from `IsMarshallingRequired` (Marshaller.ReadyToRun.cs:120-121) and revert `GeneratesPInvoke` to `return !IsMarshallingRequired(method)` | BrzVlad | +| 🟡 **Should fix** | Keep the try-catch in `ReadyToRunCodegenCompilation.cs` but update its comment — it's still needed for non-ObjC edge cases like `LCIDConversionAttribute` | — | +| 🟢 **Track separately** | Non-blittable ObjC P/Invoke support via `LibraryImport` | BrzVlad | + +### What the PR should look like after fixes + +**Keep (the actual feature):** +- `ObjectiveCMarshal.ThrowPendingExceptionObject()` + corelib.h entry +- `PInvokeILEmitter.EmitPInvokeCall()` emitting the exception check after native calls +- `CorInfoImpl.ReadyToRun.cs` simplification +- try-catch in `ReadyToRunCodegenCompilation.cs` (with updated comment — still needed for `LCIDConversionAttribute` and other edge cases) + +**Remove (per BrzVlad):** +- `ShouldCheckForPendingException` from `IsMarshallingRequired` body +- `ShouldCheckForPendingException` rescue check from `GeneratesPInvoke` (revert to original form) + +**Add (per jkoritzinsky):** +- R2R `--map` validation test confirming ObjC stubs are precompiled +- Test location: `src/tests/Interop/ObjectiveC/` or `src/tests/readytorun/tests/` + +### Bottom Line + +The PR's **core approach is sound** — emitting `ThrowPendingExceptionObject()` after `objc_msgSend` calls mirrors the proven NativeAOT pattern. But the implementation has a logic layering issue (Concern 1) and zero test coverage (Concern 3). Both reviewers are correct in their concerns. From d390ab29203bb9ca6be8f4a8181ad8f951295c4f Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Fri, 13 Mar 2026 11:27:33 -0500 Subject: [PATCH 05/10] Update PR #124770 analysis with LibraryImport and P/Invoke inlining findings - Add LibraryImport vs DllImport analysis explaining why switching macios wouldn't solve the R2R precompilation issue - Add P/Invoke inlining call chain trace showing CorInfoImpl.ReadyToRun.cs path (not VM dllimport.cpp) for R2R marshalling decisions - Document graceful fallback behavior for unsupported marshalling types - Confirm BrzVlad's approach is both cleaner and more efficient Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pr-124770-analysis.md | 232 ++++++++++++++++++++++++++++++++++++++++++ pr-124770-plan.md | 13 +++ 2 files changed, 245 insertions(+) create mode 100644 pr-124770-analysis.md diff --git a/pr-124770-analysis.md b/pr-124770-analysis.md new file mode 100644 index 00000000000000..5d50c395888299 --- /dev/null +++ b/pr-124770-analysis.md @@ -0,0 +1,232 @@ +# PR #124770 Analysis: Enable R2R precompilation of objc_msgSend P/Invoke stubs + +## PR Overview + +- **Title**: Enable R2R precompilation of objc_msgSend P/Invoke stubs +- **Author**: davidnguyen-tech +- **Branch**: `feature/r2r-objc-pinvoke-stubs` → `main` +- **Status**: Draft, open +- **Files changed**: 6 (+41/-21) +- **Head commit**: 8226f744d0ff53032abee5786729fef021e05eaa + +## Problem Statement + +On iOS with CoreCLR, the JIT is unavailable. ReadyToRun (R2R) precompiles code ahead of time, but ObjC `objc_msgSend` P/Invoke stubs were explicitly blocked from R2R compilation — the `PInvokeILEmitter` threw `NotSupportedException` for any P/Invoke that needed a pending exception check. This forced these calls to fall back to runtime JIT stub generation, which on JIT-less iOS means falling back to the interpreter, causing a performance penalty. + +## Key Concepts + +### ReadyToRun (R2R) vs NativeAOT +- **R2R**: Hybrid precompilation. Produces native code bundled alongside IL. Can fall back to JIT at runtime for things it couldn't precompile. Used for CoreCLR on iOS. +- **NativeAOT**: Full ahead-of-time compilation. No IL, no JIT, no fallback. Standalone native binary. Already handles ObjC P/Invokes correctly. + +### Blittable Types +Types with identical memory layout in managed and unmanaged memory (e.g., `int`, `double`, `IntPtr`, flat structs of blittable fields). Non-blittable types (e.g., `string`, `bool`, arrays, classes with references) require marshalling — data conversion between managed and unmanaged representations. + +### `IsMarshallingRequired` +`Marshaller.IsMarshallingRequired(MethodDesc)` in `Marshaller.ReadyToRun.cs` determines whether a P/Invoke method needs an IL stub for parameter/return value marshalling. Returns `true` if any parameter is non-blittable, or if flags like `SetLastError`, `!PreserveSig`, `IsUnmanagedCallersOnly` are set. + +### `GeneratesPInvoke` +`ReadyToRunCompilationModuleGroupBase.GeneratesPInvoke(MethodDesc)` decides whether R2R should precompile a P/Invoke. Currently: `return !Marshaller.IsMarshallingRequired(method)` — i.e., only precompile if no marshalling is needed (blittable params, no special flags). + +### ObjC Pending Exception Check +After calling `objc_msgSend`, the runtime must check if the ObjC runtime set a pending exception and rethrow it on the managed side. This is done by calling `ObjectiveCMarshal.ThrowPendingExceptionObject()`. + +## Files Changed in the PR + +### 1. `src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs` +- Added `ThrowPendingExceptionObject()` — a new `[StackTraceHidden] internal static` method that calls `StubHelpers.GetPendingExceptionObject()` and rethrows via `ExceptionDispatchInfo.Throw(ex)`. + +### 2. `src/coreclr/vm/corelib.h` +- Registered `ThrowPendingExceptionObject` in the VM's managed method table under `#ifdef FEATURE_OBJCMARSHAL`. + +### 3. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs` +- In `EmitPInvokeCall`: After the native call, now emits `call ObjectiveCMarshal.ThrowPendingExceptionObject()` when `ShouldCheckForPendingException` is true. +- In `EmitIL()`: Removed the `NotSupportedException` throw that previously blocked ObjC P/Invokes entirely. + +### 4. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs` +- `GeneratesPInvoke()` expanded: now also returns `true` when `ShouldCheckForPendingException` is true, as an escape hatch to let ObjC P/Invokes through even if `IsMarshallingRequired` returns true. + +### 5. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs` +- Wrapped `PInvokeILEmitter.EmitIL(key)` in a try/catch for `RequiresRuntimeJitException`, returning null on failure (graceful fallback). + +### 6. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs` +- Simplified `pInvokeMarshalingRequired` by removing a redundant try/catch; now just checks if `GetMethodIL` returns null. + +## Reviewer Feedback: Vlad's Comment (r2845070125) + +Vlad commented on the change to `GeneratesPInvoke()` in `ReadyToRunCompilationModuleGroupBase.cs`: + +> "this check here looks wrong. This check is already done as part of `Marshaller.IsMarshallingRequired` and I believe it should be removed from there. In your case, this method is returning true for pinvokes with non-blittable types that require check for pending exception. It should be returning false instead and I believe this is the reason you are forcefully catching the requires jit exception above." + +### Analysis: Vlad is Correct + +The root issue is that `ShouldCheckForPendingException` inside `IsMarshallingRequired` **conflates two different concerns**: +1. "Does this P/Invoke need a pending exception check?" (simple — R2R can now emit this) +2. "Does this P/Invoke need complex parameter marshalling?" (complex — R2R may not handle this) + +Because `IsMarshallingRequired` returns `true` for ObjC P/Invokes (due to the pending exception check), the PR had to add: +- An escape hatch in `GeneratesPInvoke` (the `ShouldCheckForPendingException` override) +- A try/catch in `ReadyToRunCodegenCompilation.cs` for when the emitter still can't handle non-blittable ObjC P/Invokes + +This is a workaround for a problem that shouldn't exist. + +## Git History Investigation + +### Original Commit: 4a782d58ac4 (Aaron Robinson, May 2021, PR #52849) + +**"Objective-C msgSend* support for pending exceptions in Release"** + +Aaron added `ShouldCheckForPendingException` to `IsMarshallingRequired` as a **two-layer safety net**: + +1. **Layer 1 — `IsMarshallingRequired` returns `true`**: Prevents `GeneratesPInvoke` from returning `true`, so R2R won't try to inline ObjC P/Invokes as raw native calls (bypassing the stub entirely). + +2. **Layer 2 — `PInvokeILEmitter.EmitIL()` throws `NotSupportedException`**: Even if R2R tries the stub path, it fails gracefully and falls back to runtime JIT. + +**Why it was designed this way**: At the time (2021), CrossGen2/R2R had **no ability** to emit the pending exception check. The check in `IsMarshallingRequired` was an expedient way to keep ObjC methods off the R2R fast path entirely. It wasn't saying "this needs parameter marshalling" — it was saying "this needs a stub that R2R can't produce." + +The VM-level equivalent in `dllimport.cpp` has the same check inside `NDirect::MarshalingRequired()`, guarded by `#ifndef CROSSGEN_COMPILE` (meaning it was already excluded from the old crossgen path). + +## Recommended Plan + +### What should change (addressing Vlad's feedback) + +1. **Remove `ShouldCheckForPendingException` from `IsMarshallingRequired`** in `Marshaller.ReadyToRun.cs` (lines 120-121) + - This makes `IsMarshallingRequired` purely about parameter marshalling again. + - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs` + +2. **Remove the escape hatch from `GeneratesPInvoke`** in `ReadyToRunCompilationModuleGroupBase.cs` + - Revert `GeneratesPInvoke` back to just `return !Marshaller.IsMarshallingRequired(method)` + - The ObjC P/Invokes with blittable params will now naturally pass through (IsMarshallingRequired=false → GeneratesPInvoke=true) + - ObjC P/Invokes with non-blittable params will correctly be excluded (IsMarshallingRequired=true → GeneratesPInvoke=false → runtime fallback) + - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs` + +3. **Keep the try/catch safety net** in `ReadyToRunCodegenCompilation.cs` (but update the comment) + - `PInvokeILEmitter.EmitIL()` throws `NotSupportedException` for `LCIDConversionAttribute`, which is NOT checked by `IsMarshallingRequired`. A blittable P/Invoke with `[LCIDConversion]` would pass `GeneratesPInvoke` but fail in `EmitIL`. The try/catch is the only thing preventing crossgen2 from crashing. + - Update the comment to explain this is a general safety net for any `EmitIL` failure, not ObjC-specific. + - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs` + +4. **Keep the `PInvokeILEmitter.cs` changes** that emit `ThrowPendingExceptionObject()` + - This is the core value of the PR — teaching R2R to emit the pending exception check + - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs` + +5. **Keep the `CorInfoImpl.ReadyToRun.cs` simplification** + - The simplified null check is cleaner regardless + - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs` + +6. **Keep the CoreLib/VM changes** (`ObjectiveCMarshal.CoreCLR.cs`, `corelib.h`) + - These provide the managed method that the emitted stub calls + - Files: `src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs`, `src/coreclr/vm/corelib.h` + +### Expected behavior after the fix + +| Scenario | `IsMarshallingRequired` | `GeneratesPInvoke` | Result | +|----------|------------------------|-------------------|--------| +| ObjC P/Invoke, blittable params | `false` | `true` | R2R precompiles with pending exception check | +| ObjC P/Invoke, non-blittable params | `true` (due to params) | `false` | Falls back to runtime (correct) | +| Regular P/Invoke, blittable params | `false` | `true` | R2R precompiles (unchanged) | +| Regular P/Invoke, non-blittable params | `true` | `false` | Falls back to runtime (unchanged) | + +--- + +## LibraryImport vs DllImport Analysis + +### Question: Would switching macios to `[LibraryImport]` fix this? + +**Answer: No.** `[LibraryImport]` does **not** solve the problem and would cause significant breakage. + +### How LibraryImport Works Under the Hood + +`[LibraryImport]` uses a Roslyn source generator to emit a managed wrapper that handles marshalling at compile time. But the **inner** P/Invoke that the generator emits is still a `[DllImport("libobjc.dylib", EntryPoint = "objc_msgSend")]` targeting blittable types. + +That inner call: +1. Still targets `objc_msgSend` in `libobjc.dylib` +2. Still triggers `ShouldCheckForPendingException` in both the VM and R2R +3. Still gets blocked from R2R precompilation by the same `IsMarshallingRequired` gate + +**Verification:** `ShouldCheckForPendingException` (in `MarshalHelpers.cs:935-955`) matches on `metadata.Module` (the library path) and `metadata.Name` (the entry point). With `[LibraryImport]`, the source generator preserves both on the inner `[DllImport]`, so the check still triggers. + +### Why LibraryImport Would Break Users + +1. **Binary breaking change** — `[DllImport] extern` methods have a fundamentally different calling convention than `[LibraryImport]` generated wrappers. Existing compiled assemblies referencing these methods would fail at runtime. +2. **Massive scope** — macios has hundreds/thousands of `objc_msgSend` overloads. These are the backbone of all iOS/macOS ObjC interop. +3. **Source generator doesn't know about pending exceptions** — `[LibraryImport]`'s generator has no concept of `ObjectiveCMarshal.ThrowPendingExceptionObject()`. You'd need custom logic in the generated wrapper to call it, which the source generator doesn't support. + +### After This PR's Fix + +After removing `ShouldCheckForPendingException` from `IsMarshallingRequired`, the inner `[DllImport]` generated by `[LibraryImport]` (which has blittable-only parameters) *would* pass `IsMarshallingRequired` = `false` → `GeneratesPInvoke` = `true` → R2R precompiles it with the pending exception check. So the fix works equally well regardless of whether the consumer uses `[DllImport]` or `[LibraryImport]`. + +### Conclusion + +Switching macios to `[LibraryImport]` wouldn't solve the problem *without* this runtime fix, and would be a massive breaking change for no benefit. The fix belongs in the runtime (this PR). + +--- + +## P/Invoke Inlining: Which `IsMarshallingRequired` Gets Called? + +### The Question + +When the JIT considers "inlining" a P/Invoke (embedding the native call directly instead of going through an IL stub), which marshalling check does it consult? + +### Answer: R2R uses `CorInfoImpl.ReadyToRun.cs`, NOT the VM `dllimport.cpp` + +**Full call chain for R2R P/Invoke direct-call decisions:** + +``` +JIT: impCheckForPInvokeCall (importercalls.cpp:6849) + → CorInfoImpl.pInvokeMarshalingRequired (CorInfoImpl.ReadyToRun.cs:3111-3142) + → _compilation.GetMethodIL(method) + → GeneratesPInvoke gate (ReadyToRunCompilationModuleGroupBase.cs:712-725) + → PInvokeILEmitter.EmitIL (PInvokeILEmitter.cs) + → Marshaller.IsMarshallingRequired (Marshaller.ReadyToRun.cs:104-131) + → PInvokeILStubMethodIL.IsMarshallingRequired property +``` + +**Key details of `CorInfoImpl.pInvokeMarshalingRequired` (lines 3111-3142):** + +1. If `method.IsRawPInvoke()` → return `false` (this is the synthetic raw native target *inside* an IL stub) +2. If method is outside the version bubble → return `true` +3. Call `_compilation.GetMethodIL(method)` — if null → return `true` (don't inline; runtime fallback) +4. Otherwise return `((PInvokeILStubMethodIL)stubIL).IsMarshallingRequired` + +The `IsMarshallingRequired` property is set during `PInvokeILEmitter.EmitIL()` from `Marshaller.IsMarshallingRequired(MethodDesc)`. + +**The VM path** (`CEEInfo::pInvokeMarshalingRequired` → `NDirect::MarshalingRequired` in `dllimport.cpp`) is only used by **normal runtime JIT**, not R2R. + +### Is Inlining a P/Invoke That Requires Marshalling Safe? + +**No — but the system prevents it with graceful fallback.** + +R2R's marshaller factory (`Marshaller.ReadyToRun.cs:12-25`) only supports trivially blittable cases: +- Supported: `Enum`, `BlittableValue`, `BlittableStruct`, `UnicodeChar`, `VoidReturn` +- Everything else: `new NotSupportedMarshaller()` + +If unsupported marshalling is encountered: +1. `NotSupportedMarshaller.EmitMarshallingIL()` throws `NotSupportedException` +2. `PInvokeILEmitter.EmitIL()` catches it, rethrows as `RequiresRuntimeJitException` +3. `ReadyToRunCodegenCompilation.cs` catches that, returns `null` for `methodIL` +4. `CorInfoImpl.pInvokeMarshalingRequired` sees `stubIL == null` → returns `true` +5. JIT sees marshalling required → does NOT direct-call → falls back to runtime stub + +**It never silently generates incorrect direct-call code.** + +### Implications for This PR + +The current PR adds a `ShouldCheckForPendingException` escape hatch in `GeneratesPInvoke` that returns `true` for ALL ObjC P/Invokes, including non-blittable ones. For non-blittable ObjC P/Invokes, this causes: +- `GeneratesPInvoke` = true → R2R attempts to emit IL stub +- `NotSupportedMarshaller` throws → `RequiresRuntimeJitException` → caught by try-catch → `stubIL = null` +- Fallback to runtime (correct but wasteful) + +**With BrzVlad's fix** (removing the check from `IsMarshallingRequired`): +- Non-blittable ObjC: `IsMarshallingRequired` = true (due to params) → `GeneratesPInvoke` = false → never attempts emission → no waste +- Blittable ObjC: `IsMarshallingRequired` = false → `GeneratesPInvoke` = true → R2R emits stub with pending exception check → works perfectly + +This confirms BrzVlad's approach is both cleaner and more efficient. + +--- + +### Build & Test + +- Component: CoreCLR (files under `src/coreclr/`) +- Baseline build: `./build.sh clr+libs+host` (from main branch first) +- After changes: rebuild tools and run tests +- The ObjC interop tests are in `src/libraries/System.Runtime.InteropServices/tests/` — look for ObjectiveC-related test files diff --git a/pr-124770-plan.md b/pr-124770-plan.md index fc34ca1b36e15d..95c32959348dda 100644 --- a/pr-124770-plan.md +++ b/pr-124770-plan.md @@ -167,3 +167,16 @@ The current code is **functionally correct** (the try-catch catches failures), b ### Bottom Line The PR's **core approach is sound** — emitting `ThrowPendingExceptionObject()` after `objc_msgSend` calls mirrors the proven NativeAOT pattern. But the implementation has a logic layering issue (Concern 1) and zero test coverage (Concern 3). Both reviewers are correct in their concerns. + +--- + +## Appendix: LibraryImport vs DllImport + +A question was raised about whether switching the macios SDK from `[DllImport]` to `[LibraryImport]` would avoid the need for this PR entirely. **The answer is no:** + +- `[LibraryImport]`'s source generator emits an inner `[DllImport]` with the same library name and entry point → `ShouldCheckForPendingException` still matches +- The generator has no concept of `ObjectiveCMarshal.ThrowPendingExceptionObject()` and cannot emit the pending exception check +- Switching would be a binary breaking change across hundreds of `objc_msgSend` overloads +- **After this PR's fix**, both `[DllImport]` and `[LibraryImport]` consumers benefit automatically — the fix is in the runtime, not in consumer code + +See `pr-124770-analysis.md` for the full analysis. From 9e02c648e784b6da379c6dc8c6de30e8cf4c39dc Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Tue, 17 Mar 2026 13:56:12 -0500 Subject: [PATCH 06/10] Update PR #124770 notes for March 17 force-push - Document new 3-commit structure and PInvokeILStubMethodIL mechanism - Mark BrzVlad's recommendations as implemented (GeneratesPInvoke reverted, ShouldCheckForPendingException removed from IsMarshallingRequired) - Add open concerns: code size, P/Invoke frame correctness, non-void IL stack risk, JIT helper alternative under investigation - Test coverage still flagged as missing (merge blocker) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pr-124770-analysis.md | 136 ++++++++++++++++++++++++------------- pr-124770-plan.md | 153 ++++++++++++++++++------------------------ 2 files changed, 154 insertions(+), 135 deletions(-) diff --git a/pr-124770-analysis.md b/pr-124770-analysis.md index 5d50c395888299..a999424d02b394 100644 --- a/pr-124770-analysis.md +++ b/pr-124770-analysis.md @@ -6,8 +6,12 @@ - **Author**: davidnguyen-tech - **Branch**: `feature/r2r-objc-pinvoke-stubs` → `main` - **Status**: Draft, open -- **Files changed**: 6 (+41/-21) -- **Head commit**: 8226f744d0ff53032abee5786729fef021e05eaa +- **March 17 update**: The PR was force-pushed and restructured into 3 cleaner commits: + 1. `01a7fe190ad` — add `ThrowPendingExceptionObject()` plus the `corelib.h` binder entry + 2. `b41aa09deb6` — emit `ThrowPendingExceptionObject` in `PInvokeILEmitter.EmitPInvokeCall()` and remove the old ObjC blocker + 3. `e04bc858067` — remove `ShouldCheckForPendingException` from `Marshaller.IsMarshallingRequired` and move the direct-call suppression to `PInvokeILStubMethodIL.IsMarshallingRequired` +- **Historical note**: The earlier 6-file snapshot below is still useful context, but sections 4-6 are now historical because the force-push removed that part of the design. +- **Scope note**: The March 17 updates in this document describe the PR as force-pushed on GitHub, even if the local checkout you are reading alongside this note still reflects the older pre-force-push revision. ## Problem Statement @@ -26,7 +30,10 @@ Types with identical memory layout in managed and unmanaged memory (e.g., `int`, `Marshaller.IsMarshallingRequired(MethodDesc)` in `Marshaller.ReadyToRun.cs` determines whether a P/Invoke method needs an IL stub for parameter/return value marshalling. Returns `true` if any parameter is non-blittable, or if flags like `SetLastError`, `!PreserveSig`, `IsUnmanagedCallersOnly` are set. ### `GeneratesPInvoke` -`ReadyToRunCompilationModuleGroupBase.GeneratesPInvoke(MethodDesc)` decides whether R2R should precompile a P/Invoke. Currently: `return !Marshaller.IsMarshallingRequired(method)` — i.e., only precompile if no marshalling is needed (blittable params, no special flags). +`ReadyToRunCompilationModuleGroupBase.GeneratesPInvoke(MethodDesc)` decides whether R2R should precompile a P/Invoke. In the force-pushed PR, it is back to the original `return !Marshaller.IsMarshallingRequired(method)` shape — i.e., only precompile if parameter marshalling itself is not required. + +### `PInvokeILStubMethodIL.IsMarshallingRequired` (class in `PInvokeILEmitter.cs`) +`PInvokeILStubMethodIL.IsMarshallingRequired` is what `CorInfoImpl.ReadyToRun.cs` ultimately reports back to the JIT for R2R direct-call decisions once a stub has been built. The March 17 force-push moves the ObjC pending-exception special case here so R2R can still precompile a stub for blittable ObjC signatures while preventing the JIT from bypassing that stub as a raw native direct-call. ### ObjC Pending Exception Check After calling `objc_msgSend`, the runtime must check if the ObjC runtime set a pending exception and rethrow it on the managed side. This is done by calling `ObjectiveCMarshal.ThrowPendingExceptionObject()`. @@ -40,17 +47,27 @@ After calling `objc_msgSend`, the runtime must check if the ObjC runtime set a p - Registered `ThrowPendingExceptionObject` in the VM's managed method table under `#ifdef FEATURE_OBJCMARSHAL`. ### 3. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs` -- In `EmitPInvokeCall`: After the native call, now emits `call ObjectiveCMarshal.ThrowPendingExceptionObject()` when `ShouldCheckForPendingException` is true. -- In `EmitIL()`: Removed the `NotSupportedException` throw that previously blocked ObjC P/Invokes entirely. +- In `EmitPInvokeCall`: After the native call, the force-pushed PR now emits `call ObjectiveCMarshal.ThrowPendingExceptionObject()` when `ShouldCheckForPendingException` is true. +- In `EmitIL()`: Removed the old `NotSupportedException` throw that previously blocked ObjC P/Invokes entirely. +- The intended stub shape is now: `marshal args → call objc_msgSend → call ThrowPendingExceptionObject → ret`. +- **Open concern**: that shape is trivial for `void` returns, but non-void returns must preserve the native return value across the helper call. A Copilot review comment flagged possible IL stack corruption if the helper is emitted while the return value is still on the evaluation stack. ### 4. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs` -- `GeneratesPInvoke()` expanded: now also returns `true` when `ShouldCheckForPendingException` is true, as an escape hatch to let ObjC P/Invokes through even if `IsMarshallingRequired` returns true. +- **Update (March 17)**: The temporary `GeneratesPInvoke` escape hatch was removed from the PR. +- `GeneratesPInvoke()` is back to `return !Marshaller.IsMarshallingRequired(method)`. +- The old escape-hatch design is still important historical context because it explains Vlad's review comment, but it is no longer in the current PR. ### 5. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs` -- Wrapped `PInvokeILEmitter.EmitIL(key)` in a try/catch for `RequiresRuntimeJitException`, returning null on failure (graceful fallback). +- **Update (March 17)**: The temporary try/catch for `RequiresRuntimeJitException` was removed from the PR. +- The new design relies on better up-front gating rather than speculative emission plus catch-and-fallback. ### 6. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs` -- Simplified `pInvokeMarshalingRequired` by removing a redundant try/catch; now just checks if `GetMethodIL` returns null. +- **Update (March 17)**: This file is no longer part of the PR. +- The earlier simplification became unnecessary once the design moved the ObjC special case into `PInvokeILStubMethodIL.IsMarshallingRequired` instead. + +### 7. `src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs` (`PInvokeILStubMethodIL.IsMarshallingRequired`) +- **Key March 17 change**: `ShouldCheckForPendingException` was removed from `Marshaller.IsMarshallingRequired` and instead reflected in `PInvokeILStubMethodIL.IsMarshallingRequired`. +- That keeps blittable ObjC P/Invokes eligible for stub generation while still telling the JIT that a raw direct-call would be incorrect because it would skip `ThrowPendingExceptionObject()`. ## Reviewer Feedback: Vlad's Comment (r2845070125) @@ -70,6 +87,14 @@ Because `IsMarshallingRequired` returns `true` for ObjC P/Invokes (due to the pe This is a workaround for a problem that shouldn't exist. +### Update (March 17) + +The force-pushed PR adopted Vlad's core suggestion: +- `ShouldCheckForPendingException` was removed from `Marshaller.IsMarshallingRequired` +- the `GeneratesPInvoke` escape hatch was removed + +It also added an extra safeguard that was not part of the earlier review thread: `PInvokeILStubMethodIL.IsMarshallingRequired` now carries the ObjC pending-exception requirement so the JIT still avoids the raw direct-call path for blittable ObjC P/Invokes. + ## Git History Investigation ### Original Commit: 4a782d58ac4 (Aaron Robinson, May 2021, PR #52849) @@ -88,43 +113,60 @@ The VM-level equivalent in `dllimport.cpp` has the same check inside `NDirect::M ## Recommended Plan -### What should change (addressing Vlad's feedback) +### Update (March 17): what is already implemented vs what is still open -1. **Remove `ShouldCheckForPendingException` from `IsMarshallingRequired`** in `Marshaller.ReadyToRun.cs` (lines 120-121) - - This makes `IsMarshallingRequired` purely about parameter marshalling again. +1. **✅ Implemented: remove `ShouldCheckForPendingException` from `Marshaller.IsMarshallingRequired`** + - This restores `Marshaller.IsMarshallingRequired` to being about actual parameter/return marshalling. - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs` -2. **Remove the escape hatch from `GeneratesPInvoke`** in `ReadyToRunCompilationModuleGroupBase.cs` - - Revert `GeneratesPInvoke` back to just `return !Marshaller.IsMarshallingRequired(method)` - - The ObjC P/Invokes with blittable params will now naturally pass through (IsMarshallingRequired=false → GeneratesPInvoke=true) - - ObjC P/Invokes with non-blittable params will correctly be excluded (IsMarshallingRequired=true → GeneratesPInvoke=false → runtime fallback) +2. **✅ Implemented: remove the `GeneratesPInvoke` escape hatch** + - `GeneratesPInvoke` is back to `return !Marshaller.IsMarshallingRequired(method)`. + - Blittable ObjC P/Invokes now get through naturally; non-blittable ObjC P/Invokes are excluded naturally. - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs` -3. **Keep the try/catch safety net** in `ReadyToRunCodegenCompilation.cs` (but update the comment) - - `PInvokeILEmitter.EmitIL()` throws `NotSupportedException` for `LCIDConversionAttribute`, which is NOT checked by `IsMarshallingRequired`. A blittable P/Invoke with `[LCIDConversion]` would pass `GeneratesPInvoke` but fail in `EmitIL`. The try/catch is the only thing preventing crossgen2 from crashing. - - Update the comment to explain this is a general safety net for any `EmitIL` failure, not ObjC-specific. - - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs` - -4. **Keep the `PInvokeILEmitter.cs` changes** that emit `ThrowPendingExceptionObject()` - - This is the core value of the PR — teaching R2R to emit the pending exception check +3. **✅ Implemented: move the ObjC direct-call suppression to `PInvokeILStubMethodIL.IsMarshallingRequired`** + - This is the key new mechanism from the force-push. + - It preserves stub generation for blittable ObjC signatures while still preventing the JIT from treating them as raw direct-call candidates. - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs` -5. **Keep the `CorInfoImpl.ReadyToRun.cs` simplification** - - The simplified null check is cleaner regardless - - File: `src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs` +4. **✅ Implemented: keep the `PInvokeILEmitter.cs` changes that emit `ThrowPendingExceptionObject()`** + - This remains the core value of the PR. + - The generated stub is intended to be `marshal args → call objc_msgSend → call ThrowPendingExceptionObject → ret`. + +5. **✅ Implemented: keep the CoreLib/VM changes** (`ObjectiveCMarshal.CoreCLR.cs`, `corelib.h`) + - These provide the managed helper the emitted stub calls. + +6. **❌ Still open: add regression coverage and resolve the new design questions** + - Add an R2R `--map` validation test proving ObjC stubs are precompiled. + - Validate non-void return handling, image-size impact, and P/Invoke frame correctness. + - Keep the JIT-helper alternative on the table if the emitted-IL approach becomes too awkward. + +### Expected behavior after the force-push + +| Scenario | `Marshaller.IsMarshallingRequired` | `GeneratesPInvoke` | `PInvokeILStubMethodIL.IsMarshallingRequired` | Result | +|----------|------------------------------------|-------------------|-----------------------------------------------|--------| +| ObjC P/Invoke, blittable params | `false` | `true` | `true` | R2R emits a stub and the JIT stays on the stub path so the pending-exception helper runs | +| ObjC P/Invoke, non-blittable params | `true` (due to params) | `false` | n/a | Falls back to runtime/interpreter (correct) | +| Regular P/Invoke, blittable params | `false` | `true` | `false` | Existing direct-call behavior stays unchanged | +| Regular P/Invoke, non-blittable params | `true` | `false` | n/a | Falls back to runtime (unchanged) | + +## Open Concerns After the March 17 Force-Push + +1. **Code size / stub count impact** + - More ObjC callsites now intentionally take the precompiled stub path. + - That is probably the right trade-off, but it should be measured rather than assumed. -6. **Keep the CoreLib/VM changes** (`ObjectiveCMarshal.CoreCLR.cs`, `corelib.h`) - - These provide the managed method that the emitted stub calls - - Files: `src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs`, `src/coreclr/vm/corelib.h` +2. **P/Invoke frame correctness** + - Forcing the stub path changes where the pending-exception logic runs. + - It is worth validating that the resulting frame/transition behavior matches what the runtime expects for ObjC interop on iOS. -### Expected behavior after the fix +3. **Non-void return IL shape** + - The simple `call native → call ThrowPendingExceptionObject → ret` sketch is only obviously correct for `void` returns. + - For non-void P/Invokes, the return value likely needs to be stored and reloaded around the helper call. -| Scenario | `IsMarshallingRequired` | `GeneratesPInvoke` | Result | -|----------|------------------------|-------------------|--------| -| ObjC P/Invoke, blittable params | `false` | `true` | R2R precompiles with pending exception check | -| ObjC P/Invoke, non-blittable params | `true` (due to params) | `false` | Falls back to runtime (correct) | -| Regular P/Invoke, blittable params | `false` | `true` | R2R precompiles (unchanged) | -| Regular P/Invoke, non-blittable params | `true` | `false` | Falls back to runtime (unchanged) | +4. **JIT helper alternative** + - jkoritzinsky suggested a JIT-helper implementation instead of emitting the helper call directly in IL. + - David and jkoritzinsky exploring that option is reasonable if it simplifies stack handling, frame correctness, or code size. --- @@ -141,7 +183,7 @@ The VM-level equivalent in `dllimport.cpp` has the same check inside `NDirect::M That inner call: 1. Still targets `objc_msgSend` in `libobjc.dylib` 2. Still triggers `ShouldCheckForPendingException` in both the VM and R2R -3. Still gets blocked from R2R precompilation by the same `IsMarshallingRequired` gate +3. In the pre-force-push design, it was blocked by the same `IsMarshallingRequired` gate; in the March 17 design, that gate is removed and the direct-call suppression happens later via `PInvokeILStubMethodIL.IsMarshallingRequired` **Verification:** `ShouldCheckForPendingException` (in `MarshalHelpers.cs:935-955`) matches on `metadata.Module` (the library path) and `metadata.Name` (the entry point). With `[LibraryImport]`, the source generator preserves both on the inner `[DllImport]`, so the check still triggers. @@ -151,9 +193,9 @@ That inner call: 2. **Massive scope** — macios has hundreds/thousands of `objc_msgSend` overloads. These are the backbone of all iOS/macOS ObjC interop. 3. **Source generator doesn't know about pending exceptions** — `[LibraryImport]`'s generator has no concept of `ObjectiveCMarshal.ThrowPendingExceptionObject()`. You'd need custom logic in the generated wrapper to call it, which the source generator doesn't support. -### After This PR's Fix +### After This PR's Force-Pushed Design -After removing `ShouldCheckForPendingException` from `IsMarshallingRequired`, the inner `[DllImport]` generated by `[LibraryImport]` (which has blittable-only parameters) *would* pass `IsMarshallingRequired` = `false` → `GeneratesPInvoke` = `true` → R2R precompiles it with the pending exception check. So the fix works equally well regardless of whether the consumer uses `[DllImport]` or `[LibraryImport]`. +After the March 17 force-push, the inner `[DllImport]` generated by `[LibraryImport]` (which has blittable-only parameters) can pass `Marshaller.IsMarshallingRequired = false` and `GeneratesPInvoke = true`, while `PInvokeILStubMethodIL.IsMarshallingRequired = true` still keeps the JIT on the stub path so `ThrowPendingExceptionObject()` runs. So the fix still benefits both `[DllImport]` and `[LibraryImport]` consumers automatically — the mechanism just moved. ### Conclusion @@ -188,7 +230,7 @@ JIT: impCheckForPInvokeCall (importercalls.cpp:6849) 3. Call `_compilation.GetMethodIL(method)` — if null → return `true` (don't inline; runtime fallback) 4. Otherwise return `((PInvokeILStubMethodIL)stubIL).IsMarshallingRequired` -The `IsMarshallingRequired` property is set during `PInvokeILEmitter.EmitIL()` from `Marshaller.IsMarshallingRequired(MethodDesc)`. +In the force-pushed PR, `PInvokeILStubMethodIL.IsMarshallingRequired` is no longer just a mirror of `Marshaller.IsMarshallingRequired(MethodDesc)`. It also carries the ObjC pending-exception requirement so the JIT will not raw-inline/direct-call a blittable ObjC P/Invoke and accidentally skip `ThrowPendingExceptionObject()`. **The VM path** (`CEEInfo::pInvokeMarshalingRequired` → `NDirect::MarshalingRequired` in `dllimport.cpp`) is only used by **normal runtime JIT**, not R2R. @@ -211,16 +253,16 @@ If unsupported marshalling is encountered: ### Implications for This PR -The current PR adds a `ShouldCheckForPendingException` escape hatch in `GeneratesPInvoke` that returns `true` for ALL ObjC P/Invokes, including non-blittable ones. For non-blittable ObjC P/Invokes, this causes: -- `GeneratesPInvoke` = true → R2R attempts to emit IL stub -- `NotSupportedMarshaller` throws → `RequiresRuntimeJitException` → caught by try-catch → `stubIL = null` -- Fallback to runtime (correct but wasteful) +The force-pushed PR no longer relies on a `GeneratesPInvoke` escape hatch. Instead, it splits the two concerns cleanly: +- **Stub generation** still uses `Marshaller.IsMarshallingRequired` +- **JIT direct-call suppression** now uses `PInvokeILStubMethodIL.IsMarshallingRequired` -**With BrzVlad's fix** (removing the check from `IsMarshallingRequired`): -- Non-blittable ObjC: `IsMarshallingRequired` = true (due to params) → `GeneratesPInvoke` = false → never attempts emission → no waste -- Blittable ObjC: `IsMarshallingRequired` = false → `GeneratesPInvoke` = true → R2R emits stub with pending exception check → works perfectly +That means: +- **Non-blittable ObjC**: `Marshaller.IsMarshallingRequired = true` (because of real marshalling needs) → `GeneratesPInvoke = false` → no speculative emission +- **Blittable ObjC**: `Marshaller.IsMarshallingRequired = false` → `GeneratesPInvoke = true` → R2R emits a stub, but `PInvokeILStubMethodIL.IsMarshallingRequired = true` keeps the JIT from bypassing that stub +- **Regular blittable P/Invokes**: both stay false, so existing direct-call behavior is preserved -This confirms BrzVlad's approach is both cleaner and more efficient. +This is cleaner than the old escape-hatch design. The remaining question is no longer the layering — it is whether the emitted stub shape is correct and worth the code-size/runtime trade-off. --- diff --git a/pr-124770-plan.md b/pr-124770-plan.md index 95c32959348dda..d0a53ae0191dc3 100644 --- a/pr-124770-plan.md +++ b/pr-124770-plan.md @@ -2,7 +2,7 @@ ## Summary -Analysis of 3 reviewer concerns on PR #124770 ("Enable R2R precompilation of objc_msgSend P/Invoke stubs"), validated against the actual code. +Analysis of the original 3 reviewer concerns on PR #124770 ("Enable R2R precompilation of objc_msgSend P/Invoke stubs"), updated for the March 17 force-push that replaced the earlier escape-hatch/try-catch design with a cleaner 3-commit approach. These March 17 notes describe the PR's force-pushed GitHub state, even if the local checkout still contains the earlier revision. ### Historical Context @@ -20,80 +20,30 @@ Now that R2R **can** emit the check (via this PR's `ThrowPendingExceptionObject` > "This check here looks wrong. This check is already done as part of `Marshaller.IsMarshallingRequired` and I believe it should be removed from there. In your case, this method is returning true for pinvokes with non-blittable types that require check for pending exception. It should be returning false instead and I believe this is the reason you are forcefully catching the requires jit exception above." -### Verdict: ✅ BrzVlad is correct — the root cause is a conflation of concerns +### Verdict: ✅ BrzVlad's core concern was correct, and the force-push implemented it -**The code evidence:** +**Historical context:** the earlier PR revision conflated two concerns: +1. "Does this P/Invoke need a pending exception check?" +2. "Does this P/Invoke need real parameter/return marshalling?" -`Marshaller.IsMarshallingRequired` (Marshaller.ReadyToRun.cs:104-131) contains: -```csharp -// line 120-121: -if (MarshalHelpers.ShouldCheckForPendingException(targetMethod.Context.Target, metadata)) - return true; -// line 123-128: then checks each marshaller for type conversion needs -``` +That conflation is what created the old `GeneratesPInvoke` escape hatch and the temporary `ReadyToRunCodegenCompilation.cs` try/catch workaround. -This means `IsMarshallingRequired` returns `true` for ANY ObjC P/Invoke **before even checking parameter types**. It conflates "needs ObjC exception check" with "needs type marshalling." +### Update (March 17): implemented in the force-push -`GeneratesPInvoke` (ReadyToRunCompilationModuleGroupBase.cs:712-726): -```csharp -if (!Marshaller.IsMarshallingRequired(method)) // line 719 - return true; // line 720 — blittable non-ObjC -if (MarshalHelpers.ShouldCheckForPendingException(...)) // line 722 - return true; // line 723 — ANY ObjC -return false; // line 725 -``` +The restructured PR now: +1. Removes `ShouldCheckForPendingException` from `Marshaller.IsMarshallingRequired` +2. Reverts `GeneratesPInvoke` to `return !Marshaller.IsMarshallingRequired(method)` +3. Adds the ObjC special-case to `PInvokeILStubMethodIL.IsMarshallingRequired` -**Traced scenarios:** +That third bullet is the key extra insight beyond Vlad's original review comment: without it, the JIT could see a blittable ObjC signature and raw-inline/direct-call it, skipping `ThrowPendingExceptionObject()` entirely. -| Scenario | `IsMarshallingRequired` | `GeneratesPInvoke` | Actually compilable? | -|----------|------------------------|--------------------|--------------------| -| Blittable non-ObjC (`Sleep(int)`) | false | true (line 720) | ✅ Yes | -| Blittable ObjC (`objc_msgSend(IntPtr, IntPtr)`) | true (line 120) | true (line 723) | ✅ Yes | -| **Non-blittable ObjC** (`objc_msgSend(IntPtr, IntPtr, string)`) | true (line 120) | **true (line 723)** | ❌ **No!** | +### Current assessment -For the non-blittable ObjC case, `GeneratesPInvoke` promises "yes, we'll compile this," but then `PInvokeILEmitter.EmitIL()` hits `NotSupportedMarshaller` for the `string` parameter → throws `NotSupportedException` → converted to `RequiresRuntimeJitException` (line 133-135) → caught by the try-catch in `ReadyToRunCodegenCompilation.cs` (line 228-233). +- The original layering issue is fixed. +- The removed pieces are the `GeneratesPInvoke` escape hatch, the `ReadyToRunCodegenCompilation.cs` try/catch, and the `CorInfoImpl.ReadyToRun.cs` cleanup. +- The remaining work is validating the new stub shape, code-size impact, P/Invoke frame correctness, and test coverage. -**The try-catch IS a workaround** for `GeneratesPInvoke` returning an incorrect `true`. - -### BrzVlad's suggested fix (remove from both places) - -BrzVlad is saying remove `ShouldCheckForPendingException` from **both**: -1. **`IsMarshallingRequired`** (Marshaller.ReadyToRun.cs line 120-121) — "needs ObjC exception check" ≠ "needs marshalling" -2. **`GeneratesPInvoke` line 722-723** (the PR's addition) — "this check here looks wrong" - -With both removed, `GeneratesPInvoke` simplifies back to just `!IsMarshallingRequired`: - -| Scenario | New `IsMarshallingRequired` | New `GeneratesPInvoke` | Correct? | -|----------|---------------------------|----------------------|----------| -| Blittable non-ObjC (`Sleep(int)`) | false | true (line 720) | ✅ | -| Blittable ObjC (`objc_msgSend(IntPtr, IntPtr)`) | **false** (all params blittable) | true (line 720) | ✅ | -| Non-blittable ObjC (`objc_msgSend(IntPtr, string)`) | true (string marshaller) | false (line 725) | ✅ | - -This works because: -- The pending exception check doesn't belong in the "is marshalling required?" gate — it belongs in `PInvokeILEmitter.EmitPInvokeCall()` where it already independently calls `ShouldCheckForPendingException` and emits `ThrowPendingExceptionObject`. -- With `GeneratesPInvoke` now accurate, the try-catch in `ReadyToRunCodegenCompilation.cs` (line 224-233) also becomes unnecessary and should be removed. - -**Three things to change per BrzVlad:** -1. Remove lines 120-121 in `Marshaller.ReadyToRun.cs` (`ShouldCheckForPendingException` inside `IsMarshallingRequired`) -2. Remove lines 722-723 in `ReadyToRunCompilationModuleGroupBase.cs` (`ShouldCheckForPendingException` in `GeneratesPInvoke`), reverting it back to its original `return !Marshaller.IsMarshallingRequired(method)` form - -**⚠️ Keep the try-catch in `ReadyToRunCodegenCompilation.cs`** — contrary to BrzVlad's implication, this safety net is still needed. While the ObjC non-blittable case no longer triggers it, `EmitIL()` can still throw for `LCIDConversionAttribute` (line 86-87 of PInvokeILEmitter.cs), which is NOT checked by `IsMarshallingRequired`. The original CorInfoImpl try-catch (removed by the PR) handled this. Since the PR keeps the CorInfoImpl simplification, the CreateValueFromKey try-catch serves as the replacement. Update its comment to reflect the broader purpose rather than being ObjC-specific. - -**What stays in the PR (the actual feature):** -- `ThrowPendingExceptionObject()` method in `ObjectiveCMarshal.CoreCLR.cs` ✅ -- The corelib.h binder entry ✅ -- The `ShouldCheckForPendingException` + emit call in `PInvokeILEmitter.EmitPInvokeCall()` ✅ -- The simplified `CorInfoImpl.ReadyToRun.cs` cleanup ✅ - -### Impact: Should fix — eliminates unnecessary complexity - -The current code is **functionally correct** (the try-catch catches failures), but it: -1. Introduces an unnecessary try-catch workaround -2. Causes wasted compilation attempts for non-blittable ObjC P/Invokes -3. Makes the code harder to reason about -4. BrzVlad's fix is simpler AND more correct - -**Severity: 🟡 Should fix** — the PR is cleaner and more correct with BrzVlad's approach. +**Severity: ✅ Fixed** — the original logic issue is no longer an open blocker. --- @@ -106,7 +56,7 @@ The current code is **functionally correct** (the try-catch catches failures), b ### Verdict: ✅ Valid concern, but not a blocker for this PR - In practice, `objc_msgSend` is almost always called with **blittable** params (IntPtr handles, selectors, primitives). This covers the vast majority of real MAUI/iOS interop. -- Non-blittable ObjC P/Invokes would still fall back to JIT/interpreter — **same behavior as before this PR**. No regression. +- After the force-push, non-blittable ObjC P/Invokes are cleanly excluded by actual marshalling requirements and still fall back to JIT/interpreter — **same behavior as before this PR**. No regression. - The `LibraryImport` source generator approach would handle non-blittable cases at compile time, but that's a separate effort. **Severity: 🟢 Low** — informational / future work, not a PR blocker. @@ -141,32 +91,59 @@ The current code is **functionally correct** (the try-catch catches failures), b ## Final Recommendations -| Priority | Action | Reviewer | -|----------|--------|----------| -| 🔴 **Must fix** | Add R2R map-validation test proving ObjC stubs are precompiled | jkoritzinsky | -| 🟡 **Should fix** | Per BrzVlad: remove `ShouldCheckForPendingException` from `IsMarshallingRequired` (Marshaller.ReadyToRun.cs:120-121) and revert `GeneratesPInvoke` to `return !IsMarshallingRequired(method)` | BrzVlad | -| 🟡 **Should fix** | Keep the try-catch in `ReadyToRunCodegenCompilation.cs` but update its comment — it's still needed for non-ObjC edge cases like `LCIDConversionAttribute` | — | -| 🟢 **Track separately** | Non-blittable ObjC P/Invoke support via `LibraryImport` | BrzVlad | +| Status | Priority | Action | Reviewer | +|--------|----------|--------|----------| +| ✅ Fixed | Formerly 🟡 Should fix | Remove `ShouldCheckForPendingException` from `Marshaller.IsMarshallingRequired` and revert `GeneratesPInvoke` to `return !IsMarshallingRequired(method)` | BrzVlad | +| ✅ Fixed | Formerly 🟡 Should fix | Drop the temporary `ReadyToRunCodegenCompilation.cs` / `CorInfoImpl.ReadyToRun.cs` workaround path and use `PInvokeILStubMethodIL.IsMarshallingRequired` instead | March 17 force-push | +| 🔴 Open | Must fix | Add an R2R `--map` validation test proving ObjC stubs are precompiled | jkoritzinsky | +| 🟡 Open | Should validate | Ensure non-void P/Invoke stubs preserve return values across `ThrowPendingExceptionObject()` | Copilot review | +| 🟡 Open | Should validate | Measure code-size impact and confirm P/Invoke frame correctness on the forced stub path | — | +| 🟢 Track | Alternative design | Evaluate a JIT-helper approach if it simplifies correctness or size concerns | jkoritzinsky / david | +| 🟢 Track separately | Future work | Non-blittable ObjC P/Invoke support via `LibraryImport` | BrzVlad | + +### What the PR looks like after the March 17 force-push + +**Implemented:** +- `ObjectiveCMarshal.ThrowPendingExceptionObject()` + `corelib.h` entry +- `PInvokeILEmitter.EmitPInvokeCall()` emitting the pending-exception check after native calls +- Removal of the old ObjC `NotSupportedException` blocker +- `ShouldCheckForPendingException` removed from `Marshaller.IsMarshallingRequired` +- `GeneratesPInvoke` reverted to `return !IsMarshallingRequired(method)` +- `PInvokeILStubMethodIL.IsMarshallingRequired` updated so blittable ObjC P/Invokes still stay on the stub path + +**Removed from the PR:** +- The `GeneratesPInvoke` escape hatch +- The try/catch in `ReadyToRunCodegenCompilation.cs` +- The `CorInfoImpl.ReadyToRun.cs` simplification +- The `IsObjCMessageSendPInvoke` helper + +**Still needed / still open:** +- R2R `--map` validation test confirming ObjC stubs are precompiled +- Validation of non-void return handling around `ThrowPendingExceptionObject()` +- Measurement of size impact and validation of P/Invoke frame correctness +- Possible JIT-helper alternative if it ends up being cleaner -### What the PR should look like after fixes +## Open Concerns After the March 17 Force-Push -**Keep (the actual feature):** -- `ObjectiveCMarshal.ThrowPendingExceptionObject()` + corelib.h entry -- `PInvokeILEmitter.EmitPInvokeCall()` emitting the exception check after native calls -- `CorInfoImpl.ReadyToRun.cs` simplification -- try-catch in `ReadyToRunCodegenCompilation.cs` (with updated comment — still needed for `LCIDConversionAttribute` and other edge cases) +1. **Code size / stub count impact** + - More ObjC signatures now intentionally go through a precompiled stub. + - That may be fine, but it should be measured explicitly. -**Remove (per BrzVlad):** -- `ShouldCheckForPendingException` from `IsMarshallingRequired` body -- `ShouldCheckForPendingException` rescue check from `GeneratesPInvoke` (revert to original form) +2. **P/Invoke frame correctness** + - The new design forces more ObjC calls down the stub path specifically so the pending-exception check always runs. + - That makes runtime transition correctness worth validating directly. -**Add (per jkoritzinsky):** -- R2R `--map` validation test confirming ObjC stubs are precompiled -- Test location: `src/tests/Interop/ObjectiveC/` or `src/tests/readytorun/tests/` +3. **Non-void return IL stack handling** + - `call native → call ThrowPendingExceptionObject → ret` is only obviously correct for `void` returns. + - Non-void returns likely need a temporary local or equivalent preservation step. + +4. **JIT helper alternative** + - A JIT helper may be cleaner than emitting the helper call directly in IL. + - David and jkoritzinsky exploring that option is reasonable. ### Bottom Line -The PR's **core approach is sound** — emitting `ThrowPendingExceptionObject()` after `objc_msgSend` calls mirrors the proven NativeAOT pattern. But the implementation has a logic layering issue (Concern 1) and zero test coverage (Concern 3). Both reviewers are correct in their concerns. +The force-pushed PR is materially better than the earlier version: it adopts Vlad's layering fix and adds the missing `PInvokeILStubMethodIL.IsMarshallingRequired` safeguard so blittable ObjC P/Invokes cannot bypass the stub. The remaining blockers are test coverage and correctness/perf validation, not the old escape-hatch design. --- @@ -177,6 +154,6 @@ A question was raised about whether switching the macios SDK from `[DllImport]` - `[LibraryImport]`'s source generator emits an inner `[DllImport]` with the same library name and entry point → `ShouldCheckForPendingException` still matches - The generator has no concept of `ObjectiveCMarshal.ThrowPendingExceptionObject()` and cannot emit the pending exception check - Switching would be a binary breaking change across hundreds of `objc_msgSend` overloads -- **After this PR's fix**, both `[DllImport]` and `[LibraryImport]` consumers benefit automatically — the fix is in the runtime, not in consumer code +- **After the March 17 force-pushed design**, both `[DllImport]` and `[LibraryImport]` consumers still benefit automatically — the fix remains in the runtime, not in consumer code See `pr-124770-analysis.md` for the full analysis. From d1ec12d543b1e6ab90eb67a792cec2ed2617d9e7 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 18 Mar 2026 10:51:08 -0500 Subject: [PATCH 07/10] Add R2R map validation test for ObjC P/Invoke stubs Adds a ReadyToRun test that validates objc_msgSend P/Invoke stubs are precompiled into the R2R image using crossgen2's --map output. The test: - Declares blittable objc_msgSend DllImport methods - Compiles them with crossgen2 --map - At runtime, reads the .map file and asserts P/Invoke stub entries exist - Only runs on macOS (CLRTestTargetUnsupported for non-Apple platforms) Addresses reviewer feedback from jkoritzinsky and AaronRobinsonMSFT requesting test coverage for the R2R ObjC P/Invoke precompilation feature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ObjCPInvokeR2R/ObjCPInvokeR2R.cs | 76 +++++++++++++++++++ .../ObjCPInvokeR2R/ObjCPInvokeR2R.csproj | 59 ++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs create mode 100644 src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj diff --git a/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs new file mode 100644 index 00000000000000..30092c5a6a83ee --- /dev/null +++ b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs @@ -0,0 +1,76 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Runtime.InteropServices; + +/// +/// Validates that blittable objc_msgSend P/Invoke stubs are precompiled +/// into the ReadyToRun image by checking the crossgen2 map file output. +/// +public static class ObjCPInvokeR2RTest +{ + // Blittable objc_msgSend declarations — these should be precompiled by R2R + [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend")] + private static extern IntPtr objc_msgSend(IntPtr receiver, IntPtr selector); + + [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend")] + private static extern IntPtr objc_msgSend_2(IntPtr receiver, IntPtr selector, IntPtr arg1); + + [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend_stret")] + private static extern void objc_msgSend_stret(IntPtr receiver, IntPtr selector); + + // This method references the P/Invoke declarations to ensure crossgen2 processes them + private static void EnsurePInvokesReferenced() + { + // These calls are never actually executed — they just ensure crossgen2 + // sees the P/Invoke declarations and attempts to generate stubs. + // The test validates stubs exist in the map, not runtime behavior. + if (Environment.GetEnvironmentVariable("NEVER_SET_THIS_VARIABLE_12345") != null) + { + objc_msgSend(IntPtr.Zero, IntPtr.Zero); + objc_msgSend_2(IntPtr.Zero, IntPtr.Zero, IntPtr.Zero); + objc_msgSend_stret(IntPtr.Zero, IntPtr.Zero); + } + } + + public static int Main() + { + EnsurePInvokesReferenced(); + + string assemblyPath = Assembly.GetExecutingAssembly().Location; + string mapFile = Path.ChangeExtension(assemblyPath, "map"); + + if (!File.Exists(mapFile)) + { + Console.WriteLine($"FAILED: Map file not found at {mapFile}"); + return 1; + } + + string[] mapLines = File.ReadAllLines(mapFile); + Console.WriteLine($"Map file has {mapLines.Length} lines"); + + // Search for objc_msgSend P/Invoke stub entries in the map + string[] objcLines = mapLines.Where(l => l.Contains("objc_msgSend")).ToArray(); + + Console.WriteLine($"Found {objcLines.Length} lines containing objc_msgSend:"); + foreach (string line in objcLines) + { + Console.WriteLine($" {line}"); + } + + bool foundMsgSend = objcLines.Any(l => l.Contains("objc_msgSend")); + if (!foundMsgSend) + { + Console.WriteLine("FAILED: objc_msgSend P/Invoke stub not found in R2R map."); + Console.WriteLine("This means the P/Invoke was not precompiled by crossgen2."); + return 1; + } + + Console.WriteLine("PASSED: objc_msgSend P/Invoke stubs found in R2R map."); + return 100; + } +} diff --git a/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj new file mode 100644 index 00000000000000..21567bfc71eb11 --- /dev/null +++ b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj @@ -0,0 +1,59 @@ + + + true + false + false + + true + + true + + true + + + + + + + + + From 76f43470e47915582c47ea5a39bca68a8dde6f45 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 18 Mar 2026 10:51:08 -0500 Subject: [PATCH 08/10] Add R2R map validation test for ObjC P/Invoke stubs Validates that blittable objc_msgSend P/Invoke stubs are precompiled into the ReadyToRun image by checking the crossgen2 --map output for MethodWithGCInfo entries. Key details: - Uses /usr/lib/libobjc.dylib (exact path matched by ShouldCheckForPendingException) - Checks specifically for MethodWithGCInfo entries (not just any map reference) - Requires --inputbubble so crossgen2 can resolve ThrowPendingExceptionObject tokens - macOS-only (CLRTestTargetUnsupported for non-OSX) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ObjCPInvokeR2R/ObjCPInvokeR2R.cs | 49 ++++++++++++++----- .../ObjCPInvokeR2R/ObjCPInvokeR2R.csproj | 2 +- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs index 30092c5a6a83ee..2bbbf29d9477c7 100644 --- a/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs +++ b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs @@ -13,14 +13,16 @@ /// public static class ObjCPInvokeR2RTest { - // Blittable objc_msgSend declarations — these should be precompiled by R2R - [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend")] + // Blittable objc_msgSend declarations — these should be precompiled by R2R. + // The module path must be "/usr/lib/libobjc.dylib" to match the ObjC detection + // in ShouldCheckForPendingException (MarshalHelpers.cs). + [DllImport("/usr/lib/libobjc.dylib", EntryPoint = "objc_msgSend")] private static extern IntPtr objc_msgSend(IntPtr receiver, IntPtr selector); - [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend")] + [DllImport("/usr/lib/libobjc.dylib", EntryPoint = "objc_msgSend")] private static extern IntPtr objc_msgSend_2(IntPtr receiver, IntPtr selector, IntPtr arg1); - [DllImport("libobjc.dylib", EntryPoint = "objc_msgSend_stret")] + [DllImport("/usr/lib/libobjc.dylib", EntryPoint = "objc_msgSend_stret")] private static extern void objc_msgSend_stret(IntPtr receiver, IntPtr selector); // This method references the P/Invoke declarations to ensure crossgen2 processes them @@ -53,24 +55,45 @@ public static int Main() string[] mapLines = File.ReadAllLines(mapFile); Console.WriteLine($"Map file has {mapLines.Length} lines"); - // Search for objc_msgSend P/Invoke stub entries in the map - string[] objcLines = mapLines.Where(l => l.Contains("objc_msgSend")).ToArray(); + // Search for objc_msgSend P/Invoke stubs that were actually compiled (MethodWithGCInfo). + // MethodFixupSignature entries are just metadata references that exist regardless + // of whether the stub was precompiled — only MethodWithGCInfo entries prove the + // P/Invoke IL stub was actually generated and compiled into the R2R image. + string[] compiledStubs = mapLines + .Where(l => l.Contains("objc_msgSend") && l.Contains("MethodWithGCInfo")) + .ToArray(); - Console.WriteLine($"Found {objcLines.Length} lines containing objc_msgSend:"); - foreach (string line in objcLines) + Console.WriteLine($"Found {compiledStubs.Length} compiled objc_msgSend stubs (MethodWithGCInfo):"); + foreach (string line in compiledStubs) { Console.WriteLine($" {line}"); } - bool foundMsgSend = objcLines.Any(l => l.Contains("objc_msgSend")); - if (!foundMsgSend) + // Verify all three P/Invoke stubs are precompiled + string[] expectedStubs = new[] { - Console.WriteLine("FAILED: objc_msgSend P/Invoke stub not found in R2R map."); - Console.WriteLine("This means the P/Invoke was not precompiled by crossgen2."); + "__objc_msgSend ", // 2-arg variant (trailing space to avoid partial match) + "__objc_msgSend_2 ", // 3-arg variant + "__objc_msgSend_stret " // stret variant + }; + + bool allFound = true; + foreach (string expected in expectedStubs) + { + bool found = compiledStubs.Any(l => l.Contains(expected)); + Console.WriteLine($" {(found ? "OK" : "MISSING")}: {expected.Trim()}"); + if (!found) + allFound = false; + } + + if (!allFound) + { + Console.WriteLine("FAILED: Not all objc_msgSend P/Invoke stubs were precompiled."); + Console.WriteLine("This means R2R did not generate IL stubs for ObjC P/Invokes."); return 1; } - Console.WriteLine("PASSED: objc_msgSend P/Invoke stubs found in R2R map."); + Console.WriteLine("PASSED: All objc_msgSend P/Invoke stubs found as compiled methods in R2R map."); return 100; } } diff --git a/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj index 21567bfc71eb11..b48d9df043ec5b 100644 --- a/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj +++ b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj @@ -39,7 +39,7 @@ then exit 1 fi -"$CORE_ROOT"/crossgen2/crossgen2 --map -r:"$CORE_ROOT"/*.dll -o:ObjCPInvokeR2R.dll IL_DLLS/ObjCPInvokeR2R.dll +"$CORE_ROOT"/crossgen2/crossgen2 --map --inputbubble -r:"$CORE_ROOT"/*.dll -o:ObjCPInvokeR2R.dll IL_DLLS/ObjCPInvokeR2R.dll __cgExitCode=$? if [ $__cgExitCode -ne 0 ] From 1716e404ef1f109056a8d6fefc2f1d704722debf Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 18 Mar 2026 14:24:44 -0500 Subject: [PATCH 09/10] Improve R2R test guidance and enforce negative verification - Promote regression test verification to a mandatory section in copilot-instructions.md with explicit guidance on using baseline build artifacts for negative verification - Add ReadyToRun/crossgen2 test gotchas section covering --inputbubble, map entry semantics, and MarshalHelpers.cs string matching - Add cross-component build warning (crossgen2 + CoreLib changes) - Add design doc discoverability instruction - Strengthen CoreLib rebuild guidance to explain libs.pretest necessity - Add Step 5 (mandatory negative verification) to jit-regression-test skill - Create src/tests/readytorun/README.md documenting test patterns, map validation rules, version bubble constraints, and platform gating Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/copilot-instructions.md | 30 +++++-- .github/skills/jit-regression-test/SKILL.md | 17 +++- src/tests/readytorun/README.md | 97 +++++++++++++++++++++ 3 files changed, 137 insertions(+), 7 deletions(-) create mode 100644 src/tests/readytorun/README.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d3c1221e713383..4355d406393b9b 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -14,6 +14,8 @@ When NOT running under CCA, skip the `code-review` skill if the user has stated Before making changes to a directory, search for `README.md` files in that directory and its parent directories up to the repository root. Read any you find — they contain conventions, patterns, and architectural context relevant to your work. +For specialized runtime subsystems, also check `docs/design/features/` for design documents that describe architectural constraints relevant to your work. + If the changes are intended to improve performance, or if they could negatively impact performance, use the `performance-benchmark` skill to validate the impact before completing. You MUST follow all code-formatting and naming conventions defined in [`.editorconfig`](/.editorconfig). @@ -121,7 +123,7 @@ Test projects are typically at: `tests/.Tests.csproj` or `tests/
  • .dll --- -## Adding new tests +## ⚠️ MANDATORY: Regression Test Verification + +**Before committing a regression test, you MUST confirm BOTH conditions:** +- the test fails against the unfixed code, and +- the test passes with the fix applied. + +The baseline build artifacts are the unfixed binaries for negative verification. Reuse that known-good pre-change build/output when proving the test fails without your fix; do not assume you need to reconstruct a separate "bad" build after editing code. + +⚠️ A first green run is **not** evidence that the test is valid. It may be passing because it never exercised the bug, because it ran against already-fixed binaries, or because it is only asserting existing behavior. When creating a regression test for a bug fix: -1. **Verify the test FAILS without the fix** — build and run against the unfixed code. -2. **Verify the test PASSES with the fix** — apply the fix, rebuild, and run again. -3. If the fix is not yet merged locally, manually apply the minimal changes from the PR/commit to verify. +1. **Verify the test FAILS without the fix** — run it against the baseline/unfixed build artifacts and confirm you are exercising the pre-fix binaries. +2. **Verify the test PASSES with the fix** — apply the fix, rebuild the affected components, and rerun the same test to confirm the behavior changed for the right reason. +3. **If the fix is not yet merged locally, manually apply the minimal changes from the PR/commit to verify** — do not skip negative verification just because the fix originated elsewhere. -Do not mark a regression test task as complete until both conditions are confirmed. +Do not mark a regression test task as complete until both conditions are explicitly confirmed. ## Troubleshooting @@ -221,6 +238,7 @@ Do not mark a regression test task as complete until both conditions are confirm |-------|----------| | "shared framework must be built" | Run baseline build: `./build.sh clr+libs -rc release` | | "testhost" missing / FileNotFoundException | Run baseline build first (Step 2 above) | +| crossgen2 + CoreLib changed | When a PR modifies both crossgen2 and CoreLib (e.g., adding new helper methods), do a full `./build.sh clr+libs` from the PR branch. Incremental crossgen2-only rebuilds will fail because the framework CoreLib won't have the new methods. | | Build timeout | Wait up to 40 min; only fail if no output for 5 min | | "Target does not exist" | Avoid specifying a target framework; the build will auto-select `$(NetCoreAppCurrent)` | | "0 test projects" after `build.sh -Test` | The test has `` > 0; add `-priority1` to the build command | diff --git a/.github/skills/jit-regression-test/SKILL.md b/.github/skills/jit-regression-test/SKILL.md index e6cc8f82d58c50..adc6582bbdbf0b 100644 --- a/.github/skills/jit-regression-test/SKILL.md +++ b/.github/skills/jit-regression-test/SKILL.md @@ -61,7 +61,7 @@ public class Runtime_ - **License header**: Always include the standard .NET Foundation license header - **Class name**: Match the file name exactly (`Runtime_`) -- **Test method**: `[Fact]` attribute, named `TestEntryPoint()` +- **Test method**: `[Fact]` attribute, named `TestEntryPoint()` - **Minimize the reproduction**: Strip to the minimal case that triggers the bug - **Use `[MethodImpl(MethodImplOptions.NoInlining)]`** when preventing inlining is needed to reproduce @@ -122,6 +122,21 @@ If a custom .csproj file is needed, it should be located next to the test source ``` +## Step 5: ⚠️ Verify Test Correctness (Mandatory) + +A regression test is only valid if it **fails without the fix** and **passes with the fix**. A test that passes in both cases is worthless — it does not actually test the bug. + +1. **Verify the test FAILS without the fix:** + - If you have a baseline build from `main` (before the fix), use those artifacts to run the test. + - The test should fail (non-zero exit code, assertion failure, or incorrect output). + - If the test passes without the fix, the test is wrong — revisit your assertions. + +2. **Verify the test PASSES with the fix:** + - Build and run with the fix applied. + - The test should pass (exit code 100 for CoreCLR tests, or all assertions green for xUnit tests). + +Do not consider the test complete until both conditions are confirmed. A first green run is not evidence the test is valid — it may be a false positive caused by incorrect assertions or test inputs that don't exercise the bug. + ## Tips - **No .csproj needed for simple tests** — register the `.cs` file in `Regression_ro_2.csproj` instead. diff --git a/src/tests/readytorun/README.md b/src/tests/readytorun/README.md new file mode 100644 index 00000000000000..2dff43d26075c9 --- /dev/null +++ b/src/tests/readytorun/README.md @@ -0,0 +1,97 @@ +# ReadyToRun Tests + +These tests validate ReadyToRun (R2R) behavior in the CoreCLR test suite, especially crossgen2 compilation correctness, cross-module compilation behavior, platform-specific code generation, and determinism. The directory contains both harness-driven tests and tests that invoke crossgen2 manually when they need more control over inputs or emitted artifacts. + +## Test Patterns + +### Pattern 1: Built-in Harness (preferred for simple tests) + +Use the built-in harness when a test only needs the standard ReadyToRun pipeline for a single assembly. + +- Enable it with `true`. +- Add extra crossgen2 switches when needed with ``. +- Examples: + - `crossgen2/crossgen2smoke.csproj` for the basic harness-driven pattern + - `HardwareIntrinsics/` for harness-driven tests with extra instruction-set flags + - `GenericCycleDetection/` for harness-driven tests with extra cycle-detection flags +- When to use: single-assembly compilation, standard validation, or tests that only need a few extra crossgen2 switches. + +This pattern is the simplest to maintain because the normal test infrastructure handles the crossgen2 invocation and test execution. For example, `GenericCycleDetection/*.csproj` enables `AlwaysUseCrossGen2` and adds cycle-detection options through `CrossGen2TestExtraArguments`, while `HardwareIntrinsics/X86/*.csproj` appends instruction-set flags the same way. + +### Pattern 2: Manual crossgen2 Invocation + +Use manual crossgen2 invocation when the built-in harness is not expressive enough. + +- Disable automatic crossgen with `false`. +- Run custom compilation steps with `CLRTestBashPreCommands` (and usually matching `CLRTestBatchPreCommands` for Windows). +- When to use: need `--map`, `--inputbubble`, `--opt-cross-module`, multi-step compilation, or platform-specific scripting. +- Examples: + - `tests/mainv1.csproj` + - `determinism/crossgen2determinism.csproj` + - `ObjCPInvokeR2R/ObjCPInvokeR2R.csproj` + +This pattern is common when a test must stage input assemblies, compile multiple outputs in a specific order, compare multiple generated binaries, or run platform-specific shell logic before execution. + +## Map File Validation + +When using `--map` to validate R2R compilation: + +- **`MethodWithGCInfo`** entries represent actual compiled native code in the ReadyToRun image. This is the signal to use when asserting that a method was precompiled. +- **`MethodFixupSignature`** entries are metadata or signature references. They do not prove that the method body was compiled. +- **`DelayLoadHelperImport`** entries are call-site fixups. They are also metadata-related and are not proof of compilation. + +Always check for `MethodWithGCInfo` when asserting that a method was precompiled into the R2R image. `ObjCPInvokeR2R/ObjCPInvokeR2R.cs` and `tests/test.cs` both use this rule when validating map output. + +## Version Bubble and `--inputbubble` + +R2R compilation outside CoreLib has cross-module reference limitations. The main background is documented in [R2R P/Invoke Design](../../../docs/design/features/readytorun-pinvoke.md). + +- P/Invoke stubs that call CoreLib helpers, such as Objective-C pending-exception helpers or `SetLastError` support, require `--inputbubble` so crossgen2 can create fixups for CoreLib methods. +- Without `--inputbubble`, crossgen2 can only reference members that the input assembly already references through existing `MemberRef` tokens in IL metadata. + +In practice, this is why tests like `ObjCPInvokeR2R` use manual crossgen2 invocation with `--inputbubble`. + +## Platform Gating + +Use MSBuild conditions to keep ReadyToRun tests targeted to the environments they actually validate. + +- Use `true` for platform-specific tests. +- Example: `ObjCPInvokeR2R.csproj` uses `Condition="'$(TargetsOSX)' != 'true'"` because `objc_msgSend` behavior is only relevant on Apple platforms. +- When sanitizers are enabled, crossgen2 tests may need `true` or `DisableProjectBuild` to avoid unsupported infrastructure combinations. + +Other tests in this directory also gate on `$(RuntimeFlavor)`, target architecture, or 32-bit limitations. + +## P/Invoke Detection Tests + +When testing platform-specific P/Invoke behavior, validate the exact detection logic used by the type system. + +- Check the exact library path and entrypoint constants in `src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalHelpers.cs`. +- Example: Objective-C detection matches `"/usr/lib/libobjc.dylib"` exactly. +- Using only `"libobjc.dylib"` in a `DllImport` will not trigger the Objective-C-specific code path. + +For ObjC-related tests, also verify the expected entrypoint name such as `objc_msgSend`. + +## Building and Running + +Tests are built and run as part of the CoreCLR test suite: + +```bash +# Build all R2R tests +src/tests/build.sh checked -tree:src/tests/readytorun + +# Generate Core_Root layout (required for manual runs) +src/tests/build.sh -GenerateLayoutOnly x64 Release + +# Run a single test manually +export CORE_ROOT=$(pwd)/artifacts/tests/coreclr/..Release/Tests/Core_Root +cd artifacts/tests/coreclr/..Debug/readytorun// +$CORE_ROOT/corerun .dll +# Exit code 100 = pass +``` + +Manual-invocation tests often expect the generated `.map` files and any staged IL assemblies to live beside the test output, so run them from the built test directory, not from the source tree. + +## Related Documentation + +- [R2R P/Invoke Design](../../../docs/design/features/readytorun-pinvoke.md) - version bubble constraints and marshalling pregeneration +- [R2R Composite Format](../../../docs/design/features/readytorun-composite-format-design.md) - composite image design From d747f894e07d30fde565f7e4f0c6c4640cad6216 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 19 Mar 2026 15:30:55 -0500 Subject: [PATCH 10/10] Add R2R domain rules to code-review skill Add P/Invoke marshalling and R2R-specific review rules: - inputbubble requirement for cross-module CoreLib references - IsMarshallingRequired prevents JIT from inlining past R2R stubs - Separation of marshalling vs platform-specific stub requirements - DllImport detection string matching against MarshalHelpers.cs - R2R map validation (MethodWithGCInfo vs MethodFixupSignature) - R2R+NativeAOT P/Invoke emitter alignment checks - R2R tests need both map validation and runtime verification Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/skills/code-review/SKILL.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 7542b697368f0b..ff7d53061fdf5b 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -577,6 +577,8 @@ Flag discrepancies with the following severity: - **Consider NativeAOT parity for runtime changes.** When changing CoreCLR behavior, verify whether the same change is needed for NativeAOT. > "The code you have changed is not used on NativeAOT. Do we need the same change for NativeAOT as well?" — jkotas +- **R2R and NativeAOT P/Invoke IL emission must stay aligned.** When adding new IL stub logic to the R2R `PInvokeILEmitter` (e.g., platform-specific exception checks, new marshalling helpers), verify the same logic exists in the shared `Common/TypeSystem/IL/Stubs/PInvokeILEmitter.cs` used by NativeAOT. Divergence between the two emitters causes subtle behavioral differences between R2R and NativeAOT compilation modes. + - **Keep interpreter behavior consistent with the regular JIT.** Follow the same patterns, naming, error codes (`CORJIT_BADCODE`), and macros (`NO_WAY`). Use `FEATURE_INTERPRETER` guards. > "Should we call it NO_WAY like in a regular JIT? I think the more similar the interpreter JIT to the regular JIT, the better." — jkotas @@ -629,6 +631,10 @@ Flag discrepancies with the following severity: - **Follow naming conventions for regression test directories.** In `src/tests/Regressions/coreclr/`, use `GitHub_` for the directory and `test` for the test name. > "Please follow the pre-existing pattern for naming. The directory name should be GitHub_122933 and the test name should be test122933." — jkotas +- **R2R map validation must check `MethodWithGCInfo`, not `MethodFixupSignature`.** When a ReadyToRun test asserts a method was precompiled into the R2R image, the assertion must check for `MethodWithGCInfo` entries in the crossgen2 `--map` output. `MethodFixupSignature` and `DelayLoadHelperImport` entries are metadata references that exist regardless of whether the method body was actually compiled to native code. See `src/tests/readytorun/README.md` for details. + +- **R2R tests that change P/Invoke compilation behavior need both map validation and runtime verification.** A map test proves stubs were precompiled; a runtime test proves they work correctly. For example, adding R2R support for a new P/Invoke category should include both a `--map` check (proving precompilation) and verification that the precompiled stub's behavior matches the JIT-compiled version (e.g., exception propagation, marshalling correctness). + --- ## Documentation & Comments @@ -727,3 +733,11 @@ Flag discrepancies with the following severity: - **Prefer 4-byte `BOOL` for native interop marshalling.** Use `UnmanagedType.Bool`. Verify P/Invoke return types match native signatures exactly—mismatches may work on 64-bit but fail on 32-bit/WASM. > "bool marshalling has always been bug prone area. The 4-byte bool (UnmanagedType.Bool) tends to be the least bug-prone option." — jkotas + +- **R2R P/Invoke stubs that call CoreLib helpers require `--inputbubble`.** When crossgen2 generates IL stubs that reference CoreLib methods (e.g., `ThrowPendingExceptionObject` for ObjC exception checks, `SetLastError` support), the input assembly must be compiled with `--inputbubble` so crossgen2 can create cross-module fixups. Without it, `ModuleTokenResolver.GetModuleTokenForMethod` throws `NotImplementedException` for methods the input assembly doesn't already reference. See `docs/design/features/readytorun-pinvoke.md`. + +- **Verify `IsMarshallingRequired` prevents JIT from inlining past R2R P/Invoke stubs.** When a P/Invoke stub contains safety logic (exception checks, GC transitions), `PInvokeILStubMethodIL.IsMarshallingRequired` must return `true`. The JIT checks `pInvokeMarshalingRequired` to decide whether to use the precompiled stub or inline a raw native call (`GTF_CALL_UNMANAGED`). If `IsMarshallingRequired` is `false` for a blittable P/Invoke, the JIT will inline the raw call and silently skip the stub's safety logic. + +- **Separate marshalling requirements from platform-specific stub requirements.** `Marshaller.IsMarshallingRequired` should only return `true` when actual data marshalling is needed (non-blittable parameters, `SetLastError`, non-`PreserveSig`). Platform-specific stub requirements — like ObjC pending exception checks — are not marshalling concerns and should not be injected into `Marshaller.IsMarshallingRequired`. Instead, set `PInvokeILStubMethodIL.IsMarshallingRequired` directly. Mixing these concerns creates fragile transitive coupling: if the marshalling check is later cleaned up (because the P/Invoke is blittable), the platform-specific JIT inlining protection silently breaks. Check whether the safety invariant depends on a check being in the right abstraction layer or merely happens to work via a side effect in a different layer. + +- **P/Invoke platform detection strings must match `MarshalHelpers.cs` exactly.** `ShouldCheckForPendingException` in `MarshalHelpers.cs` matches specific module paths (e.g., `"/usr/lib/libobjc.dylib"`) and entrypoint prefixes (e.g., `"objc_msgSend"`). Using an alias like `"libobjc.dylib"` in a `DllImport` attribute will bypass the detection entirely, causing the platform-specific code path to never trigger.