From 60761f7a313f88cc43e1051a1cc125216048d513 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Mon, 15 Nov 2021 16:45:15 +0200 Subject: [PATCH 1/2] [interp] Improve logging on mobile devices Use a single g_print in bulk for every instruction. Otherwise, the instruction ends up being displayed on multiple lines. --- src/mono/mono/mini/interp/transform.c | 52 +++++++++++++-------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index 8566b12552adea..f6070f06229c70 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -1456,25 +1456,27 @@ dump_interp_compacted_ins (const guint16 *ip, const guint16 *start) { int opcode = *ip; int ins_offset = ip - start; + GString *str = g_string_new (""); - g_print ("IR_%04x: %-14s", ins_offset, mono_interp_opname (opcode)); + g_string_append_printf (str, "IR_%04x: %-14s", ins_offset, mono_interp_opname (opcode)); ip++; if (mono_interp_op_dregs [opcode] > 0) - g_print (" [%d <-", *ip++); + g_string_append_printf (str, " [%d <-", *ip++); else - g_print (" [nil <-"); + g_string_append_printf (str, " [nil <-"); if (mono_interp_op_sregs [opcode] > 0) { for (int i = 0; i < mono_interp_op_sregs [opcode]; i++) - g_print (" %d", *ip++); - g_print ("],"); + g_string_append_printf (str, " %d", *ip++); + g_string_append_printf (str, "],"); } else { - g_print (" nil],"); + g_string_append_printf (str, " nil],"); } - char *ins = dump_interp_ins_data (NULL, ins_offset, ip, opcode); - g_print ("%s\n", ins); - g_free (ins); + char *ins_data = dump_interp_ins_data (NULL, ins_offset, ip, opcode); + g_print ("%s%s\n", str->str, ins_data); + g_string_free (str, TRUE); + g_free (ins_data); } static void @@ -1488,51 +1490,47 @@ dump_interp_code (const guint16 *start, const guint16* end) } static void -dump_interp_inst_no_newline (InterpInst *ins) +dump_interp_inst (InterpInst *ins) { int opcode = ins->opcode; - g_print ("IL_%04x: %-14s", ins->il_offset, mono_interp_opname (opcode)); + GString *str = g_string_new (""); + g_string_append_printf (str, "IL_%04x: %-14s", ins->il_offset, mono_interp_opname (opcode)); if (mono_interp_op_dregs [opcode] > 0) - g_print (" [%d <-", ins->dreg); + g_string_append_printf (str, " [%d <-", ins->dreg); else - g_print (" [nil <-"); + g_string_append_printf (str, " [nil <-"); if (mono_interp_op_sregs [opcode] > 0) { for (int i = 0; i < mono_interp_op_sregs [opcode]; i++) { if (ins->sregs [i] == MINT_CALL_ARGS_SREG) { - g_print (" c:"); + g_string_append_printf (str, " c:"); int *call_args = ins->info.call_args; if (call_args) { while (*call_args != -1) { - g_print (" %d", *call_args); + g_string_append_printf (str, " %d", *call_args); call_args++; } } } else { - g_print (" %d", ins->sregs [i]); + g_string_append_printf (str, " %d", ins->sregs [i]); } } - g_print ("],"); + g_string_append_printf (str, "],"); } else { - g_print (" nil],"); + g_string_append_printf (str, " nil],"); } if (opcode == MINT_LDLOCA_S) { // LDLOCA has special semantics, it has data in sregs [0], but it doesn't have any sregs - g_print (" %d", ins->sregs [0]); + g_string_append_printf (str, " %d", ins->sregs [0]); } else { char *descr = dump_interp_ins_data (ins, ins->il_offset, &ins->data [0], ins->opcode); - g_print ("%s", descr); + g_string_append_printf (str, "%s", descr); g_free (descr); } -} - -static void -dump_interp_inst (InterpInst *ins) -{ - dump_interp_inst_no_newline (ins); - g_print ("\n"); + g_print ("%s\n", str->str); + g_string_free (str, TRUE); } static G_GNUC_UNUSED void From ec958377b093382ea0d0ac6cfb1b558053e538b7 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Mon, 15 Nov 2021 16:51:52 +0200 Subject: [PATCH 2/2] [interp] Remove hack for nint/nfloat These structures are valuetypes, but their mint_type is a primitive. This means that a LDFLD applied on the type would have attempted to do a pointer dereference, because it saw that the current top of stack is not STACK_TYPE_VT. This was fixed in the past by passing the managed pointer to the valuetype rather than the valuetype itself, against the normal call convention, which lead to inconsistencies in the code. This commit removes that hack and fixes the problem by ignoring LDFLD applied to nint/nfloat valuetypes in the first place. --- src/mono/mono/mini/interp/transform.c | 45 ++++++--------------------- 1 file changed, 9 insertions(+), 36 deletions(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index f6070f06229c70..7f2933b2c38a0e 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -1589,21 +1589,6 @@ interp_method_get_header (MonoMethod* method, MonoError *error) return mono_method_get_header_internal (method, error); } -/* stores top of stack as local and pushes address of it on stack */ -static void -emit_store_value_as_local (TransformData *td, MonoType *src) -{ - int local = create_interp_local (td, mini_native_type_replace_type (src)); - - store_local (td, local); - - interp_add_ins (td, MINT_LDLOCA_S); - push_simple_type (td, STACK_TYPE_MP); - interp_ins_set_dreg (td->last_ins, td->sp [-1].local); - interp_ins_set_sreg (td->last_ins, local); - td->locals [local].indirects++; -} - static gboolean interp_ip_in_cbb (TransformData *td, int il_offset) { @@ -1914,15 +1899,15 @@ interp_handle_magic_type_intrinsics (TransformData *td, MonoMethod *target_metho int src_size = mini_magic_type_size (NULL, src); int dst_size = mini_magic_type_size (NULL, dst); - gboolean store_value_as_local = FALSE; + gboolean managed_fallback = FALSE; switch (type_index) { case 0: case 1: if (!mini_magic_is_int_type (src) || !mini_magic_is_int_type (dst)) { if (mini_magic_is_int_type (src)) - store_value_as_local = TRUE; + managed_fallback = TRUE; else if (mono_class_is_magic_float (src_klass)) - store_value_as_local = TRUE; + managed_fallback = TRUE; else return FALSE; } @@ -1930,21 +1915,17 @@ interp_handle_magic_type_intrinsics (TransformData *td, MonoMethod *target_metho case 2: if (!mini_magic_is_float_type (src) || !mini_magic_is_float_type (dst)) { if (mini_magic_is_float_type (src)) - store_value_as_local = TRUE; + managed_fallback = TRUE; else if (mono_class_is_magic_int (src_klass)) - store_value_as_local = TRUE; + managed_fallback = TRUE; else return FALSE; } break; } - if (store_value_as_local) { - emit_store_value_as_local (td, src); - - /* emit call to managed conversion method */ + if (managed_fallback) return FALSE; - } if (src_size > dst_size) { // 8 -> 4 switch (type_index) { @@ -2001,15 +1982,6 @@ interp_handle_magic_type_intrinsics (TransformData *td, MonoMethod *target_metho td->ip += 5; return TRUE; } else if (!strcmp ("CompareTo", tm) || !strcmp ("Equals", tm)) { - MonoType *arg = csignature->params [0]; - int mt = mint_type (arg); - - /* on 'System.n*::{CompareTo,Equals} (System.n*)' variant we need to push managed - * pointer instead of value */ - if (mt != MINT_TYPE_O) - emit_store_value_as_local (td, arg); - - /* emit call to managed conversion method */ return FALSE; } else if (!strcmp (".cctor", tm)) { return FALSE; @@ -2846,8 +2818,7 @@ interp_method_check_inlining (TransformData *td, MonoMethod *method, MonoMethodS if (method->wrapper_type != MONO_WRAPPER_NONE) return FALSE; - /* Our usage of `emit_store_value_as_local ()` for nint, nuint and nfloat - * is kinda hacky, and doesn't work with the inliner */ + // FIXME Re-enable this if (mono_class_get_magic_index (method->klass) >= 0) return FALSE; @@ -6034,6 +6005,8 @@ generate_code (TransformData *td, MonoMethod *method, MonoMethodHeader *header, td->sp--; interp_emit_sfld_access (td, field, field_klass, mt, TRUE, error); goto_if_nok (error, exit); + } else if (td->sp [-1].type != STACK_TYPE_O && td->sp [-1].type != STACK_TYPE_MP && (mono_class_is_magic_int (klass) || mono_class_is_magic_float (klass))) { + // No need to load anything, the value is already on the execution stack } else if (td->sp [-1].type == STACK_TYPE_VT) { int size = 0; /* First we pop the vt object from the stack. Then we push the field */