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

Permit loading Explicit layouts with generics constrained to unmanaged #97526

Open
MichalPetryka opened this issue Jan 25, 2024 · 15 comments
Open

Comments

@MichalPetryka
Copy link
Contributor

MichalPetryka commented Jan 25, 2024

Currently the runtime errors out when trying to load such type:

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

[StructLayout(LayoutKind.Explicit)]
public struct S<T> where T : unmanaged
{
    [FieldOffset(0)]
    public T t;
    [FieldOffset(0)]
    public int i;
}

with System.TypeLoadException: Could not load type 'S`1' from assembly '_, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' because generic types cannot have explicit layout.

While determining proper offsets for generics is problematic due to unknown sizes, union types with all fields at 0 offset don't face such issues and since the generic is constrained to unmanaged, the runtime doesn't need to worry about managed types appearing in the layout.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 25, 2024
@hamarb123
Copy link
Contributor

+1 from me, this would be very useful.

@MichalStrehovsky
Copy link
Member

Sidenote: the unmanaged constraint is a concept the C# compiler made up and runtime hasn't so far made decisions around that. typeof(S<>).MakeGenericType(typeof(KeyValuePair<string, string>)) will load fine.

@hamarb123
Copy link
Contributor

Sidenote: the unmanaged constraint is a concept the C# compiler made up and runtime hasn't so far made decisions around that. typeof(S<>).MakeGenericType(typeof(KeyValuePair<string, string>)) will load fine.

That's inaccurate, and has been so since .NET 5.0 at least iirc, but probably even a bit earlier - link.

@MichalStrehovsky
Copy link
Member

Sidenote: the unmanaged constraint is a concept the C# compiler made up and runtime hasn't so far made decisions around that. typeof(S<>).MakeGenericType(typeof(KeyValuePair<string, string>)) will load fine.

That's inaccurate, and has been so since .NET 5.0 at least iirc, but probably even a bit earlier - link.

My comment is about unmanaged constraint being meaningless to the runtime (it's the same as struct). Your example doesn't load because of the StructLayout, not because of unmet constraints.

@hamarb123
Copy link
Contributor

hamarb123 commented Jan 26, 2024

I swear I tested this in the past with generic methods, but presumably I must have tested it incorrectly.

I think it would make more sense to allow it based on what the type parameters are in practice, as opposed to what they're constrained to anyway.

So, what I would suggest, is that if the type parameters are unmanaged (checked when loading generic type instantiation), then we allow it to work as expected. And, if you constrain it, then you can ensure yourself that it will successfully load.

@MichalPetryka
Copy link
Contributor Author

MichalPetryka commented Jan 26, 2024

Sidenote: the unmanaged constraint is a concept the C# compiler made up and runtime hasn't so far made decisions around that. typeof(S<>).MakeGenericType(typeof(KeyValuePair<string, string>)) will load fine.

That's inaccurate, and has been so since .NET 5.0 at least iirc, but probably even a bit earlier - link.

My comment is about unmanaged constraint being meaningless to the runtime (it's the same as struct). Your example doesn't load because of the StructLayout, not because of unmet constraints.

That doesn't seem to be fully the case:

Console.WriteLine(typeof(S<>).MakeGenericType(typeof(string)).FullName);
public struct S<T> where T : unmanaged
{
    public T t;
}

gives:

System.ArgumentException: GenericArguments[0], 'System.String', on 'S`1[T]' violates the constraint of type 'T'.
 ---> System.TypeLoadException: GenericArguments[0], 'System.String', on 'S`1[T]' violates the constraint of type parameter 'T'.
   at System.RuntimeTypeHandle.Instantiate(RuntimeType inst)
   at System.RuntimeType.MakeGenericType(Type[] instantiation)
   --- End of inner exception stack trace ---
   at System.RuntimeType.ValidateGenericArguments(MemberInfo definition, RuntimeType[] genericArguments, Exception e)
   at System.RuntimeType.MakeGenericType(Type[] instantiation)
   at Program.<Main>$(String[] args)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)

@MichalStrehovsky
Copy link
Member

That doesn't seem to be fully the case:

Not sure I follow - I wrote unmanaged is same as struct constraint and yes, string is not a struct.

@MichalPetryka
Copy link
Contributor Author

That doesn't seem to be fully the case:

Not sure I follow - I wrote unmanaged is same as struct constraint and yes, string is not a struct.

Oh I thought you meant it as struct being meaningless to the runtime too.

@lambdageek
Copy link
Member

I wonder how this will interact with the ref/non-ref check in the runtime for something like this:

[StructLayout(LayoutKind.Explicit)]
public struct S<T> where T : unmanaged
{
    [FieldOffset(0)]
    public T t;
    [FieldOffset(8)]
    public object i;
}

public struct TwoLongs {
   public long a;
   public long b;
}

So S<long> is ok, but S<TwoLongs> overlaps. I'm not sure if we currently do the overlap check on instantiations.

@MichalPetryka
Copy link
Contributor Author

I wonder how this will interact with the ref/non-ref check in the runtime for something like this

I'd say that blocking such layouts with managed fields would be okay. I don't have any use cases for generic managed unions.

@lambdageek
Copy link
Member

lambdageek commented Feb 5, 2024

I wonder how this will interact with the ref/non-ref check in the runtime for something like this

I'd say that blocking such layouts with managed fields would be okay. I don't have any use cases for generic managed unions.

My point isn't just that we need to block such structs. (But yes, we would need to block them) My point is that we don't have a check on generic instances right now, but if we allow an explicit layout on generics, we would need such a check. That might be fine - but right now the check isn't there.

@kg
Copy link
Member

kg commented Feb 5, 2024

I'm curious about the use scenario for this. It makes me nervous since it would allow you to author a struct that looks memory and type safe but will be broken depending on the size of T. Is there a problem this solves that can't be solved by a property that wraps Unsafe.As?

@adams85
Copy link

adams85 commented Feb 5, 2024

It makes me nervous since it would allow you to author a struct that looks memory and type safe but will be broken depending on the size of T

There are safe use cases. For example, when the field with the generic type is the last one in the memory layout, it won't be broken. Or, when the set of possible type arguments are limited and known at compile time, so the layout offsets can be calculated in a type safe way (see the problem below).

Is there a problem this solves that can't be solved by a property that wraps Unsafe.As?

Although I'm not 100% familiar with the topic, e.g. this looks like a problem that could benefit from this.

@lambdageek
Copy link
Member

Sidenote: the unmanaged constraint is a concept the C# compiler made up and runtime hasn't so far made decisions around that.

I dont' think this issue is about unmanaged actually.

What we need is some logic in the runtime like this:

  1. if a generic type definition has an explicit layout, set some bit on the type definition
  2. when instantiating a generic type definition, if the bit is set, run the field overlap check on the generic instance.

@steveisok steveisok removed the untriaged New issue has not been triaged by the area owner label Feb 5, 2024
@steveisok steveisok added this to the 9.0.0 milestone Feb 5, 2024
@steveisok steveisok modified the milestones: 9.0.0, 10.0.0 Aug 9, 2024
@timcassell
Copy link

#43486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants