dotnet / runtime

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

Portable Span<T> incorrectly throws for a struct containing a pointer #53821

Open tannergooding opened 3 years ago

tannergooding commented 3 years ago

Description

The following program on full framework (or anywhere using portable span) fails with 'Cannot use type 'S'. Only value types without pointers or references are supported.'

using System;

unsafe struct S { public void* x; }

unsafe class Program
{
    static void Main(string[] args)
    {
        Span<S> x = new Span<S>(null, 0);
    }
}

This is thrown as part of SpanHelpers.IsReferenceOrContainsReferences<T>() which currently only checks Type.IsPrimitive and Type.IsValueType and causes issues in netstandard libraries, particularly around interop, that may be dealing with structs containing pointers.

I believe simply also checking Type.IsPointer would be sufficient here.

ghost commented 3 years ago

Tagging subscribers to this area: @GrabYourPitchForks, @carlossanlop See info in area-owners.md if you want to be subscribed.

Issue Details
### Description The following program on full framework (or anywhere using `portable span`) fails with 'Cannot use type 'S'. Only value types without pointers or references are supported.' ```csharp using System; unsafe struct S { public void* x; } unsafe class Program { static void Main(string[] args) { Span x = new Span(null, 0); } } ``` This is thrown as part of `SpanHelpers.IsReferenceOrContainsReferences()` which currently only checks `Type.IsPrimitive` and `Type.IsValueType` and causes issues in netstandard libraries, particularly around interop, that may be dealing with structs containing pointers. I believe simply also checking `Type.IsPointer` would be sufficient here.
Author: tannergooding
Assignees: -
Labels: `area-System.Memory`, `untriaged`
Milestone: -
tannergooding commented 3 years ago

CC. @jkotas, @jeffhandley, @ericstj, @ViktorHofer

Is this something that we would consider for servicing or are there complications/reasons why this is hard/not possible?

-- This was raised for the microsoft/ClangSharp repo which has helper methods for returning a Span<T> over certain native allocations (which involve pointer types)

jkotas commented 3 years ago

I do not think that this meets the 2.1 servicing bar: "High impact for significant number of customers without reasonable workaround".

I would not be opposed to servicing this if somebody can provide justification that meets the servicing bar. We have shipped multiple servicing updates of https://www.nuget.org/packages/System.Memory/ and I do not recall hearing any issues with it.

GrabYourPitchforks commented 3 years ago

The workaround would be to change your struct's field types to IntPtr instead of having actual pointers. That said, I don't know how much pain this would cause in the consuming app (do they now need to add a cast to every field access?), and this might not be feasible if the struct is auto-generated or provided by a third party lib.

tannergooding commented 3 years ago

The workaround would be to change your struct's field types to IntPtr instead of having actual pointers

Right, and I'm not going to do that for ClangSharp. It would make the surface significantly worse on .NET Core/.NET 5+ and only one person has reported it so far in the 2 years its been this way.

Likewise, as you pointed out, it would require changes to the ClangSharp P/Invoke generator itself to basically never generate pointers in "compat" mode, which just seems strictly worse 😄

tannergooding commented 3 years ago

The most "frustrating" part is that the following is legal as the check only exists for the pointer constructor:

S[] a = new S[5];
Span<S> x = new Span<S>(a);

But this also means that there is a workaround, just one that involves allocating.

saucecontrol commented 2 years ago

I just hit this today in a library that's targeting ns2.0 and netcoreapp3.1. Definitely a confusing error message given there's no technical reason it shouldn't work.

AraHaan commented 2 years ago

I agree, sometimes one needs to store unmanaged function pointers in said structs as well (where they call the Windows APIs to get the function's address to "call" it and assign it to that struct's member).

A prime example is a C++ class member function marked extern "C" and exported, so then you go about it to retrieve it in C# using function pointers.