Add GCHandle<T> by huoyaoyuan · Pull Request #111307 · dotnet/runtime

huoyaoyuan · 2025-01-11T13:19:20Z

Closes #94134. IEquatable<T> included.

ghost · 2025-01-11T13:19:26Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2025-01-11T13:19:27Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandleExtensions.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

Sergio0694

Left a few notes based on previous discussions 🙂
Thank you for taking this one!

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PinnedGCHandle.T.cs

Sergio0694 · 2025-01-11T13:58:36Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

+            get
+            {
+                IntPtr handle = _handle;
+                GCHandle.ThrowIfInvalid(_handle);


All these checks should be removed. Using any instance method on an instance that's not allocated is explicitly not supported. As long as we can throw an exception (eg. this should throw NRE) so the behavior is still consistent, that's sufficient.

As long as we can throw an exception (eg. this should throw NRE)

This is the questionable part - the current implementation uses FCall in some cases, and the ability to throw exception from FCall will soon be removed. In other words, no NRE can be thrown, at least for some members.

With that said, providing invalid value via FromIntPtr will still cause hard AV in unmanaged code.

At least for the getter, on CoreCLR and NAOT this should just be *(T*)_handle, which should already throw NRE implicitly.

Maybe an implicit read (_ = *(byte*)value) can be applied with lowest overhead? I'm not worried about garbage value from FromIntPtr, but make new GCHandle<T>{ Target = obj } crashing doesn't look good.

What do you mean by implicit read? You need to read anyway, since you're returning a value. What I'm saying is the whole getter should literally just be get => *(T*)_handle. That will automatically NRE if handle is null, which is all we need.

I mean adding a read in managed code in setter. Currently the entire setter is executed in unmanaged code and will cause AV, which won't be caught as NRE.

Oh I see what you mean, I thought you were talking about the other accessor. That would seem fine to me, but I'll defer to Jan and others to confirm 🙂

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandleExtensions.cs

MichalPetryka · 2025-01-11T17:40:03Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PinnedGCHandle.T.cs

+            IntPtr handle = _handle;
+            GCHandle.ThrowIfInvalid(handle);
+
+            return GCHandle.AddrOfPinnedObjectFromHandle(_handle);


Is this expected to follow GCHandle in special casing strings and arrays here?

Memory<T>.Pin and fixed are all following the contract to return the address of first element. Not following the contract would require strong justification.

This method should be only used for non-array non-string blittable types. Memory<T>.Pin and fixed do not work for non-array non-string blittable types.

I think it would make sense to make it work just for the non-array non-string blittable types and document the behavior as undefined to avoid unnecessary overhead. The overall goal with these new GCHandle types was to eliminate unnecessary overheads.

The original intention with this method was to provide a replacement for GCHandle.AddrOfPinnedObject. I think the method has different enough name to have different behavior.

MichalPetryka · 2025-01-11T17:40:51Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandleExtensions.cs

+#nullable disable // Nullable oblivious because no covariance between PinnedGCHandle<T> and PinnedGCHandle<T?>
+            this PinnedGCHandle<T[]> handle)
+#nullable restore
+            => (T*)handle.GetAddressOfObjectData();


Should those be implemented separately here to avoid runtime checks GCHandle does for performance?

Good point. Yes the check can't be eliminated here.

stephentoub · 2025-01-13T21:11:21Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

