Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[mono] generic wrapper methods for unsafe accessors#101732

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
lambdageek merged 31 commits intodotnet:mainfromlambdageek:hack-generic-wrappers
May 13, 2024

Conversation

@lambdageek
Copy link
Member

@lambdageeklambdageek commentedApr 30, 2024
edited
Loading

  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 theMonoClassField* directly and then emit a fake token that is just the index of theMonoClassField* in theMonoMethodWrapper: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 forMyList<T> instead ofMyList<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 alsofixes[mono][aot] Baseservices runtime tests failed in Mono fullAOT llvm job #92883

Contributes to#99830,#89439

NOT DONE

  • We don't check constraints on the generic target types yet

lewing, steveisok, fanyang-mono, and PaulusParssinen reacted with hooray emoji
@ghostghost added the area-VM-meta-mono labelApr 30, 2024
@lambdageeklambdageek added the NO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons) labelApr 30, 2024
@lambdageeklambdageekforce-pushed thehack-generic-wrappers branch 2 times, most recently from0149584 to817356bCompareMay 1, 2024 15:01
@lambdageeklambdageekforce-pushed thehack-generic-wrappers branch from4a9bcb4 todb8cbadCompareMay 1, 2024 19:06
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.
When we create generic wrappers (or wrappers in a generic class),if the wrapper data needs to refer to a field, method, or parametertype of the definition, that data might need to be inflated if thecontaining class is inflated (or the generic wrapper method isinflated).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 othercall that calls mono_mb_add_data):```cmono_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
Try to separate the ideas of "the call to the UnsafeAccessor methodwas inflated, so we need to inflate the wrapper" and "theUnsafeAccessor method is a generic method definition, so the wrappershould be a generic method definition, too"
@lambdageeklambdageek changed the title[NO MERGE] generic wrapper methods for unsafe accessors[mono] generic wrapper methods for unsafe accessorsMay 6, 2024
@lambdageek
Copy link
MemberAuthor

/azp run runtime-llvm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageeklambdageekforce-pushed thehack-generic-wrappers branch 2 times, most recently from6276119 toc3dccc3CompareMay 9, 2024 19:40
@lambdageeklambdageekforce-pushed thehack-generic-wrappers branch fromc3dccc3 to25f9e99CompareMay 9, 2024 19:47
Copy link
Member

@fanyang-monofanyang-mono left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've reviewed the unsafe accessor related change. Will review the method builder related change then AOT related change.

mb=mono_mb_new (accessor_method->klass,accessor_method->name,MONO_WRAPPER_OTHER);
MonoGenericContextinst_ctx= {0,};
// FIXME: if is_inflated, do we need to mess with ctx?
inst_ctx.method_inst=container->context.method_inst;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why only the method context is set here? What about the class context?

Copy link
MemberAuthor

@lambdageeklambdageekMay 10, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So conceptually if both the class is generic and thestatic extern accessor method is generic, when we make a wrapper we want its signature to use the same exact class gparams as the accessor method (conceptually both the accessor method and the wrapper are members of the class). But we want it to use its own brand new method gparams. So we take the original signature of hte access method and replace the method gparams:

classG<T>{publicstaticexternTuple<T,U>Accessor<U>(Tt,Uu);// wrapper:// public static extern Tuple<T, U2> wrapper_Accessor<U2>(T t, U2 u);}

so it's as if the wrapper instead of having a gparam namedU has a gparam namedU2. And so if we just take signature of the accessor, we have to replace allUs byU2. But theT is the same

// we can't set inflate_wrapper_data to 0 on the result, it's possible it
// will need to be inflated again (for example in the method_inst ==
// generic_container->context.method_inst case, below)
resw->inflate_wrapper_data=1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nit: Consistency between1 andTRUE.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

MonoMethodWrapper:inflate_wrapper_data is a bitfield, so I use1

MonoMethodBuilder:inflate_wrapper_data is a gboolean, so I useTRUE

kotlarmilos reacted with thumbs up emoji
generic_wrapper= TRUE;
}

if (is_inflated) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Theis_inflated appears redundant sinceaccessor_method is already passed to theemit_unsafe_accessor_[field/ctor/method]_wrapper function.

Canaccessor_method->is_inflated be used directly instead of theis_inflated variable? The idea is to make the wrapper inflated if the accessor method is inflated and removegboolean to_be_inflated.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's not redundant. Note that below on line 5272 we reassignaccessor_method to be the declaring method. So we would have to switch fromaccessor_method->is_inflated toorig_method->is_inflated in this function. And down inemit_unsafe_accessor_wrapper we don't haveorig_method available.

Honestly what I would prefer is to makeemit_unsafe_accessor_wrapper not to take anaccessor_method argument at all. We can figure out the target class from the signature, and if we need to inflate the target method we can figure that out from the generic context. But there's other (mostly error checking) stuff that we can only get fromaccessor_method: like whether it's static.

But there's a good point here: I always just need to knowgboolean inflate_generic_data = accessor_method->is_generic || is_inflated inemit_unsafe_accessor_{ctor/method/field}_wrapper. So I will update marshal.c to just pass that down, instead of computing it in three different places.

kotlarmilos reacted with thumbs up emoji
Copy link
Member

@fanyang-monofanyang-mono left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

These are my feedback for the method builder part.

Copy link
Member

@fanyang-monofanyang-mono left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

These are my feedbacks for the aot part of change.

char*member_name=NULL;
intaccessor_kind=-1;
if (mono_method_get_unsafe_accessor_attr_data (method,&accessor_kind,&member_name,error)) {
MonoMethod*wrapper=mono_marshal_get_unsafe_accessor_wrapper (method, (MonoUnsafeAccessorKind)accessor_kind,member_name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I wonder why inflation is not needed here? i.e. callingreplace_generated_method instead.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This part is iterating over the method definitions in theMONO_TABLE_METHOD table. None of them are inflated.

Copy link
Member

@fanyang-monofanyang-mono left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for making this infrastructure change!

method_index=find_aot_method (shared,&amodule);
if (method_index!=0xffffff)
method=shared;
if (method_index==0xffffff&& !mono_method_metadata_has_header (method)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is it true that method without header will always be extern method with unsafe accessor attribute?

Copy link
MemberAuthor

@lambdageeklambdageekMay 13, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The overall structure ofmono_aot_get_method is that it keeps trying different ways to find the method that it needs whilemethod_index == 0xffffff (which means it hasn't found one yet).

So whenmini_replace_generated_method succeeds, it will return some method and we will try to find its method_index.

Ifmini_replace_generated_method doesn't find something that it can work on, it will returnNULL and we will keepmethod_index still as0xffffff andmono_aot_get_method will keep trying other ways.

mini_replace_generated_method is a little bit expensive - looking up a custom attribute is expensive. So we don't do it for everymethod. We only do it for the ones where there's no method header. Checking if there's a header is much cheaper than trying to find the UnsafeAccessorAttribute on every single method.

There might be other kinds of methods that have no method header. but we don't care about that - in those casesmini_replace_generated_method will return NULL and we'll keep going.

fanyang-mono reacted with thumbs up emoji
@lambdageeklambdageek merged commit2385d08 intodotnet:mainMay 13, 2024
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull requestMay 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 alsofixesdotnet#92883Contributes todotnet#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 readthe 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
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 14, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@kotlarmiloskotlarmiloskotlarmilos left review comments

@fanyang-monofanyang-monofanyang-mono approved these changes

@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

@marek-safarmarek-safarAwaiting requested review from marek-safar

@kgkgAwaiting requested review from kg

Assignees

@lambdageeklambdageek

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[mono][aot] Baseservices runtime tests failed in Mono fullAOT llvm job

3 participants

@lambdageek@kotlarmilos@fanyang-mono

[8]ページ先頭

©2009-2025 Movatter.jp