Skip to content

Commit

Permalink
[mono] generic wrapper methods for unsafe accessors (#101732)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lambdageek authored May 13, 2024
1 parent 29cb8a6 commit 2385d08
Show file tree
Hide file tree
Showing 16 changed files with 533 additions and 104 deletions.
1 change: 1 addition & 0 deletions src/mono/mono/metadata/class-internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ struct _MonoMethodWrapper {
MonoMethodHeader *header;
MonoMemoryManager *mem_manager;
void *method_data;
unsigned int inflate_wrapper_data : 1; /* method_data[MONO_MB_ILGEN_INFLATE_WRAPPER_INFO_IDX] is an MonoMethodBuilderInflateWrapperData array */
};

struct _MonoDynamicMethod {
Expand Down
12 changes: 12 additions & 0 deletions src/mono/mono/metadata/class.c
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include <mono/metadata/gc-internals.h>
#include <mono/metadata/mono-debug.h>
#include <mono/metadata/metadata-update.h>
#include <mono/metadata/method-builder-ilgen.h>
#include <mono/utils/mono-string.h>
#include <mono/utils/mono-error-internals.h>
#include <mono/utils/mono-logger-internals.h>
Expand Down Expand Up @@ -1263,6 +1264,17 @@ mono_class_inflate_generic_method_full_checked (MonoMethod *method, MonoClass *k

resw->method_data = (void **)g_malloc (sizeof (gpointer) * (len + 1));
memcpy (resw->method_data, mw->method_data, sizeof (gpointer) * (len + 1));
if (mw->inflate_wrapper_data) {
mono_mb_inflate_generic_wrapper_data (context, (gpointer*)resw->method_data, error);
if (!is_ok (error)) {
g_free (resw->method_data);
goto fail;
}
// 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;
}
}

if (iresult->context.method_inst) {
Expand Down
110 changes: 49 additions & 61 deletions src/mono/mono/metadata/marshal-lightweight.c
Original file line number Diff line number Diff line change
Expand Up @@ -2136,6 +2136,16 @@ mb_skip_visibility_ilgen (MonoMethodBuilder *mb)
mb->skip_visibility = 1;
}

static void
mb_inflate_wrapper_data_ilgen (MonoMethodBuilder *mb)
{
g_assert (!mb->dynamic); // dynamic methods with inflated data not implemented yet - needs at least mono_free_method changes, probably more
mb->inflate_wrapper_data = TRUE;
int idx = mono_mb_add_data (mb, NULL);
// note: match index used in create_method_ilgen
g_assertf (idx == MONO_MB_ILGEN_INFLATE_WRAPPER_INFO_IDX, "mb_inflate_wrapper_data called after data already added");
}

static void
emit_synchronized_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *method, MonoGenericContext *ctx, MonoGenericContainer *container, MonoMethod *enter_method, MonoMethod *exit_method, MonoMethod *gettypefromhandle_method)
{
Expand Down Expand Up @@ -2265,20 +2275,26 @@ emit_array_accessor_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *method, Mo
mono_mb_emit_byte (mb, CEE_RET);
}

static gboolean
unsafe_accessor_target_type_forbidden (MonoType *target_type);

static void
emit_unsafe_accessor_field_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name)
emit_unsafe_accessor_field_wrapper (MonoMethodBuilder *mb, gboolean inflate_generic_data, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoUnsafeAccessorKind kind, const char *member_name)
{
// Field access requires a single argument for target type and a return type.
g_assert (kind == MONO_UNSAFE_ACCESSOR_FIELD || kind == MONO_UNSAFE_ACCESSOR_STATIC_FIELD);
g_assert (member_name != NULL);

MonoType *target_type = sig->params[0]; // params[0] is the field's parent
MonoType *ret_type = sig->ret;
if (sig->param_count != 1 || target_type == NULL || sig->ret->type == MONO_TYPE_VOID) {

MonoType *target_type = sig->param_count == 1 ? sig->params[0] : NULL; // params[0] is the field's parent

if (sig->param_count != 1 || target_type == NULL || sig->ret->type == MONO_TYPE_VOID || unsafe_accessor_target_type_forbidden (target_type)) {
mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute.");
return;
}

MonoType *ret_type = sig->ret;

MonoClass *target_class = mono_class_from_mono_type_internal (target_type);
gboolean target_byref = m_type_is_byref (target_type);
gboolean target_valuetype = m_class_is_valuetype (target_class);
Expand Down Expand Up @@ -2307,6 +2323,8 @@ emit_unsafe_accessor_field_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_
if (kind == MONO_UNSAFE_ACCESSOR_FIELD)
mono_mb_emit_ldarg (mb, 0);
mono_mb_emit_op (mb, kind == MONO_UNSAFE_ACCESSOR_FIELD ? CEE_LDFLDA : CEE_LDSFLDA, target_field);
if (inflate_generic_data)
mono_mb_set_wrapper_data_kind (mb, MONO_MB_ILGEN_WRAPPER_DATA_FIELD);
mono_mb_emit_byte (mb, CEE_RET);
}

Expand All @@ -2315,7 +2333,7 @@ emit_unsafe_accessor_field_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_
* of the expected member method (ie, with the first arg removed)
*/
static MonoMethodSignature *
method_sig_from_accessor_sig (MonoMethodBuilder *mb, gboolean hasthis, MonoMethodSignature *accessor_sig, MonoGenericContext *ctx)
method_sig_from_accessor_sig (MonoMethodBuilder *mb, gboolean hasthis, MonoMethodSignature *accessor_sig)
{
MonoMethodSignature *ret = mono_metadata_signature_dup_full (get_method_image (mb->method), accessor_sig);
g_assert (ret->param_count > 0);
Expand All @@ -2332,7 +2350,7 @@ method_sig_from_accessor_sig (MonoMethodBuilder *mb, gboolean hasthis, MonoMetho
* of the expected constructor method (same args, but return type is void).
*/
static MonoMethodSignature *
ctor_sig_from_accessor_sig (MonoMethodBuilder *mb, MonoMethodSignature *accessor_sig, MonoGenericContext *ctx)
ctor_sig_from_accessor_sig (MonoMethodBuilder *mb, MonoMethodSignature *accessor_sig)
{
MonoMethodSignature *ret = mono_metadata_signature_dup_full (get_method_image (mb->method), accessor_sig);
ret->hasthis = TRUE; /* ctors are considered instance methods */
Expand Down Expand Up @@ -2373,28 +2391,6 @@ emit_missing_method_error (MonoMethodBuilder *mb, MonoError *failure, const char
}
}

static MonoMethodSignature *
update_signature (MonoMethod *accessor_method)
{
MonoClass *accessor_method_class_instance = accessor_method->klass;
MonoClass *accessor_method_class = mono_class_get_generic_type_definition (accessor_method_class_instance);

const char *accessor_method_name = accessor_method->name;

gpointer iter = NULL;
MonoMethod *m = NULL;
while ((m = mono_class_get_methods (accessor_method_class, &iter))) {
if (!m)
continue;

if (strcmp (m->name, accessor_method_name))
continue;

return mono_metadata_signature_dup_full (get_method_image (m), mono_method_signature_internal (m));
}
g_assert_not_reached ();
}

static MonoMethod *
inflate_method (MonoClass *klass, MonoMethod *method, MonoMethod *accessor_method, MonoError *error)
{
Expand All @@ -2412,7 +2408,7 @@ inflate_method (MonoClass *klass, MonoMethod *method, MonoMethod *accessor_metho
}

static void
emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name)
emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, gboolean inflate_generic_data, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoUnsafeAccessorKind kind, const char *member_name)
{
g_assert (kind == MONO_UNSAFE_ACCESSOR_CTOR);
// null or empty string member name is ok for a constructor
Expand All @@ -2424,22 +2420,19 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_m
}

MonoType *target_type = sig->ret; // for constructors the return type is the target type
MonoClass *target_class = mono_class_from_mono_type_internal (target_type);

ERROR_DECL(find_method_error);
if (accessor_method->is_inflated) {
sig = update_signature(accessor_method);
target_type = sig->ret;
}

if (target_type == NULL || m_type_is_byref (target_type) || unsafe_accessor_target_type_forbidden (target_type)) {
mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute.");
return;
}

MonoClass *target_class = mono_class_from_mono_type_internal (target_type);

ERROR_DECL(find_method_error);

MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig, ctx);
MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig);

