Fix NativeAOT reflection invoke copyback#130065
Conversation
Align NativeAOT reflection invoke argument copy-back with CoreCLR for byref, Type.Missing, and custom binder conversions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
There was a problem hiding this comment.
Pull request overview
This PR updates NativeAOT reflection invocation to correctly copy back argument values when a custom binder performs conversions, and re-enables previously skipped reflection tests for NativeAOT.
Changes:
- Track binder-driven conversions and copy converted argument values back into the caller-provided argument array.
- Change NativeAOT
DynamicInvokeInfocopy-back logic to be per-argument (viashouldCopyBack) instead of a single_needsCopyBackflag. - Remove
ActiveIssueskips so the affected System.Reflection tests run on NativeAOT again.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Reflection.Tests/MethodInfoTests.cs | Re-enables a binder conversion Invoke test on NativeAOT. |
| src/libraries/System.Runtime/tests/System.Reflection.Tests/ConstructorInfoTests.cs | Re-enables binder conversion Invoke tests on NativeAOT. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/MethodInfos/CustomMethodInvoker.cs | Adds binder conversion tracking and copies back converted args. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs | Adds per-argument copy-back tracking (byref / Missing / binder conversions) and adjusts copy-back timing. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs | Adds out bool copyBack plumbing for binder-based conversions and factors assignability checks. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeAugments.cs | Exposes the new CheckArgument(..., out bool copyBack) overload for invoker use. |
| srcObject = binderBundle.ChangeType(srcObject, exactDstType); | ||
| copyBack = srcObject is not null && IsArgumentAssignable(srcObject, dstEEType); | ||
|
|
||
| // For compat with desktop, the result of the binder call gets processed through the default rules again. | ||
| return CheckArgument(srcObject, dstEEType, semantics, binderBundle: null); |
|
Huh, so the TestLinqExpressions test I added in dotnet/corert#8072 is failing... But it also fails on CoreCLR. It does not fail on .NET Framework. Delete the test and file a bug? Or do we care at all? |
|
.NET Framework does not the copy back under finally in reflection invoke https://github.com/microsoft/referencesource/blob/ec9fa9ae770d522a5b5f0607898044b7478574a3/mscorlib/system/reflection/methodinfo.cs#L761-L765 Is this actually a difference between System.Linq.Expression interpreter vs. compiler? I do not think we care about that. System.Linq.Expression is effectively archived. |
Fixes #67531.
Better to review with whitespace diffs off. We were not copying back results of binder conversions.
This also aligns reflection invoke argument copy-back with CoreCLR for byref and Type.Missing.
I was scratching my head over why we do the copyback in a
finally(CoreCLR demonstratably doesn't and neither does .NET Framework), I guess CI will tell me if I missed something. So this aligns the copyback to also not be underfinally.