+        /// <param name="target">The object that uses the <see cref="GCHandle{T}"/>.</param>
+        public GCHandle(T target)
+        {
+            _handle = GCHandle.InternalAlloc(target, GCHandleType.Normal);


Since we rely on IntPtr.Zero elsewhere in the implementation having special meaning, worth asserting here that _handle != IntPtr.Zero?

Should the assert go to InternalAlloc since there are more types depending on this?

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandleExtensions.cs

src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PinnedGCHandle.T.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/WeakGCHandle.T.cs

…pServices/PinnedGCHandle.T.cs Co-authored-by: Aaron Robinson <arobins@microsoft.com>

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CryptoStream.cs

huoyaoyuan · 2025-01-19T08:44:58Z

Any further review on this?
I'm going to OOF for holidays starting from next Wednesday. Please feel free to directly update the branch if you need to drive this.

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs

jkotas

LGTM. Thank you!

huoyaoyuan added 11 commits January 11, 2025 19:02

Add GCHandle<T>

4912b81

Implementation and documentation

34667f6

PinnedGCHandle

0b92685

Add missing setters

b533663

WeakGCHandle

a443880

GCHandleExtensions

37c22a5

Reference source

7e9dcab

Unit test

daa2d71

Only allows pinnable object

3d6c6cc

Fix unit tests

6e3a26e

Don't change file ending

52a892e

ghost added area-System.Runtime.InteropServices new-api-needs-documentation labels Jan 11, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 11, 2025

huoyaoyuan commented Jan 11, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandleExtensions.cs Outdated Show resolved Hide resolved

MihaZupan reviewed Jan 11, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs Outdated Show resolved Hide resolved

Maximize unsafety

cd45974

Sergio0694 suggested changes Jan 11, 2025

View reviewed changes

MihaZupan reviewed Jan 11, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs Outdated Show resolved Hide resolved

MihaZupan added this to the 10.0.0 milestone Jan 11, 2025

huoyaoyuan added 5 commits January 11, 2025 22:10

Adjust wording

06f7298

Remove interlock

af5be56

Allow pinning any object

948b665

Nullable oblivious

4c82bfe

Reduce nullable oblivious range

80a8580

AaronRobinsonMSFT reviewed Jan 11, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandleExtensions.cs Show resolved Hide resolved

MichalPetryka reviewed Jan 11, 2025

View reviewed changes

Avoid extra overhead of null check

c25bf1b

stephentoub reviewed Jan 13, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs Show resolved Hide resolved

stephentoub reviewed Jan 13, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandleExtensions.cs Show resolved Hide resolved

huoyaoyuan added 3 commits January 14, 2025 10:50

Fix constraint

cf45bcd

Fix xmldoc

3598aa0

Fix build

1337d04

build-analysis bot mentioned this pull request Jan 14, 2025

Test failure: baseservices/exceptions/stackoverflow/stackoverflowtester/stackoverflowtester.cmd #110173

Open

jkotas reviewed Jan 14, 2025

View reviewed changes

huoyaoyuan and others added 5 commits January 15, 2025 19:38

Merge branch 'main' into gchandle

43859e2

Fix unintended change

6c80e1b

Revert CryptoStream

c8a9497

Update xmldoc

f8d0158

Update src/libraries/System.Private.CoreLib/src/System/Runtime/Intero…

42c6362

…pServices/PinnedGCHandle.T.cs Co-authored-by: Aaron Robinson <arobins@microsoft.com>

AaronRobinsonMSFT reviewed Jan 16, 2025

View reviewed changes

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CryptoStream.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Jan 16, 2025

View reviewed changes

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CryptoStream.cs Show resolved Hide resolved

Sergio0694 reviewed Feb 9, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs Show resolved Hide resolved

Merge branch 'main' into gchandle

d7800bf

jkotas approved these changes Feb 13, 2025

View reviewed changes

AaronRobinsonMSFT approved these changes Feb 13, 2025

View reviewed changes

jkotas merged commit 7b5f7f0 into dotnet:main Feb 13, 2025
143 of 149 checks passed

huoyaoyuan deleted the gchandle branch February 13, 2025 02:47

github-actions bot locked and limited conversation to collaborators Mar 15, 2025

Conversation

huoyaoyuan commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 11, 2025

Uh oh!

ghost commented Jan 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sergio0694 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huoyaoyuan commented Jan 19, 2025

Uh oh!

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

huoyaoyuan commented Jan 11, 2025 •

edited

Loading