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

Marshal.SizeOf(Type) is throwing incorrectly on generic types #46426

Open
pgrawehr opened this issue Dec 28, 2020 · 17 comments
Open

Marshal.SizeOf(Type) is throwing incorrectly on generic types #46426

pgrawehr opened this issue Dec 28, 2020 · 17 comments

Comments

@pgrawehr
Copy link

Description

Marshal.SizeOf(typeof(somegenericstruct)) throws ArgumentException when T is a closed generic type. It should only throw on open generic types.

Here's the implementation

public static int SizeOf(Type t)
        {
            if (t is null)
            {
                throw new ArgumentNullException(nameof(t));
            }
            if (!t.IsRuntimeImplemented())
            {
                throw new ArgumentException(SR.Argument_MustBeRuntimeType, nameof(t));
            }
            if (t.IsGenericType) // This should be changed to IsGenericTypeDefinition, I think
            {
                throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(t));
            }

            return SizeOfHelper(t, throwIfNotMarshalable: true);
        }

Configuration

Seen on .NET 5.0

Analysis

The documentation says that ArgumentException is thrown if t is a generic type definition. The implementation however throws when it is any generic type (including a closed one). The implementation of the similar method

public static int SizeOf<T>(T structure)

does not throw this exception - it can be used as workaround.

Here's some test code:

    public class FrameworkBehaviorTests
    {
        [Fact]
        public void CannotGetSizeOfOpenGenericType()
        {
			// This exception is expected
            Assert.Throws<ArgumentException>(() =>
            {
                Marshal.SizeOf(typeof(GenericStruct<>));
            });
        }

        [Fact]
        public void CanGetSizeOfOpenGenericType()
        {
            // This test fails and throws as well
            Assert.Equal(8, Marshal.SizeOf(typeof(GenericStruct<short>)));
        }

        [Fact]
        public void CanGetSizeOfOpenGenericTypeViaInstance()
        {
			// This test passes, because it uses the SizeOf<T>(T object) overload
            GenericStruct<short> gs;
            gs._data1 = 2;
            gs._data2 = 10;
            Assert.Equal(8, Marshal.SizeOf(gs));
        }

        private struct GenericStruct<T>
        {
            public T _data1;
            public int _data2;
        }
    }
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 28, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added needs more info and removed untriaged New issue has not been triaged by the area owner labels Jan 6, 2021
@AaronRobinsonMSFT
Copy link
Member

@pgrawehr This is the expected behavior. This behavior is consistent with .NET Framework and has been this way for quite some time - please let me know if I am missing the scenario. The fact that a Generic type has a closed type or not isn't a consideration for this API. Relaxing this to permit closed Generic types is possible, but it would require a discussion.

At this point I believe the open question is, is this a reported bug in the documentation or is the request to permit this operation?

@AaronRobinsonMSFT
Copy link
Member

It should be noted that the Marshal.SizeOf suite of functions are specifically for interop scenarios and not for computing arbitrary type size. With the support for some blittable Generic types in P/Invoke signatures (Issue: #4547 PR: dotnet/coreclr#23899) we may want to permit some narrow functionality in this area.

/cc @jkoritzinsky @elinor-fung @tannergooding

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Jan 6, 2021
@tannergooding
Copy link
Member

I believe we had previously discussed updating S.R.I.Marshal to handle generics as well but it was decided against. I can't seem to find the relevant thread at the moment, however (IIRC, enabling generics was one of the migrated PRs from dotnet/coreclr so that might be contributing).

@tannergooding
Copy link
Member

I might be remembering #32963 (comment), which was related to GetDelegateForFunctionPointer rather than SizeOf...

@pgrawehr
Copy link
Author

pgrawehr commented Jan 6, 2021

@AaronRobinsonMSFT I know that it's probably a rare scenario that one uses a struct with a generic parameter for blitting, however I see no reason why this shouldn't work (at least if the generic parameter is by itself blittable). Do note that it works as I would expect with the other overloads of SizeOf(), i.e. given an actual instance of the generic type. The SizeOf(Type) overload is the only one checking for this IsGenericType condition.

@eiriktsarpalis eiriktsarpalis added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs more info labels Oct 8, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: Future, 7.0.0 May 21, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label May 21, 2022
@AaronRobinsonMSFT
Copy link
Member

The SizeOf(Type) overload is the only one checking for this IsGenericType condition.

Yes and I think that is the point actually. I've given this some more thought and rummaged through some interop code bases and I think this API is commonly used for the "is blittable" check for a Type. This would imply that being more permissible could break existing applications, which is difficult to mitigate on the interop boundary.

Here are some other related issues to Generics and the Marshal class that we should either decide to enable or close.

@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: 7.0.0, Future May 21, 2022
@pgrawehr
Copy link
Author

@AaronRobinsonMSFT That's what I'm expecting. I'm expecting Sizeof(Type) to work when Type is blittable. And a generic struct with (defined) generic arguments is blittable. Consider (from the example above):

[StructLayout(LayoutKind.Sequenzial, Pack=1)]
private struct GenericStruct<T>
        {
            public T _data1;
            public int _data2;
        }

then the type GenericStruct<short> is blittable, and hence Marshal.SizeOf(typeof(GenericStruct<short>)) should succeed and return 6. I'm absolutely fine that Marshal.SizeOf(typeof(GenericStruct<>)) throws.

Note that all I'm suggesting is that the condition

            if (t.IsGenericType)
            {
                throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(t));
            }

