Add GCHandle<T> #111307

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

IEquatable<T> and equality operators are included.

dotnet-issue-labeler · 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.

dotnet-issue-labeler · 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.

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

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

+        public void Dispose()
+        {
+            // Free the handle if it hasn't already been freed.
+            IntPtr handle = Interlocked.Exchange(ref _handle, IntPtr.Zero);
+            if (handle != IntPtr.Zero)
+            {
+                GCHandle.InternalFree(handle);
+            }
+        }


Different from GCHandle.Free, the Dispose methods are not throwing. This matches the contract of IDisposable to allow duplicated dispose calls.

huoyaoyuan · 2025-01-11T13:22:12Z

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

+        [CLSCompliant(false)]
+        public static unsafe T* GetAddressOfArrayData<T>(this PinnedGCHandle<T[]?> handle)
+            => (T*)handle.GetAddressOfObjectData();
+
+        /// <summary>
+        /// Retrieves the address of string data in a <see cref="PinnedGCHandle{T}"/> of <see cref="string"/>.
+        /// </summary>
+        /// <returns>The address of the pinned <see cref="string"/>.</returns>
+        [CLSCompliant(false)]
+        public static unsafe char* GetAddressOfStringData(this PinnedGCHandle<string?> handle)
+            => (char*)handle.GetAddressOfObjectData();


Should these be nullable oblivious? Given there's no covariance, annotating either with nullable or non-nullable will give warning for the handles annotated with the opposite.

We suggested making these nullable during API review under the assumption that that would allow both without warnings. If that's not actually the case (as in, calling this with a non nullable T still produces a warning), making these nullable oblivious makes sense to me 🤔

It will produce warning for non-nullable T. PinnedGCHandle<string> can't be converted to PinnedGCHandle<string?> because Set(null) will be warned for the former.

Marking nullable oblivious will allow cause oblivious of T?* and T* when T is managed type. I don't think it's a big deal since pointers to managed type is more unsafe and uncommon. Instead top level nullability will bother people more.

What about only making the parameter nullable oblivious? Would that be doable?
As in:

