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

Add GCHandle<T> #111307

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,8 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ExternalException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\FieldOffsetAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\GCHandle.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\GCHandle.T.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\GCHandleExtensions.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\GCHandleType.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\GuidAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\HandleRef.cs" />
Expand Down Expand Up @@ -1014,6 +1016,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\OptionalAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\OSPlatform.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\OutAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\PinnedGCHandle.T.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\PosixSignal.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\PosixSignalContext.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\PosixSignalRegistration.cs" />
Expand All @@ -1038,6 +1041,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\VarEnum.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\VariantWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\WasmImportLinkageAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\WeakGCHandle.T.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\Arm\Enums.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\ISimdVector_2.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\Scalar.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Threading;

namespace System.Runtime.InteropServices
{
/// <summary>
/// Represents an strongly-typed opaque, GC handle to a managed object.
/// A GC handle is used when an object reference must be reachable from
/// unmanaged memory.
/// </summary>
/// <remarks>
/// <see cref="GCHandle{T}"/> corresponds to Normal roots.
/// For Weak and WeakTrackResurrection, see <see cref="WeakGCHandle{T}"/>.
/// For Pinned, see <see cref="PinnedGCHandle{T}"/>.
/// </remarks>
/// <seealso cref="GCHandle" />
/// <typeparam name="T">The type of the object this <see cref="GCHandle{T}"/> tracks to.</typeparam>
public struct GCHandle<T> : IDisposable
where T : class?
{
// The actual integer handle value that the EE uses internally.
private IntPtr _handle;

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

private GCHandle(IntPtr handle) => _handle = handle;

/// <summary>Determine whether this handle has been allocated or not.</summary>
public readonly bool IsAllocated => _handle != IntPtr.Zero;

/// <summary>Gets or sets the object this handle represents.</summary>
public readonly T Target
{
get
{
IntPtr handle = _handle;
GCHandle.ThrowIfInvalid(_handle);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙂


// Skip the type check to provide maximum unsafety.
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved
return Unsafe.As<T>(GCHandle.InternalGet(handle));
}
set
{
IntPtr handle = _handle;
GCHandle.ThrowIfInvalid(handle);

GCHandle.InternalSet(handle, value);
}
}

/// <summary>
/// Returns a new <see cref="GCHandle{T}"/> object created from a handle to a managed object.
/// </summary>
/// <param name="value">An <see cref="IntPtr"/> handle to a managed object to create a <see cref="GCHandle{T}"/> object from.</param>
/// <returns>A new <see cref="GCHandle{T}"/> object that corresponds to the value parameter.</returns>
/// <remarks>
/// The <see cref="IntPtr"/> representation of <see cref="GCHandle{T}"/> is not
/// interchangable with <see cref="GCHandle"/>.
/// </remarks>
public static GCHandle<T> FromIntPtr(IntPtr value)
{
GCHandle.ThrowIfInvalid(value);
return new GCHandle<T>(value);
}

/// <summary>
/// Returns the internal integer representation of a <see cref="GCHandle{T}"/> object.
/// </summary>
/// <param name="value">A <see cref="GCHandle{T}"/> object to retrieve an internal integer representation from.</param>
/// <returns>An <see cref="IntPtr"/> object that represents a <see cref="GCHandle{T}"/> object.</returns>
/// <remarks>
/// The <see cref="IntPtr"/> representation of <see cref="GCHandle{T}"/> is not
/// interchangable with <see cref="GCHandle"/>.
/// </remarks>
public static IntPtr ToIntPtr(GCHandle<T> value) => value._handle;

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

{
// Free the handle if it hasn't already been freed.
IntPtr handle = Interlocked.Exchange(ref _handle, IntPtr.Zero);
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved
if (handle != IntPtr.Zero)
{
GCHandle.InternalFree(handle);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace System.Runtime.InteropServices
/// WeakTrackResurrection: Same as Weak, but stays until after object is really gone.
/// Pinned - same as Normal, but allows the address of the actual object to be taken.
/// </remarks>
/// <seealso cref="GCHandle{T}"/>
[StructLayout(LayoutKind.Sequential)]
public partial struct GCHandle : IEquatable<GCHandle>
{
Expand All @@ -36,9 +37,9 @@ private GCHandle(object? value, GCHandleType type)
throw new ArgumentOutOfRangeException(nameof(type), SR.ArgumentOutOfRange_Enum);
}

if (type == GCHandleType.Pinned && !Marshal.IsPinnable(value))
if (type == GCHandleType.Pinned)
{
throw new ArgumentException(SR.ArgumentException_NotIsomorphic, nameof(value));
ThrowIfNotPinnable(value);
}

IntPtr handle = InternalAlloc(value, type);
Expand Down Expand Up @@ -92,9 +93,9 @@ readonly get
IntPtr handle = _handle;
ThrowIfInvalid(handle);

if (IsPinned(handle) && !Marshal.IsPinnable(value))
if (IsPinned(handle))
{
throw new ArgumentException(SR.ArgumentException_NotIsomorphic, nameof(value));
ThrowIfNotPinnable(value);
}

InternalSet(GetHandleValue(handle), value);
Expand All @@ -118,11 +119,18 @@ public readonly IntPtr AddrOfPinnedObject()
}

// Get the address.
unsafe
{
return (IntPtr)AddrOfPinnedObjectFromHandle(GetHandleValue(handle));
}
}

object? target = InternalGet(GetHandleValue(handle));
internal static unsafe void* AddrOfPinnedObjectFromHandle(IntPtr handle)
{
object? target = InternalGet(handle);
if (target is null)
{
return default;
return null;
}

unsafe
Expand All @@ -132,14 +140,14 @@ public readonly IntPtr AddrOfPinnedObject()
{
if (target.GetType() == typeof(string))
{
return (IntPtr)Unsafe.AsPointer(ref Unsafe.As<string>(target).GetRawStringData());
return Unsafe.AsPointer(ref Unsafe.As<string>(target).GetRawStringData());
}

Debug.Assert(target is Array);
return (IntPtr)Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<Array>(target)));
return Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<Array>(target)));
}

