Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax argument checks on pinned GCHandles #68694

Merged
merged 8 commits into from
Apr 30, 2022
Merged

Relax argument checks on pinned GCHandles #68694

merged 8 commits into from
Apr 30, 2022

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Apr 29, 2022

Relax argument checks on pinned GCHandles to allow any objects without references for consistency with DisableRuntimeMarshalling.

Fixes #68686

Relax argument checks on pinned GCHandles to allow any objects without references for consistency with DisableRuntimeMarshalling.

Fixes dotnet#68686
@ghost
Copy link

ghost commented Apr 29, 2022

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Relax argument checks on pinned GCHandles to allow any objects without references for consistency with DisableRuntimeMarshalling.

Fixes #68686

Author: jkotas
Assignees: -
Labels:

area-System.Runtime.InteropServices

Milestone: -

@vargaz
Copy link
Contributor

vargaz commented Apr 29, 2022

The mono changes look ok to me.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems NativeAOT already has this similar definition and only checks for null or if the type has pointers. I think at this point there is really no argument against adding support in all cases.

/// <summary>
/// Hooks for System.Private.Interop.dll code to access internal functionality in System.Private.CoreLib.dll.
///
/// Methods added to InteropExtensions should also be added to the System.Private.CoreLib.InteropServices contract
/// in order to be accessible from System.Private.Interop.dll.
/// </summary>
[CLSCompliant(false)]
[ReflectionBlocked]
public static class InteropExtensions
{
internal static bool MightBeBlittable(this EETypePtr eeType)
{
//
// This is used as the approximate implementation of MethodTable::IsBlittable(). It will err in the direction of declaring
// things blittable since it is used for argument validation only.
//
return !eeType.HasPointers;
}

What about the following that mark char and bool as non-blittable? I think Crossgen or NativeAOT might trigger off of that in some manner.

case TypeFlags.Boolean:
switch (nativeType)
{
case NativeTypeKind.Default:
case NativeTypeKind.Boolean:
return MarshallerKind.Bool;
case NativeTypeKind.U1:
case NativeTypeKind.I1:
return MarshallerKind.CBool;
case NativeTypeKind.VariantBool:
if (context.Target.IsWindows)
return MarshallerKind.VariantBool;
else
return MarshallerKind.Invalid;
default:
return MarshallerKind.Invalid;
}
case TypeFlags.Char:
switch (nativeType)
{
case NativeTypeKind.I1:
case NativeTypeKind.U1:
return MarshallerKind.AnsiChar;
case NativeTypeKind.I2:
case NativeTypeKind.U2:
return MarshallerKind.UnicodeChar;
case NativeTypeKind.Default:
if (isAnsi)
return MarshallerKind.AnsiChar;
else
return MarshallerKind.UnicodeChar;
default:
return MarshallerKind.Invalid;
}

jkotas and others added 2 commits April 29, 2022 08:58
…ime.InteropServices.UnitTests/System/Runtime/InteropServices/GCHandleTests.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinnable if it doesn't have references?

…ices/Marshal.Mono.cs

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
@jkotas
Copy link
Member Author

jkotas commented Apr 29, 2022

What about the following that mark char and bool as non-blittable? I

I do not see a problem here. The marshaling without DisableRuntimeMarshaling needs work like it always did.

@jkotas jkotas merged commit fc303bb into dotnet:main Apr 30, 2022
@jkotas jkotas deleted the gchandle branch April 30, 2022 13:40
@ghost ghost locked as resolved and limited conversation to collaborators May 30, 2022
@AndyAyersMS
Copy link
Member

AndyAyersMS commented Jun 28, 2022

Improvements on windows arm64: dotnet/perf-autofiling-issues#5147, dotnet/perf-autofiling-issues#5148

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to pin arrays of unmanaged structs
5 participants