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

Implement Generic support for UnsafeAccessorAttribute #89439

Closed
4 tasks done
AaronRobinsonMSFT opened this issue Jul 25, 2023 · 17 comments
Closed
4 tasks done

Implement Generic support for UnsafeAccessorAttribute #89439

AaronRobinsonMSFT opened this issue Jul 25, 2023 · 17 comments

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jul 25, 2023

Originally defined in #81741, Generic support in UnsafeAccessorAttribute scenarios was not added. This issue tracks that effort across the runtimes.

For bound generic parameters this was originally thought to work but was broken after the final signature validation logic was implemented. This was reported in #92633 as a regression. That issue has been subsequently closed and folded into this work to support Generics on bound parameters too.

[UnsafeAccessor(UnsafeAccessorKind.Field, Name="_myList")]
static extern ref List<string> StringField(MyClass<string> _this);

[UnsafeAccessor(UnsafeAccessorKind.Field, Name="_myList")]
static extern ref List<double> DoubleField(MyClass<double> _this);

Example with unbound generic parameters.

[UnsafeAccessor(UnsafeAccessorKind.Field, Name="_myList")]
static extern ref List<T> Field<T>(MyClass<T> _this);
@ghost
Copy link

ghost commented Jul 25, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

Originally defined in #81741, Generic support in UnsafeAccessorAttribute scenarios was not added. This issue tracks that effort across the runtimes.

  • CoreCLR
  • NativeAOT
  • Mono

We are waiting to see community need for this work so setting to Future.

Author: AaronRobinsonMSFT
Assignees: -
Labels:

area-System.Runtime.CompilerServices

Milestone: Future

@AaronRobinsonMSFT
Copy link
Member Author

@lambdageek @MichalStrehovsky @jkotas @fanyang-mono This issue is being used to track Generic support going forward.

@MichalPetryka
Copy link
Contributor

This would be really useful when working with generic code for example in Serialization scenarios. Without this a lot of things still require DynamicMethod.

@lambdageek
Copy link
Member

Is it worth clarifying that specific generic instances, work already. So if we have MyClass<T> that has a private List<T> _myList member, then you can define, for example

[UnsafeAccessor(UnsafeAccessorKind.Field, Name="_myList")]
static extern ref List<string> StringField(MyClass<string> @this);

[UnsafeAccessor(UnsafeAccessorKind.Field, Name="_myList")]
static extern ref List<double> DoubleField(MyClass<double> @this);

just not the fully generic

[UnsafeAccessor(UnsafeAccessorKind.Field, Name="_myList")]
static extern ref List<T> Field<T>(MyClass<T> @this);

@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Implement Generic support for UnsafeAccessorAttribute Implement unbound Generic support for UnsafeAccessorAttribute Aug 8, 2023
@AaronRobinsonMSFT
Copy link
Member Author

Is it worth clarifying that specific generic instances, work already. So if we have MyClass<T> that has a private List<T> _myList member, then you can define

Agreed. I've updated the title to state "unbound" generics. Will move your examples up to the main area.

@neon-sunset
Copy link
Contributor

Just to express support for the feature, writing an efficient dictionary implementation that inherits from Dictionary<SomeUtf8StringImplementation, V> and allows using ReadOnlySpan<byte> for lookup is impossible without support for generics. Yes, this is a hack that has to rely on extensive test coverage to guard against a case where dictionary internals change, but there is no other option until type system accommodates for this in a bright but distant future.

@Kir-Antipov
Copy link

Just discovered that generic support hasn't been implemented yet (and, as it currently seems, won't be implemented until .NET 9) during the unit-testing stage of my library, and thank god I did :)

This feature is definitely needed. In fact, I'd argue that it's somewhat counter-intuitive that it isn't functional yet. This is especially so given that it was mentioned in the original proposal, and that the general principle - "copy-paste the original signature, add the target type as the first parameter, and you're good to go" - fails in this instance without any compiler warning (I hope it does for NAOT, though I haven't tested it yet).

