dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.18k stars 4.72k forks source link

Proposal: Unsafe APIs to support ref void* and ref T* #44968

Closed Sergio0694 closed 3 years ago

Sergio0694 commented 3 years ago

Background and Motivation

The System.Runtime.CompilerServices.Unsafe class exposes an extremely useful set of APIs that allow developers working with low-level APIs to do things that are currently not expressible in C#, but due to how the APIs are currently designed, there are a number of scenarios that are still not properly supported, specifically around the usage of pointer types. This is mostly due to the fact that C# doesn't allow using void as a generic type parameter, nor any pointer type. So if you're doing any kind of work with a ref void* or ref T*, you're mostly out of luck today, and need to resort to either rewriting your code rifferently, or using slower workarounds like pinning the ref and then using a double pointer to do work. But even then it's not really ideal.

Proposed API

namespace System.Runtime.CompilerServices
{
    public static class Unsafe
    {
        public static ref void** Add(ref void* source, int elementOffset);
        public static ref T* Add<T>(ref T* source, int elementOffset) where T : unmanaged;
        public static ref void* Add(ref void* source, nint elementOffset);
        public static ref T* Add<T>(ref T* source, nint elementOffset) where T : unmanaged;
        public static bool AreSame(ref void* left, ref void* right);
        public static bool AreSame<T>(ref T* left, ref T* right) where T : unmanaged;
        public static ref T* As<T>(ref void* source) where T : unmanaged;
        public static ref TTo* As<TFrom, TTo>(ref TFrom* source) where TFrom : unmanaged where TTo : unmanaged;
        public static void** AsPointer(ref void* value);
        public static T** AsPointer<T>(ref T* value) where T : unmanaged;
        public static nint ByteOffset(ref void* origin, ref void* target);
        public static nint ByteOffset<T>(ref T* origin, ref T* target) where T : unmanaged;
        public static void CopyBlock(ref void* destination, ref void* source, uint byteCount);
        public static void InitBlock(ref void* startAddress, void* value, uint byteCount);
        public static bool IsAddressGreaterThan(ref void* left, ref void* right);
        public static bool IsAddressGreaterThan<T>(ref T* left, ref T* right) where T : unmanaged;
        public static bool IsAddressLessThan(ref void* left, ref void* right);
        public static bool IsAddressLessThan<T>(ref T* left, ref T* right) where T : unmanaged;
        public static bool IsNullRef(ref void* source);
        public static bool IsNullRef<T>(ref T* source) where T : unmanaged;
        public static ref void* NullRef();
        public static ref T* NullRef<T>() where T : unmanaged;
        public static void SkipInit(out void* value);
        public static void SkipInit<T>(out T* value) where T : unmanaged;
        public static ref void* Subtract<T>(ref void* source, int elementOffset);
        public static ref T* Subtract<T>(ref T* source, int elementOffset) where T : unmanaged;
        public static ref void* Subtract<T>(ref void* source, nint elementOffset);
        public static ref T* Subtract<T>(ref T* source, nint elementOffset) where T : unmanaged;
    }
}

Usage Examples

For instance, C# today doesn't allow getting the address of a field in a ref struct without pinning, even if this is already pinned by definition (see https://github.com/dotnet/csharplang/issues/1792, such a feature is not being added any time soon unfortunately). To work around this, you can just do Unsafe.AsPointer(ref myField), but if the field is of a pointer type, none of the current APIs will work.

Consider this example:

internal readonly unsafe ref struct ID3D12ResourceMap
{
    private readonly ID3D12Resource* d3D12Resource;
    public readonly void* Pointer;

    public ID3D12ResourceMap(ID3D12Resource* d3d12resource)
    {
        this.d3D12Resource = d3d12resource;

        // ID3D12Resource::Map takes a void**, but this code will not build.
        d3d12resource->Map(0, null, Unsafe.AsPointer(ref Pointer));
    }

    public void Dispose()
    {
        this.d3D12Resource->Unmap(0, null);
    }
}