MonoClass *in_class = mono_class_get_generic_type_definition (target_class);
MonoClass *in_class = target_class;

MonoMethod *target_method = mono_unsafe_accessor_find_ctor (in_class, member_sig, target_class, find_method_error);
if (!is_ok (find_method_error) || target_method == NULL) {
Expand All @@ -2458,21 +2451,27 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_m
emit_unsafe_accessor_ldargs (mb, sig, 0);

mono_mb_emit_op (mb, CEE_NEWOBJ, target_method);
if (inflate_generic_data)
mono_mb_set_wrapper_data_kind (mb, MONO_MB_ILGEN_WRAPPER_DATA_METHOD);
mono_mb_emit_byte (mb, CEE_RET);
}

static void
emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name)
emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, gboolean inflate_generic_data, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoUnsafeAccessorKind kind, const char *member_name)
{
g_assert (kind == MONO_UNSAFE_ACCESSOR_METHOD || kind == MONO_UNSAFE_ACCESSOR_STATIC_METHOD);
g_assert (member_name != NULL);

// We explicitly allow calling a constructor as if it was an instance method, but we need some hacks in a couple of places
gboolean ctor_as_method = !strcmp (member_name, ".ctor");


MonoType *target_type = sig->param_count >= 1 ? sig->params[0] : NULL;

if (sig->param_count < 1 || target_type == NULL || unsafe_accessor_target_type_forbidden (target_type)) {
mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute.");
return;
}

MonoType *target_type = sig->params[0];
gboolean hasthis = kind == MONO_UNSAFE_ACCESSOR_METHOD;
MonoClass *target_class = mono_class_from_mono_type_internal (target_type);

Expand All @@ -2482,19 +2481,10 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor
}