In my specific situation, I attempted to access Enum.TryFormatUnconstrained<TEnum>(TEnum value, Span<char> destination, out int charsWritten, ReadOnlySpan<char> format), an internal version of Enum.TryFormat without generic constraints (where TEnum : struct, Enum). Unfortunately, despite the remarkable efforts from Stephen Toub in #78580, it's still impossible to utilize the optimal way of formatting enum values in an arbitrary generic method defined as Foo<T>(). This remains the case even if you've conducted a check yourself using typeof(T).IsEnum, as generic constraints remain unaware of it.

My problem here can be broken down into two components:

  1. The inability to apply the UnsafeAccessorAttribute to a generic method.
  2. The inability to invoke a method with generic constraints that represent a superset of the current method's generic constraints, even if I've conducted all the necessary checks myself.

There's a suggestion to establish a "bridging" system for generic constraints (dotnet/csharplang#6308) that might address (2). However, since it's proposed as a language feature, and the consensus on its design is yet to be found, it may take another 5 or 10 years to implement. So, why not consider it as a runtime feature instead, while simultaneously addressing (1)?

The original proposal states:

The generic constraints of the extern static method must match generic constraints of the target type, field or method.

I suggest either entirely disregarding this section or, preferably, introducing a mechanism to deliberately opt out of this verification. For instance, in my scenario, if I wish to call Enum.TryFormat with an arbitrary generic argument, I could create an accessor that permits me to bypass the constraints:

[UnsafeAccessorIgnoreGenericConstraints]
[UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name = nameof(Enum.TryFormat))]
static extern bool TryFormat<TEnum>(Enum? targetType, TEnum value, Span<char> destination, out int charsWritten, ReadOnlySpan<char> format);

To be quite frankly honest with you, this approach seems so much better compared to relying on the existence of an internal helper method, as such methods can be moved, renamed, or deleted in any non-breaking .NET update, as they aren't part of the public API.

If the final implementation of unsafe accessors evolves in this direction, it would be fantastic, as it simultaneously addresses:

  1. The suppression of visibility checks.
  2. The suppression of generic constraint checks.

At the end of the day, they are unsafe accessors, so incorporating this feature seems to fit seamlessly within their purview. In any case, I'm eager to hear others' opinions on this topic! :)

@MichalPetryka
Copy link
Contributor

I suggest either entirely disregarding this section or, preferably, introducing a mechanism to deliberately opt out of this verification.

I feel like adding such support would be useful in some cases, the runtime would just throw when the T doesn't meet the constraints, not when the constraints don't match.

@En3Tho
Copy link
Contributor

En3Tho commented Nov 12, 2023

F# has a feature of generic state machine builders. It's a feature that empowers you to write all sorts of stuff like specialized Tasks, AsyncEnumerables, custom Coroutines, all sorts of iterators using a unified approach.

These generally relay on template type that has among others has a Data field with user defined data like for example T Result field to store a return value. When expanded, F# first generate fields for state machine captures/closures before Data so getting Data field from resulting generated type is tricky and requires either knowing an offset (by invoking reflection) or resorting to other hacks.

It would be really nice to have an UnsafeAccessor that could just get Data field from any generated state machine.

Just wanted to share a use case for this.

@Sergio0694
Copy link
Contributor

Sergio0694 commented Jan 18, 2024

We could also use this in CsWinRT (here), so we could go from this:

private static Func<T, IObjectReference> BindToAbi()
{
    var objField = HelperType.GetField("_obj", BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly);
    return (T arg) => (IObjectReference) objField.GetValue(arg);
}

To this:

private static Func<T, IObjectReference> BindToAbi()
{
    [UnsafeAccessor(UnsafeAccessorKind.Field, Name="_obj")]
    static extern ref IObjectReference GetObj(T _this);

    return (T arg) => GetObj(arg);
}

Or actually just skip the function entirely and just call GetObj directly when needed, no caching necessary anymore.

