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/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. 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/pr-124770-analysis.md b/pr-124770-analysis.md new file mode 100644 index 00000000000000..a999424d02b394 --- /dev/null +++ b/pr-124770-analysis.md @@ -0,0 +1,274 @@ +# 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 +- **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 + +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. 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()`. + +## 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, 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` +- **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` +- **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` +- **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) + +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. + +### 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) + +**"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 + +### Update (March 17): what is already implemented vs what is still open + +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. **✅ 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. **✅ 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` + +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. + +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. + +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. + +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. + +--- + +## 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. 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. + +### 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 Force-Pushed Design + +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 + +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` + +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. + +### 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 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` + +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 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. + +--- + +### 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 new file mode 100644 index 00000000000000..d0a53ae0191dc3 --- /dev/null +++ b/pr-124770-plan.md @@ -0,0 +1,159 @@ +# PR #124770 — Final Review Assessment + +## Summary + +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 + +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's core concern was correct, and the force-push implemented it + +**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?" + +That conflation is what created the old `GeneratesPInvoke` escape hatch and the temporary `ReadyToRunCodegenCompilation.cs` try/catch workaround. + +### Update (March 17): implemented in the force-push + +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` + +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. + +### Current assessment + +- 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. + +**Severity: ✅ Fixed** — the original logic issue is no longer an open blocker. + +--- + +## 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. +- 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. + +--- + +## 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 + +| 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 + +## Open Concerns After the March 17 Force-Push + +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. + +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. + +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 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. + +--- + +## 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 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. 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/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs index ed05deb5f23581..58d71b9cc7d128 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(); @@ -142,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++) { 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 diff --git a/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs new file mode 100644 index 00000000000000..2bbbf29d9477c7 --- /dev/null +++ b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.cs @@ -0,0 +1,99 @@ +// 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. + // 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("/usr/lib/libobjc.dylib", EntryPoint = "objc_msgSend")] + private static extern IntPtr objc_msgSend_2(IntPtr receiver, IntPtr selector, IntPtr arg1); + + [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 + 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 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 {compiledStubs.Length} compiled objc_msgSend stubs (MethodWithGCInfo):"); + foreach (string line in compiledStubs) + { + Console.WriteLine($" {line}"); + } + + // Verify all three P/Invoke stubs are precompiled + string[] expectedStubs = new[] + { + "__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: 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 new file mode 100644 index 00000000000000..b48d9df043ec5b --- /dev/null +++ b/src/tests/readytorun/ObjCPInvokeR2R/ObjCPInvokeR2R.csproj @@ -0,0 +1,59 @@ + + + true + false + false + + true + + true + + true + + + + + + + + + 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