Relax argument checks on pinned GCHandles by jkotas · Pull Request #68694 · dotnet/runtime

jkotas · 2022-04-29T02:56:42Z

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 · 2022-04-29T02:56:48Z

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 · 2022-04-29T13:05:59Z

The mono changes look ok to me.

src/mono/mono/metadata/marshal.c

AaronRobinsonMSFT

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.

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/InteropExtensions.cs

Lines 15 to 32 in 8656518

    
           /// <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.

runtime/src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalHelpers.cs

Lines 273 to 312 in 8656518

    
           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; 
        
               }

src/coreclr/vm/marshalnative.cpp

...sts/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/GCHandleTests.cs

…ime.InteropServices.UnitTests/System/Runtime/InteropServices/GCHandleTests.cs Co-authored-by: Aaron Robinson <arobins@microsoft.com>

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

lambdageek

Pinnable if it doesn't have references?

src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs

…ices/Marshal.Mono.cs Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

jkotas · 2022-04-29T16:15:11Z

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.

AndyAyersMS · 2022-06-28T18:28:15Z

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

Relax argument checks on pinned GCHandles

ee1212f

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

ghost added the area-System.Runtime.InteropServices label Apr 29, 2022

ghost assigned jkotas Apr 29, 2022

Add test

f516f2f

jkotas requested review from lambdageek, thaystg and vargaz as code owners April 29, 2022 06:17

Fix Mono

46bc810

jkotas force-pushed the gchandle branch from d51e554 to 46bc810 Compare April 29, 2022 06:18

lambdageek reviewed Apr 29, 2022

View reviewed changes

src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Apr 29, 2022

View reviewed changes

src/coreclr/vm/marshalnative.cpp Show resolved Hide resolved

...sts/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/GCHandleTests.cs Outdated Show resolved Hide resolved

This was referenced Apr 29, 2022

jit.1 work item failing on mono #67888

Closed

Test failure JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-5/regex-redux-5.sh #66625

Closed

CR feedback

de011ac

jkotas requested review from MichalStrehovsky and marek-safar as code owners April 29, 2022 15:58

jkotas and others added 2 commits April 29, 2022 08:58

Update src/libraries/System.Runtime.InteropServices/tests/System.Runt…

c3492fc

…ime.InteropServices.UnitTests/System/Runtime/InteropServices/GCHandleTests.cs Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Update src/coreclr/vm/marshalnative.cpp

4bf5514

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

lambdageek reviewed Apr 29, 2022

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs Outdated Show resolved Hide resolved

Update src/mono/System.Private.CoreLib/src/System/Runtime/InteropServ…

07a848f

…ices/Marshal.Mono.cs Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

More tests

78c3412

AaronRobinsonMSFT approved these changes Apr 29, 2022

View reviewed changes

jkotas merged commit fc303bb into dotnet:main Apr 30, 2022

jkotas deleted the gchandle branch April 30, 2022 13:40

ghost locked as resolved and limited conversation to collaborators May 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax argument checks on pinned GCHandles#68694

Relax argument checks on pinned GCHandles#68694
jkotas merged 8 commits intodotnet:mainfrom
jkotas:gchandle

jkotas commented Apr 29, 2022

Uh oh!

ghost commented Apr 29, 2022

Uh oh!

vargaz commented Apr 29, 2022

Uh oh!

Uh oh!

AaronRobinsonMSFT left a comment

Uh oh!

Uh oh!

Uh oh!

lambdageek left a comment

Uh oh!

Uh oh!

jkotas commented Apr 29, 2022

Uh oh!

AndyAyersMS commented Jun 28, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	/// <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;
	}

	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;
	}

Conversation

jkotas commented Apr 29, 2022

Uh oh!

ghost commented Apr 29, 2022

Uh oh!

vargaz commented Apr 29, 2022

Uh oh!

Uh oh!

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lambdageek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotas commented Apr 29, 2022

Uh oh!

AndyAyersMS commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AndyAyersMS commented Jun 28, 2022 •

edited

Loading