@AaronRobinsonMSFT
Copy link
Member Author

    [UnsafeAccessor(UnsafeAccessorKind.Field, Name="_obj")]
    static extern ref IObjectReference GetObj(T _this);

Field access in this manner on reference types won't work with shared generics, there are no fields on Canon to acquire.

@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: Future, 9.0.0 Feb 17, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Implement unbound Generic support for UnsafeAccessorAttribute Implement Generic support for UnsafeAccessorAttribute Feb 17, 2024
@roald-di
Copy link

Is there some information available on which scenarios are going to be supported?

I'm mostly interested in whether it is going to be possible to define an accessor in a generic class and if generic methods are going to work.

At the moment both of these examples result in 'Invalid usage of UnsafeAccessorAttribute'.

// note no generics are used in the accessor itself
public class Accessors<T>
{
    [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "value")]
    public static extern ref int GetValue(MyClass _this);
}
[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "Method")]
public static extern T Method<T>(MyClass _this);

If the answer to the above is yes, is a more complicated example like the following going to work?

public class Outer<TOuter>
{
    public class Inner<TInner>
    {
        Tuple<TOuter, TInner, T>? Method<T>() => null;
    }
}

public class Accessors<TOuter, TInner>
{
    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "Method")]
    public static extern Tuple<TOuter, TInner, T> Method<T>(Outer<TOuter>.Inner<TInner> _this);
}

or at least this?

public class Accessors
{
    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "Method")]
    public static extern Tuple<TOuter, TInner, T> Method<TOuter, TInner, T>(Outer<TOuter>.Inner<TInner> _this);
}

@alrz
Copy link
Member

alrz commented Mar 17, 2024

Since static members don't need an instance, could this accept a System.Type instead?

    [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name = "get_Descriptor")]
    public extern static MessageDescriptor GetDescriptor(System.Type type);

@AaronRobinsonMSFT
Copy link
Member Author

Since static members don't need an instance, could this accept a System.Type instead?

    [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name = "get_Descriptor")]
    public extern static MessageDescriptor GetDescriptor(System.Type type);

No. The design requires the type to be explicit. As mentioned though, since the target member is static a value of null can be passed.

Using Type would make things difficult since it would require inspection at each dispatch to determine the target. Requiring the type explicitly in the signature enables zero overhead after the method is jitted. This would not be the case if System.Type were used.

@ajphall
Copy link

ajphall commented May 4, 2024

Hi,

Did these changes make it into 9.0.100-preview.3.24204.13?

I am trying for example to do this:

public static class SortedListAccessor<TKey, TValue> where TKey : notnull
{
    [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "keys")]
    public static extern ref TKey[] GetKeys(SortedList<TKey, TValue> list);
}

var l = new SortedList<string, string>();
var keys = SortedListAccessor<string, string>.GetKeys(l);

But I get a System.BadImageFormatException: 'Invalid usage of UnsafeAccessorAttribute.'

@reflectronic
Copy link
Contributor

No, it'll be available in Preview 4

lambdageek added a commit that referenced this issue May 13, 2024
1. Adds support for compiling generic wrapper methods to Mono.  In some situations we need to emit a wrapper method that is generic.  This makes the MethodBuilder infrastructure support that.
2. Adds support for inflating generic wrapper data.  Wrappers have pointer data associated with them that is used by the code generator (For example instead of emitting field tokens, we record the `MonoClassField*` directly and then emit a fake token that is just the index of the `MonoClassField*` in the `MonoMethodWrapper:method_data` array).  The pointer data associated with a wrapper is normally just consumed verbatim.  But if the wrapper is part of a generic class, or if the wrapper is a generic method, the wrapper data might have generic parameters (for example it might be a MonoClassField for `MyList<T>` instead of `MyList<string>`).  Add support for tagging the data with its kind and inflating it if the wrapper method is inflated.
3. Applies (1) and (2) to unsafe accessor methods - the unsafe accesor wrapper generation now always tries to get the generic definition and to generate a wrapper for that generic definition and then inflate it.
4. Some AOT changes so that FullAOT substitutes lookups for an unsafe accessor by lookups for the wrapper.  Including if the unsafe accessor or wrapper is generic.  This also enabled gshared and gsharedvt for unsafe accessor wrappers.  This also fixes #92883