Risks

None that I can see. The class is already named Unsafe and it's in the S.R.CS namespace, so only developers looking for exactly these APIs will ever use them. And they can already shoot themselves in the foot with the currently existing APIs 😄

benaadams commented 3 years ago

Aside, this code does build

public ID3D12ResourceMap(ID3D12Resource* d3d12resource)
{
    this.d3D12Resource = d3d12resource;

    // ID3D12Resource::Map takes a void**
    fixed (void** p = &Pointer)
    {
        d3d12resource->Map(0, null, p);
    }
}

Though I assume you'd want to do this; which does not build

public ID3D12ResourceMap(ID3D12Resource* d3d12resource)
{
    this.d3D12Resource = d3d12resource;

    // ID3D12Resource::Map takes a void**, this code does not build
    d3d12resource->Map(0, null, &Pointer);
}
Sergio0694 commented 3 years ago

@benaadams Yup, mentioned that as a workaround. Either that, or just do:

void* pointer;

d3d12resource->Map(0, null, &pointer);

Pointer = pointer;

But both are very verbose for what they're doing, and fixed also adds the unnecessary pinning overhead (which the JIT currently doesn't elide, though there's an issue for this). The idea is to make all of these scenarios feel more natural and easier to use.

Of course, for how much working with this stuff can be considered "natural and easy to use" 😄

EDIT: yeah in this specific scenario, I wish C# just allowed getting the address of a field in a ref struct. It's really unfortunate that's currently not supported, and just stuck in the backlog for now 😟

GrabYourPitchforks commented 3 years ago

IMO this is going to lead to overload explosion. The cleanest solution would be for the C# language to support ref arithmetic natively and to relax some restrictions on taking pointer. Consider your earlier example:

    public ID3D12ResourceMap(ID3D12Resource* d3d12resource)
    {
        this.d3D12Resource = d3d12resource;

        // ID3D12Resource::Map takes a void**, but this code will not build.
-        d3d12resource->Map(0, null, Unsafe.AsPointer(ref Pointer));
+        d3d12resource->Map(0, null, &this.Pointer);
    }

Right now that doesn't compile, but since 'this' is a ref struct it should be perfectly legal to get the address of it or any of its member fields without pinning and without interference from the GC. That C# doesn't allow this is best filed as an issue in the csharplang repo.

As a temporary workaround, I believe all APIs proposed here can be provided on a custom UnsafeEx class written solely in terms of existing Unsafe APIs.

john-h-k commented 3 years ago

That C# doesn't allow this is best filed as an issue in the csharplang repo.

Unfortunately, they said they won't be changing this soon image

. > As a temporary workaround, I believe all APIs proposed here can be provided on a custom UnsafeEx class written solely in terms of existing Unsafe APIs.

Because you can't use pointers as generic type params, there is no way to do As, AsRef, or AsPointer without dedicated, new, IL methods, as far as I know.

tannergooding commented 3 years ago

But since 'this' is a ref struct it should be perfectly legal to get the address of it or any of its member fields without pinning and without interference from the GC. That C# doesn't allow this is best filed as an issue in the csharplang repo.

There is an existing issue and it is currently on the long term backlog due to complications in changing how fixed works: https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-10-14.md#do-not-require-fixing-a-fixed-field-of-a-ref-struct

I believe all APIs proposed here can be provided on a custom UnsafeEx class written solely in terms of existing Unsafe APIs.

This is blocked by As today. If there were an overload that allowed converting a ref T* to a ref IntPtr or similar, then you could successfully build all the other APIs. However, as it is today you can't use ref structs or pointers as generic type arguments. This blocks the ability to use As to convert to something that can be used elsewhere and also blocks other scenarios like SizeOf<T> where T is a ref struct.

tannergooding commented 3 years ago

There is also https://github.com/dotnet/runtime/issues/13627, which proposes allowing pointers as generic type arguments and which would unblock most of these scenarios (minus ref structs).