ERROR_DECL(find_method_error);
if (accessor_method->is_inflated) {
sig = update_signature(accessor_method);
target_type = sig->params[0];
}

if (sig->param_count < 1 || target_type == NULL || unsafe_accessor_target_type_forbidden (target_type)) {
mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute.");
return;
}

MonoMethodSignature *member_sig = method_sig_from_accessor_sig (mb, hasthis, sig, ctx);
MonoMethodSignature *member_sig = method_sig_from_accessor_sig (mb, hasthis, sig);

MonoClass *in_class = mono_class_get_generic_type_definition (target_class);
MonoClass *in_class = target_class;

MonoMethod *target_method = NULL;
if (!ctor_as_method)
Expand Down Expand Up @@ -2522,17 +2512,14 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor
emit_unsafe_accessor_ldargs (mb, sig, !hasthis ? 1 : 0);

mono_mb_emit_op (mb, hasthis ? CEE_CALLVIRT : CEE_CALL, target_method);
if (inflate_generic_data)
mono_mb_set_wrapper_data_kind (mb, MONO_MB_ILGEN_WRAPPER_DATA_METHOD);
mono_mb_emit_byte (mb, CEE_RET);
}

static void
emit_unsafe_accessor_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name)
emit_unsafe_accessor_wrapper_ilgen (MonoMethodBuilder *mb, gboolean inflate_generic_data, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoUnsafeAccessorKind kind, const char *member_name)
{
if (accessor_method->is_generic || ctx != NULL) {
mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "UnsafeAccessor_Generics");
return;
}

if (!m_method_is_static (accessor_method)) {
mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "UnsafeAccessor_NonStatic");
return;
Expand All @@ -2541,14 +2528,14 @@ emit_unsafe_accessor_wrapper_ilgen (MonoMethodBuilder *mb, MonoMethod *accessor_
switch (kind) {
case MONO_UNSAFE_ACCESSOR_FIELD:
case MONO_UNSAFE_ACCESSOR_STATIC_FIELD:
emit_unsafe_accessor_field_wrapper (mb, accessor_method, sig, ctx, kind, member_name);
emit_unsafe_accessor_field_wrapper (mb, inflate_generic_data, accessor_method, sig, kind, member_name);
return;
case MONO_UNSAFE_ACCESSOR_CTOR:
emit_unsafe_accessor_ctor_wrapper (mb, accessor_method, sig, ctx, kind, member_name);
emit_unsafe_accessor_ctor_wrapper (mb, inflate_generic_data, accessor_method, sig, kind, member_name);
return;
case MONO_UNSAFE_ACCESSOR_METHOD:
case MONO_UNSAFE_ACCESSOR_STATIC_METHOD:
emit_unsafe_accessor_method_wrapper (mb, accessor_method, sig, ctx, kind, member_name);
emit_unsafe_accessor_method_wrapper (mb, inflate_generic_data, accessor_method, sig, kind, member_name);
return;
default:
mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "UnsafeAccessor_InvalidKindValue");
Expand Down Expand Up @@ -3404,6 +3391,7 @@ mono_marshal_lightweight_init (void)
cb.emit_return = emit_return_ilgen;
cb.emit_vtfixup_ftnptr = emit_vtfixup_ftnptr_ilgen;
cb.mb_skip_visibility = mb_skip_visibility_ilgen;
cb.mb_inflate_wrapper_data = mb_inflate_wrapper_data_ilgen;
cb.mb_emit_exception = mb_emit_exception_ilgen;
cb.mb_emit_exception_for_error = mb_emit_exception_for_error_ilgen;
cb.mb_emit_byte = mb_emit_byte_ilgen;
Expand Down
Loading

0 comments on commit 2385d08

Please sign in to comment.