Contributes to #99830, #89439

**NOT DONE**
- We don't check constraints on the generic target types yet

---


* always AOT wrappers, even for generics, not the actual accessor

* add generic wrapper methods

* use generic method owner caches

* lookup unsafe accessor wrapper instances in aot-runtime

   if someone needs the unsafe accessor, lookup the wrapper instead.

   Needed when there's a call for extra instances

* cleanup marshaling - dont' use ctx as a flag

* handle some generic field accessors

   As long as the target is just some type that mentions a generic field, we're ok - the regular gsharing ldflda works. 
 It just can't be a type variable.

* issues.targets: enable some unsafe accessor AOT tests

* [metadata] add ability to inflate wrapper data

   When we create generic wrappers (or wrappers in a generic class), if the wrapper data needs to refer to a field, method, or parameter type of the definition, that data might need to be inflated if the containing class is inflated (or the generic wrapper method is inflated).

   Add a new function to opt into inflation:

   ```c
       get_marshal_cb ()->mb_inflate_wrapper_data (mb);
   ```

   Add a new function to be called after mono_mb_emit_op (or any other call that calls mono_mb_add_data):

   ```c
       mono_mb_emit_op (mb, CEE_LDFLDA, field);
       mono_mb_set_wrapper_data_kind (mb, MONO_MB_ILGEN_WRAPPER_DATA_FIELD);
   ```

   Note: mono_mb_set_wrapper_data_kind asserts if you haven't called mb_inflate_wrapper_data.

   TODO: add more wrapper data kinds for MonoMethod* and MonoClass* etc

* [marshal] refactor unsafe accessor; opt into inflate_wrapper_data

   Try to separate the ideas of "the call to the UnsafeAccessor method was inflated, so we need to inflate the wrapper" and "the UnsafeAccessor method is a generic method definition, so the wrapper should be a generic method definition, too"

* inflate MonoMethod wrapper data; impl ctor generic unsafe accessors

* fix windows build

* [aot] handle case of partial generic sharing for unsafe accessor

   In partial generic sharing, a call to an instance like `Foo<int>` is replaced by `Foo<T_INT>` where T is constrained to `int` and enums that use `int` as a base type.

   In that case the AOT compiler will emit the unsafe accessor wrapper instantiated with `T_INT`.  So in the AOT lookup we have to find calls to `UnsafeAccessor<int>` and replace them with calls to `(wrapper)
UnsafeAccessor<T_INT>` to match what the AOT compiler is doing

* [aot] for unsafe accessor wrappers with no name, record a length 0

   This is needed because for inflated unsafe accessors we write the inflated bit after the name.  So if we're accessing a constructor and we didn't record a name in the AOT image, we would erroneously read
the inflated bit as the name length.

* [aot-runtime] try to fix gsharedvt lookup

* better comments; remove fied FIXMEs

* update HelloWorld sample to support either normal AOT or FullAOT

* rename helper methods

* apply suggestions from code review

* DRY. compute inflate_generic_data in one place

* Just do one loop for inflating generic wrapper data

* better comments