Sergio0694 commented 3 years ago

@GrabYourPitchforks I agree with the high number of overloads - in fact I initially included the full list of possible APIs as I figured the conversation here would've trimmed them down to an actual subset to propose for review. Personally I'd be fine with a smaller subset of APIs, which currently can't be expressed otherwise as @tannergooding. Also there's the issue with C# not supporting this feature any time soon, so it'd be nice if these APIs could act as a stopgap for the time being. For instance:

namespace System.Runtime.CompilerServices
{
    public static class Unsafe
    {
        public static ref T* As<T>(ref void* source) where T : unmanaged;
        public static ref TTo* As<TFrom, TTo>(ref TFrom* source) where TFrom : unmanaged where TTo : unmanaged;
        public static void** AsPointer(ref void* value);
        public static T** AsPointer<T>(ref T* value) where T : unmanaged;
        public static void SkipInit(out void* value);
        public static void SkipInit<T>(out T* value) where T : unmanaged;

        // Optional, nice to have
        public static bool IsNullRef(ref void* source);
        public static bool IsNullRef<T>(ref T* source) where T : unmanaged;
        public static ref void* PointerNullRef();
        public static ref T* PointerNullRef<T>() where T : unmanaged;
    }
}

Seems like a more reasonable API surface for a proposal? 🙂

jkotas commented 3 years ago

However, as it is today you can't use ref structs or pointers as generic type arguments.

Why do you need to use ref structs to write unsafe code like this?

GrabYourPitchforks commented 3 years ago

If you need to support ref T* or other constructs, can you write the implementation in IL, same as we do today? Then ilasm your .il file into its own standalone DLL and you're good to go!

tannergooding commented 3 years ago

Why do you need to use ref structs to write unsafe code like this?

I don't think ref structs tend to be the problem and I merely called them out as an example of what generics don't support today.

However, I do frequently hit the case of SomeMethod<T> isn't useable because T is a pointer. Most often this gets worked around by using IntPtr instead, but that causes a loss of type information which can be bad. I've taken to using an internal struct Pointer<T> where T : unmanaged { private T* _value; } type to workaround this in a few cases and of course opened up #13627 as I think this is something worth supporting more generally.

Some example cases are dealing with Unsafe, but also cases like Lazy<T> (you want to lazily call some interop code that constructs an expensive native object), tracking a collection of unmanaged objects (List<T>, IEnumerable<T>, etc), doing custom comparisons, etc.

Having ref TTo Unsafe.As<TFrom, TTo>(ref TFrom* tfrom) and the reverse would unblock at least a number of these scenarios and would allow users to implement other helpers themselves. This would provide a stopgap until #13627 could be implemented (if it happens at all).

GrabYourPitchforks commented 3 years ago

Quick \& dirty hack to get Unsafe.AsPointer<T>(ref T* value):

namespace System.Runtime.CompilerServices
{
    public unsafe static class UnsafeEx
    {
        public static T** AsPointer<T>(ref T* value) where T : unmanaged
        {
            delegate*<ref byte, void*> d = &Unsafe.AsPointer;
            return ((delegate*<ref T*, T**>)d)(ref value);
        }
    }
}

Creating a standalone DLL where this is written in pure IL would be slightly more efficient (since no calli opcode), but if you need to stick with C# and need an immediate workaround this should get the job done.

Ninja edit, since I saw some questions on this. This sample should be fully legal per ECMA-335. It's not relying on internal / undocumented implementation details of the runtime. However, I did see some samples saying "oh, I can use a normal delegate and use Unsafe.As<T>(object) to lie about the delegate type!" Please don't use that specific overload of Unsafe.As to lie to the runtime about the type of an object. That technique isn't really supported by the runtime, and you might run into weird VM edge cases that could lead to process corruption.

EgorBo commented 3 years ago

Creating a standalone DLL where this is written in pure IL would be slightly more efficient (since no calli opcode),