public static unsafe T* GetAddressOfArrayData<T>( #nullable disable this PinnedGCHandle<T[]> handle) #nullable restore => (T*)handle.GetAddressOfObjectData();

So we can preserve the nullability annotation on the returned pointer?

While I'm not seeing any behavioral difference for type inference and nullability warning, the compiled metadata are different. Agreed that nullable oblivious range should be as short as possible.

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

AaronRobinsonMSFT · 2025-01-11T16:19:33Z

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

+    /// <summary>
+    /// Provides extension methods to operate with GC handles.
+    /// </summary>
+    public static class GCHandleExtensions


Why are these extension methods? They are defined in the same namespace in the same assembly. I don't understand the utility for these being extensions methods in this case as opposed to member/static functions on the actual type.

I imagine it's so that they can look like instance members, but only apply to specific Ts (currently a T[] or string).

Yes, it's used effectively as generic specialization. Of course with proper generic specialization we can avoid dealing with nullability of generic parameter of this.

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.

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.

Sergio0694 · 2025-01-11T20:34:54Z

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

+        {
+            get
+            {
+                GCHandle.CheckUninitialized(_handle);


How come this check is here, and also why are we using Unsafe.As below? I thought we said this entire get accessor could just be get => *(T*)_handle while preserving the same implicit semantics?

Place CheckUninitialized under #if MONO || DEBUG ifdef here?

Under release, the two read operations (against the same copy of value) will be combined by JIT:

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4axAMwNuubADMYDXBigcwGBgAlsXACbSaAbxoN3THQEkeABTMMAPoAFnaOMBrUHgxuHtpM5IwA4jAYABQAlAwAvAB8DCkYADzMdLmZkdGx7vEAKgWpRbXlWQDuwTLG9SBMpNUMrlHRHgrYUAyhDtI5QZPhlcPuQh1gANZpc9IZC4vEAOwMAKpc+kYsAIK4TeUAVGkQwABWMJY3GZsw2/0Avv39YlBeKMMMZmEgmCgGMsXusfBh/OMPhl+oNFu5AjM7g9nq93mFpDsGL9qN8gA

Coreclr provides additional check for Debug build and I'd like it to be preserved. For Release there won't be duplicated read.

Using get => *(T*)_handle would require to define another series of entries for Debug and Mono. Using Unsafe.As allows reusing the InternalGet methods with zero overhead.

I don't see the problem for that. GCHandle also has a couple ifdefs for release, and a different impl for Mono.

I think it's unnecessary to introduce extra complexity when the duplicated read can go away in Release.

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

Sergio0694 · 2025-01-12T15:59:57Z

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

+        /// <summary>
+        /// Releases this <see cref="GCHandle{T}"/>.
+        /// </summary>
+        public void Dispose()


Can't we just use /// <inheritdoc/> on all these implemented interface methods, rather than duplicating all the XML docs from those APIs manually? Applies to all implemented interface methods, overridden methods, in all 3 types in the PR.

I'm not sure whether it works fine under the documentation requirements. For some members like operators there's no source to inherit (expect the IEqualityOperator ones, which isn't implemented by these types). For some members like Dispose the documentation lives in api-docs but not this repo, and I'm not sure how tooling will process the <inheritdoc/> tag. Some docs are "templated" and filled with type names.

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

dotnet-issue-labeler bot 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

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

MichalPetryka reviewed Jan 11, 2025

View reviewed changes

Avoid extra overhead of null check

c25bf1b

Reduce overhead of GetAddressOfArrayData and GetAddressOfStringData

8dee06a

Sergio0694 reviewed Jan 11, 2025

View reviewed changes

jkotas reviewed Jan 12, 2025

View reviewed changes

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

huoyaoyuan added 3 commits January 12, 2025 13:56

Update null check

e73f07c

Reduce reads from Dispose

433047e

Add equality

47b5017

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

[WASI] Sockets - unknown handle index #108726

Open

Sergio0694 reviewed Jan 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GCHandle<T> #111307

Add GCHandle<T> #111307

huoyaoyuan commented Jan 11, 2025 •

edited

Loading

dotnet-issue-labeler bot commented Jan 11, 2025

dotnet-issue-labeler bot commented Jan 11, 2025

huoyaoyuan Jan 11, 2025

huoyaoyuan Jan 11, 2025

Sergio0694 Jan 11, 2025

huoyaoyuan Jan 11, 2025

huoyaoyuan Jan 11, 2025

Sergio0694 Jan 11, 2025 •

edited

Loading

huoyaoyuan Jan 11, 2025

Sergio0694 left a comment

Sergio0694 Jan 11, 2025

huoyaoyuan Jan 11, 2025

Sergio0694 Jan 11, 2025

huoyaoyuan Jan 11, 2025

Sergio0694 Jan 11, 2025

huoyaoyuan Jan 11, 2025

Sergio0694 Jan 11, 2025

AaronRobinsonMSFT Jan 11, 2025

MihaZupan Jan 11, 2025

huoyaoyuan Jan 11, 2025 •

edited

Loading

MichalPetryka Jan 11, 2025

huoyaoyuan Jan 12, 2025

MichalPetryka Jan 11, 2025

huoyaoyuan Jan 11, 2025

Sergio0694 Jan 11, 2025

jkotas Jan 12, 2025

huoyaoyuan Jan 12, 2025

huoyaoyuan Jan 12, 2025

Sergio0694 Jan 12, 2025

huoyaoyuan Jan 12, 2025 •

edited

Loading

Sergio0694 Jan 12, 2025

huoyaoyuan Jan 12, 2025

Add GCHandle<T> #111307

Are you sure you want to change the base?

Add GCHandle<T> #111307

Conversation

huoyaoyuan commented Jan 11, 2025 • edited Loading

dotnet-issue-labeler bot commented Jan 11, 2025

dotnet-issue-labeler bot commented Jan 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huoyaoyuan Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huoyaoyuan Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huoyaoyuan commented Jan 11, 2025 •

edited

Loading

Sergio0694 Jan 11, 2025 •

edited

Loading

huoyaoyuan Jan 11, 2025 •

edited

Loading

huoyaoyuan Jan 12, 2025 •

edited

Loading