be changed to

            if (t.IsGenericTypeDefinition)
            {
                throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(t));
            }

This would also be in line with the documentation, where it says that ArgumentException is thrown when "The t parameter is a generic type definition" (emphasis mine).

Conceptually, the entire test could be left away as well, since the implementation further down would throw anyway on something that is not blittable (only maybe with a less clear message).

@AaronRobinsonMSFT
Copy link
Member

I'm expecting Sizeof(Type) to work when Type is blittable. And a generic struct with (defined) generic arguments is blittable.

Agreed, but it isn't in .NET Framework and that means assumptions are baked into applications for that reason. Changing this would impact those assumptions about how code has evolved and branches.

@pgrawehr
Copy link
Author

Code that used to work would still work. I'm actually not expecting anyone to use this method to test whether an object contains generics, as this can be tested directly, not using this method and relying on catching the exception. No code should ever catch ArgumentException.

Also, if I understand #43486 and #103 correctly, marshalling generics was not at all supported in .NET Framework.

@AaronRobinsonMSFT
Copy link
Member

Also, if I understand #43486 and #103 correctly, marshalling generics was not at all supported in .NET Framework.

That is precisely my point. Consider the following for code that was initially ported from .NET Framework in the .NET Core 1.0 timeframe.

bool CanUseType(Type t)
{
    try
    {
        Marshal.SizeOf(t);
        return true;
    }
    catch
    {
        return false;
    }
}

Whatever logic this was blocking included cases when Type was a Generic, either open or closed. However, by relaxing the current behavior this code would now permit Generic types in code that quite possibly never expected that. This is very dangerous in the interop domain and it must be considered.

I referenced the other issues precisely to ensure or at least attempt to verify older code that was ported would continue to work. Opening up this one API could yield failures further down because other APIs haven't been updated. However, if we update how we think about Generics in all these situations then changing this behavior makes perfect sense. All I am getting at here is a piecewise permissiveness isn't something the interop team is generally comfortable with because it can expose existing fragile code paths to break in unexpected and dangerous ways.

@pgrawehr
Copy link
Author

pgrawehr commented May 23, 2022

I see your point, and digging a bit further, the problem seems to be larger than what I anticipated. It appears that when generic types where made blittable for .NET core, it was only partially done. I started with being confused that Marshal.SizeOf(typeof(GenericStruct<short>)) fails, but Marshal.Sizeof(new GenericStruct<short>()) works. Now I just see that Marshal.StructureToPtr(new GenericStruct<short>(), ptr, false) also fails, despite the documentation saying that it fails only in .NET Framework.

I guess we need a bunch of test cases first to test what works and what doesn't. And probably first a decision what behavior we would like to have and whether we risk taking a possibly breaking change. And then fix the code or update the documentation. The fact that the documentation states what I was expecting is an indication that it was at least planned to do the change at some point.

@tannergooding
Copy link
Member

When generic interop types were made possible, it was really just relaxing the restriction in the interop logic so it could cross the managed/native boundary.

There wasn't also work done to update Marshal.* to support this and much of that was done based on the feedback @AaronRobinsonMSFT has already given above.

Put another way, these types must already be blittable and so you don't want to use any of the Marshal APIs. Unsafe.SizeOf, Unsafe.WriteUnaligned, NativeMemory.Alloc, and other APIs are all faster and better for blittable data. The same goes for directly taking/passing MyStruct<T>* and using pointer types directly.

This is all "inline" with an effort to move users off of the built-in marshalling support and onto things like DllImportGenerator which generates and works off blittable data behind the scenes.

@pgrawehr
Copy link
Author

I see, but then I would expect remarks in the documentation of the Marshal class that it is considered legacy and which alternatives should be used. Instead, we have documentation that suggests that Marshal.* was updated, which apparently is just not the case.

@genaray
Copy link

genaray commented May 11, 2023

Whats the state of this issue?
I recently stumbled upon the same problem and it would really great to see this one in the future since the Marshal.SizeOf API is kinda limited rn.

@tannergooding
Copy link
Member

As indicated above, generic types must already be blittable to cross the P/Invoke boundary and so Unsafe.SizeOf will compute the correct result and be faster.

@genaray
Copy link

genaray commented May 15, 2023

As indicated above, generic types must already be blittable to cross the P/Invoke boundary and so Unsafe.SizeOf will compute the correct result and be faster.

However it does not support common Types, correct?
E.g. : Unsafe.SizeOf(typeof(MyWrapper<int>));

Would be nice to see a Type based API that is capable of determining those sizes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

7 participants