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

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

Closed
Sergio0694 opened this issue Nov 19, 2020 · 20 comments
Closed

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

Sergio0694 opened this issue Nov 19, 2020 · 20 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices
Milestone

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Nov 19, 2020

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 dotnet/csharplang#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 😄

@Sergio0694 Sergio0694 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Runtime.CompilerServices untriaged New issue has not been triaged by the area owner labels Nov 19, 2020
@benaadams
Copy link
Member

benaadams commented Nov 19, 2020

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
Copy link
Contributor Author

Sergio0694 commented Nov 19, 2020

@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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

There is also #13627, which proposes allowing pointers as generic type arguments and which would unblock most of these scenarios (minus ref structs).

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Nov 19, 2020

@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
Copy link
Member

jkotas commented Nov 19, 2020

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
Copy link
Member

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
Copy link
Member

tannergooding commented Nov 19, 2020

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
Copy link
Member

GrabYourPitchforks commented Nov 20, 2020

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
Copy link
Member

EgorBo commented Nov 20, 2020

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

Right, it needs #44610 to inline that AsPointer 🙂

@tannergooding
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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? 🤔

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Feb 16, 2021
@joperezr joperezr added this to the Future milestone Feb 16, 2021
@Sergio0694
Copy link
Contributor Author

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:

  • The right long term solution is to just make pointers as generic type arguments happen (as per Consider allow pointers as generic type arguments #13627, which @tannergooding opened already a while back), which would also bring along with it some great usability improvements for people working with interop and lowlevel code in general as well. There seems to be some holdup there due to relatively low priority of the feature, and it also needing changes to C#, so we'll probably need to also open a proposal in https://github.com/dotnet/csharplang/ to help track the feature, but we can do that in that issue.
  • This can technically already be worked around (although with a relatively terrifying solution) as @GrabYourPitchforks showed. As @EgorBo mentioned, we might also eventually get inlining for this (optimization of indirect calls #44610).
  • This is overall arguably a very, very niche problem anyway, and this proposal is more like a workaround rather than the best way to solve the whole issue (and more), which I agree with Tanner would just be to allow pointers in generic methods directly.

Should I just close this and we can eventually pick it up from #13627 once things settle for .NET 6? 🙂

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jul 19, 2021
@krwq
Copy link
Member

krwq commented Jul 20, 2021

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

@buyaa-n buyaa-n removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jul 21, 2021
@Sergio0694
Copy link
Contributor Author

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

@rickbrew
Copy link
Contributor

rickbrew commented Sep 7, 2021

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

@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices
Projects
No open projects
Development

No branches or pull requests