return (IntPtr)Unsafe.AsPointer(ref target.GetRawData());
return Unsafe.AsPointer(ref target.GetRawData());
}
}

Expand Down Expand Up @@ -183,13 +191,23 @@ public static GCHandle FromIntPtr(IntPtr value)
private static bool IsPinned(IntPtr handle) => (handle & 1) != 0; // Check Pin flag

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void ThrowIfInvalid(IntPtr handle)
internal static void ThrowIfInvalid(IntPtr handle)
{
// Check if the handle was never initialized or was freed.
if (handle == 0)
{
ThrowHelper.ThrowInvalidOperationException_HandleIsNotInitialized();
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void ThrowIfNotPinnable(object? value)
{
if (!Marshal.IsPinnable(value))
{
GC.KeepAlive(value);
throw new ArgumentException(SR.ArgumentException_NotIsomorphic, nameof(value));
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Runtime.InteropServices
{
/// <summary>
/// Provides extension methods to operate with GC handles.
/// </summary>
public static class GCHandleExtensions
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@huoyaoyuan huoyaoyuan Jan 11, 2025

Choose a reason for hiding this comment

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

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.

{
// The following methods are strongly typed generic specifications only on
// PinnedGCHandles with correct type.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


/// <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();
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Sergio0694 Sergio0694 Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Threading;

namespace System.Runtime.InteropServices
{
/// <summary>
/// Represents an strongly-typed opaque, GC handle to a managed object.
/// A GC handle is used when an object reference must be reachable from
/// unmanaged memory.
/// The object is pinned at fixed location in GC heap and allows its
/// address to be taken.
/// </summary>
/// <remarks>
/// <see cref="PinnedGCHandle{T}"/> corresponds to Pinned roots.
/// For Normal, see <see cref="GCHandle{T}"/>.
/// For Weak and WeakTrackResurrection, see <see cref="WeakGCHandle{T}"/>.
/// </remarks>
/// <seealso cref="GCHandle" />
/// <typeparam name="T">The type of the object this <see cref="GCHandle{T}"/> tracks to.</typeparam>
public struct PinnedGCHandle<T> : IDisposable
where T : class?
{
// The actual integer handle value that the EE uses internally.
private IntPtr _handle;

/// <summary>
/// Allocates a handle for the specified object.
/// </summary>
/// <param name="target">The object that uses the <see cref="GCHandle{T}"/>.</param>
public PinnedGCHandle(T target)
{
GCHandle.ThrowIfNotPinnable(target);
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved
_handle = GCHandle.InternalAlloc(target, GCHandleType.Pinned);
}

private PinnedGCHandle(IntPtr handle) => _handle = handle;

/// <summary>Determine whether this handle has been allocated or not.</summary>
public readonly bool IsAllocated => _handle != IntPtr.Zero;

/// <summary>Gets or sets the object this handle represents.</summary>
public readonly T Target
{
get
{
IntPtr handle = _handle;

// Check if the handle was never initialized or was freed.
if (handle == IntPtr.Zero)
{
ThrowHelper.ThrowInvalidOperationException_HandleIsNotInitialized();
}

// Skip the type check to provide maximum unsafety.
return Unsafe.As<T>(GCHandle.InternalGet(handle));
}
set
{
IntPtr handle = _handle;
GCHandle.ThrowIfInvalid(handle);
GCHandle.ThrowIfNotPinnable(value);
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved

GCHandle.InternalSet(handle, value);
}
}

/// <summary>
/// Retrieves the address of object data in a <see cref="PinnedGCHandle{T}"/>.
/// </summary>
/// <returns>The address of the pinned data object.</returns>
[CLSCompliant(false)]
public readonly unsafe void* GetAddressOfObjectData()
{
IntPtr handle = _handle;
GCHandle.ThrowIfInvalid(handle);

return GCHandle.AddrOfPinnedObjectFromHandle(_handle);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}

/// <summary>
/// Returns a new <see cref="PinnedGCHandle{T}"/> object created from a handle to a managed object.
/// </summary>
/// <param name="value">An <see cref="IntPtr"/> handle to a managed object to create a <see cref="PinnedGCHandle{T}"/> object from.</param>
/// <returns>A new <see cref="PinnedGCHandle{T}"/> object that corresponds to the value parameter.</returns>
/// <remarks>
/// The <see cref="IntPtr"/> representation of <see cref="PinnedGCHandle{T}"/> is not
/// interchangable with <see cref="GCHandle"/>.
/// </remarks>
public static PinnedGCHandle<T> FromIntPtr(IntPtr value)
{
GCHandle.ThrowIfInvalid(value);
return new PinnedGCHandle<T>(value);
}

/// <summary>
/// Returns the internal integer representation of a <see cref="PinnedGCHandle{T}"/> object.
/// </summary>
/// <param name="value">A <see cref="PinnedGCHandle{T}"/> object to retrieve an internal integer representation from.</param>
/// <returns>An <see cref="IntPtr"/> object that represents a <see cref="PinnedGCHandle{T}"/> object.</returns>
/// <remarks>
/// The <see cref="IntPtr"/> representation of <see cref="PinnedGCHandle{T}"/> is not
/// interchangable with <see cref="GCHandle"/>.
/// </remarks>
public static IntPtr ToIntPtr(PinnedGCHandle<T> value) => value._handle;

/// <summary>
/// Releases this <see cref="PinnedGCHandle{T}"/>.
/// </summary>
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);
}
}
}
}
Loading
Loading