Right, it needs https://github.com/dotnet/runtime/issues/44610 to inline that AsPointer 🙂

tannergooding commented 3 years ago

This is definitely a Thanks I Love/Hate It scenario.

But I'd still much rather see us expose the two As overloads than encourage people to use function pointers to lie about the return/parameter types of managed methods.

Sergio0694 commented 3 years ago

So uhm... I actually found a solution that has no pinning, and no extra calli either 🤣 I just have a single call here which would've been the same one to d3d12resource->Map anyway, but here it's explicit.

internal readonly unsafe ref struct ID3D12ResourceMap
{
    public readonly void* Pointer;

    public ID3D12ResourceMap(ID3D12Resource* d3d12resource)
    {
        var pMap = (delegate* unmanaged<ID3D12Resource*, uint, D3D12_RANGE*, void**, int>)(*(void***)d3d12resource)[8];

        ((delegate* unmanaged<ID3D12Resource*, uint, D3D12_RANGE*, out void*, int>)pMap)(d3d12resource, 0, null, out Pointer);
    }
}

Not sure how I feel about this to be honest 😄 Having a built-in API might be just a tiiiiiiny bit less error prone.

GrabYourPitchforks commented 3 years ago

Because your delegate* unmanaged<....> signature contains a non-blittable parameter (there's an out void* instead of a void**), the JIT will generate a p/invoke stub that performs pinning on your behalf.

Sergio0694 commented 3 years ago

Oooh, so that's how that works! I was just about wondering why ref/in/out parameters were allowed for unmanaged function pointers, I was actually expecting them not to compiler at all. Thanks for the additional info! 😄

But jokes aside (would never actually use code like this in production anyway, for obvious reasons), other than this specific example which I already refactored differently anyway, I agree with @tannergooding that I think there would be value in offering at least a small subset of APIs from this proposal. Like, some As overloads with pointers, possibly a couple for AsRef too. And then devs would be able to just build the others directly from there, just with the initial ability to easily switch to pointers from a ref to a pointer, and to eg. a ref IntPtr from a ref void* and the likes. As I said the initial proposal here was definitely too extensive, but just selecting a few from them might actually make sense? 🤔

Sergio0694 commented 3 years ago

Just figured I'd touch base again about this to figure out what the next steps would be, and also to avoid just letting this issue stuck in limbo if we agree it's not the best way forwards. From re-reading the previous conversation, it seems to me that:

Should I just close this and we can eventually pick it up from https://github.com/dotnet/runtime/issues/13627 once things settle for .NET 6? 🙂

krwq commented 3 years ago

@Sergio0694 this sounds reasonable for me, I'd rather push for #13627 to be fixed

Sergio0694 commented 3 years ago

Alright, closing this then, and hopefully we'll get #13627 in for .NET 7 or soon enough anyway, a dev can dream 😄

rickbrew commented 3 years ago

This would be really useful in my upcoming interop library conversion from C++/CLI to C# :) This will be several months down the road though

We were talking about it on Discord and I was able to come up with this, which doesn't require the delegate pointer trick and may produce better codegen (unverified either way):

public unsafe struct Pointer<T> where T : unmanaged
{
    public T* pointer;

    public Pointer(T* pointer)
    {
        this.pointer = pointer;
    }

    public static Pointer<T> InterlockedExchange(ref Pointer<T> target, Pointer<T> value)
    {
        ref IntPtr targetIntPtr = ref Unsafe.As<Pointer<T>, IntPtr>(ref target);
        ref IntPtr valueIntPtr = ref Unsafe.As<Pointer<T>, IntPtr>(ref value);
        IntPtr result = Interlocked.Exchange(ref targetIntPtr, valueIntPtr);
        return Unsafe.As<IntPtr, Pointer<T>>(ref result);
    }

    // ... add other methods using similar pattern
}

... although this requires changing from using e.g. IUnknown* p to Pointer<IUnknown> p. Might be reasonable depending on the context.

(Posting this in case others wander in here while searching for a solution...)