* DRY. move common AOT code to mini.c
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this issue May 30, 2024
1. Adds support for compiling generic wrapper methods to Mono.  In some situations we need to emit a wrapper method that is generic.  This makes the MethodBuilder infrastructure support that.
2. Adds support for inflating generic wrapper data.  Wrappers have pointer data associated with them that is used by the code generator (For example instead of emitting field tokens, we record the `MonoClassField*` directly and then emit a fake token that is just the index of the `MonoClassField*` in the `MonoMethodWrapper:method_data` array).  The pointer data associated with a wrapper is normally just consumed verbatim.  But if the wrapper is part of a generic class, or if the wrapper is a generic method, the wrapper data might have generic parameters (for example it might be a MonoClassField for `MyList<T>` instead of `MyList<string>`).  Add support for tagging the data with its kind and inflating it if the wrapper method is inflated.
3. Applies (1) and (2) to unsafe accessor methods - the unsafe accesor wrapper generation now always tries to get the generic definition and to generate a wrapper for that generic definition and then inflate it.
4. Some AOT changes so that FullAOT substitutes lookups for an unsafe accessor by lookups for the wrapper.  Including if the unsafe accessor or wrapper is generic.  This also enabled gshared and gsharedvt for unsafe accessor wrappers.  This also fixes dotnet#92883


Contributes to dotnet#99830, dotnet#89439

**NOT DONE**
- We don't check constraints on the generic target types yet

---


* always AOT wrappers, even for generics, not the actual accessor

* add generic wrapper methods

* use generic method owner caches

* lookup unsafe accessor wrapper instances in aot-runtime

   if someone needs the unsafe accessor, lookup the wrapper instead.

   Needed when there's a call for extra instances

* cleanup marshaling - dont' use ctx as a flag

* handle some generic field accessors

   As long as the target is just some type that mentions a generic field, we're ok - the regular gsharing ldflda works. 
 It just can't be a type variable.

* issues.targets: enable some unsafe accessor AOT tests

* [metadata] add ability to inflate wrapper data

   When we create generic wrappers (or wrappers in a generic class), if the wrapper data needs to refer to a field, method, or parameter type of the definition, that data might need to be inflated if the containing class is inflated (or the generic wrapper method is inflated).

   Add a new function to opt into inflation:

   ```c
       get_marshal_cb ()->mb_inflate_wrapper_data (mb);
   ```

   Add a new function to be called after mono_mb_emit_op (or any other call that calls mono_mb_add_data):

   ```c
       mono_mb_emit_op (mb, CEE_LDFLDA, field);
       mono_mb_set_wrapper_data_kind (mb, MONO_MB_ILGEN_WRAPPER_DATA_FIELD);
   ```

   Note: mono_mb_set_wrapper_data_kind asserts if you haven't called mb_inflate_wrapper_data.

   TODO: add more wrapper data kinds for MonoMethod* and MonoClass* etc

* [marshal] refactor unsafe accessor; opt into inflate_wrapper_data

   Try to separate the ideas of "the call to the UnsafeAccessor method was inflated, so we need to inflate the wrapper" and "the UnsafeAccessor method is a generic method definition, so the wrapper should be a generic method definition, too"

* inflate MonoMethod wrapper data; impl ctor generic unsafe accessors

* fix windows build

* [aot] handle case of partial generic sharing for unsafe accessor

   In partial generic sharing, a call to an instance like `Foo<int>` is replaced by `Foo<T_INT>` where T is constrained to `int` and enums that use `int` as a base type.

   In that case the AOT compiler will emit the unsafe accessor wrapper instantiated with `T_INT`.  So in the AOT lookup we have to find calls to `UnsafeAccessor<int>` and replace them with calls to `(wrapper)
UnsafeAccessor<T_INT>` to match what the AOT compiler is doing

* [aot] for unsafe accessor wrappers with no name, record a length 0

   This is needed because for inflated unsafe accessors we write the inflated bit after the name.  So if we're accessing a constructor and we didn't record a name in the AOT image, we would erroneously read
the inflated bit as the name length.

* [aot-runtime] try to fix gsharedvt lookup

* better comments; remove fied FIXMEs

* update HelloWorld sample to support either normal AOT or FullAOT

* rename helper methods

* apply suggestions from code review

* DRY. compute inflate_generic_data in one place

* Just do one loop for inflating generic wrapper data

* better comments

* DRY. move common AOT code to mini.c
@fanyang-mono
Copy link
Member

Mono part of support is now complete. Closing this issue now, as all planed tasks have been completed.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests