Emit profiler enter/leave instrumentation calls as icalls.

This is more portable than using the mono_arch_instrument_* functions and
thus isn't prone to nasty arch-specific bugs. In particular, the old way
of instrumenting function prologues/epilogues resulted in a somewhat elusive
stack corruption bug on Android (ARM). Eventually, we should get rid of these
functions entirely and only use icalls, with JIT opcodes for special cases
(such as saving arguments for --trace).

This new way of doing things also makes profiler instrumentation AOT-safe,
since we're now going through the proper channels to emit references to the
runtime functions (mono_profiler_method_enter/leave) and to embed MonoMethod
pointers.

Since we're using icalls without wrappers to do this, there should be no
visible performance impact from this commit.

diff --git a/mono/mini/method-to-ir.c b/mono/mini/method-to-ir.c
index 1ecf79c9ac355a72f79b02ec999f8d1e724570ed..f57da5f405fbd8cf93cf733e64915c04006eb70e 100644 (file)
--- a/mono/mini/method-to-ir.c
+++ b/mono/mini/method-to-ir.c
@@ -2027,6 +2027,24 @@ emit_pop_lmf (MonoCompile *cfg)
        }
 }
 
+static void
+emit_instrumentation_call (MonoCompile *cfg, void *func)
+{
+       MonoInst *iargs [1];
+
+       /*
+        * Avoid instrumenting inlined methods since it can
+        * distort profiling results.
+        */
+       if (cfg->method != cfg->current_method)
+               return;
+
+       if (cfg->prof_options & MONO_PROFILE_ENTER_LEAVE) {
+               EMIT_NEW_METHODCONST (cfg, iargs [0], cfg->method);
+               mono_emit_jit_icall (cfg, func, iargs);
+       }
+}
+
 static int
 ret_type_to_call_opcode (MonoType *type, int calli, int virt, MonoGenericSharingContext *gsctx)
 {
@@ -2482,9 +2500,11 @@ mono_emit_call_args (MonoCompile *cfg, MonoMethodSignature *sig,
        int i;
 #endif
 
-       if (tail)
+       if (tail) {
+               emit_instrumentation_call (cfg, mono_profiler_method_leave);
+
                MONO_INST_NEW_CALL (cfg, call, OP_TAILCALL);
-       else
+       } else
                MONO_INST_NEW_CALL (cfg, call, ret_type_to_call_opcode (sig->ret, calli, virtual, cfg->generic_sharing_context));
 
        call->args = args;
@@ -7746,6 +7766,8 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
                        if (mono_security_cas_enabled ())
                                CHECK_CFG_EXCEPTION;
 
+                       emit_instrumentation_call (cfg, mono_profiler_method_leave);
+
                        if (ARCH_HAVE_OP_TAIL_CALL) {
                                MonoMethodSignature *fsig = mono_method_signature (cmethod);
                                int i, n;
@@ -8555,6 +8577,8 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
                                        /* Handle tail calls similarly to normal calls */
                                        tail_call = TRUE;
                                } else {
+                                       emit_instrumentation_call (cfg, mono_profiler_method_leave);
+
                                        MONO_INST_NEW_CALL (cfg, call, OP_JMP);
                                        call->tail_call = TRUE;
                                        call->method = cmethod;
@@ -8682,6 +8706,8 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
                                        cfg->ret_var_set = TRUE;
                                } 
                        } else {
+                               emit_instrumentation_call (cfg, mono_profiler_method_leave);
+
                                if (cfg->lmf_var && cfg->cbb->in_count)
                                        emit_pop_lmf (cfg);
 
@@ -12179,6 +12205,9 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
                emit_push_lmf (cfg);
        }
 
+       cfg->cbb = init_localsbb;
+       emit_instrumentation_call (cfg, mono_profiler_method_enter);
+
        if (seq_points) {
                MonoBasicBlock *bb;
 

diff --git a/mono/mini/mini-amd64.c b/mono/mini/mini-amd64.c
index 9b1c7bdb181dec8163bc7db0ac7841b2c2bb0182..b5fed14d8d22f584ac059dfff08b2e0dff1a9586 100644 (file)
--- a/mono/mini/mini-amd64.c
+++ b/mono/mini/mini-amd64.c
@@ -4826,10 +4826,6 @@ mono_arch_output_basic_block (MonoCompile *cfg, MonoBasicBlock *bb)
                        MonoCallInst *call = (MonoCallInst*)ins;
                        int i, save_area_offset;
 
-                       /* FIXME: no tracing support... */
-                       if (cfg->prof_options & MONO_PROFILE_ENTER_LEAVE)
-                               code = mono_arch_instrument_epilog_full (cfg, mono_profiler_method_leave, code, FALSE, TRUE);
-
                        g_assert (!cfg->method->save_lmf);
 
                        /* Restore callee saved registers */

diff --git a/mono/mini/mini-x86.c b/mono/mini/mini-x86.c
index d34fc64190b117be978ac35aaeff74a600e684ba..02a6d1daa781c7916d546236bdc4ad61452d0368 100644 (file)
--- a/mono/mini/mini-x86.c
+++ b/mono/mini/mini-x86.c
@@ -3186,9 +3186,6 @@ mono_arch_output_basic_block (MonoCompile *cfg, MonoBasicBlock *bb)
                        ins->flags |= MONO_INST_GC_CALLSITE;
                        ins->backend.pc_offset = code - cfg->native_code;
 
-                       /* FIXME: no tracing support... */
-                       if (cfg->prof_options & MONO_PROFILE_ENTER_LEAVE)
-                               code = mono_arch_instrument_epilog (cfg, mono_profiler_method_leave, code, FALSE);
                        /* reset offset to make max_len work */
                        offset = code - cfg->native_code;
 

diff --git a/mono/mini/mini.c b/mono/mini/mini.c
index 3a9ff6b7c2765f9c91d810df2d9cca58f980cf2b..c1d5687132397cae5be8a4e2bccd06dc353bb75c 100644 (file)
--- a/mono/mini/mini.c
+++ b/mono/mini/mini.c
@@ -4102,9 +4102,6 @@ mono_codegen (MonoCompile *cfg)
 
        code = mono_arch_emit_prolog (cfg);
 
-       if (cfg->prof_options & MONO_PROFILE_ENTER_LEAVE)
-               code = mono_arch_instrument_prolog (cfg, mono_profiler_method_enter, code, FALSE);
-
        cfg->code_len = code - cfg->native_code;
        cfg->prolog_end = cfg->code_len;
 
@@ -4120,14 +4117,6 @@ mono_codegen (MonoCompile *cfg)
 
                if (bb == cfg->bb_exit) {
                        cfg->epilog_begin = cfg->code_len;
-
-                       if (cfg->prof_options & MONO_PROFILE_ENTER_LEAVE) {
-                               code = cfg->native_code + cfg->code_len;
-                               code = mono_arch_instrument_epilog (cfg, mono_profiler_method_leave, code, FALSE);
-                               cfg->code_len = code - cfg->native_code;
-                               g_assert (cfg->code_len < cfg->code_size);
-                       }
-
                        mono_arch_emit_epilog (cfg);
                }
        }
@@ -7429,8 +7418,17 @@ mini_init (const char *filename, const char *runtime_version)
        mono_marshal_init ();
 
        mono_arch_register_lowlevel_calls ();
-       register_icall (mono_profiler_method_enter, "mono_profiler_method_enter", NULL, TRUE);
-       register_icall (mono_profiler_method_leave, "mono_profiler_method_leave", NULL, TRUE);
+
+       /*
+        * It's important that we pass `TRUE` as the last argument here, as
+        * it causes the JIT to omit a wrapper for these icalls. If the JIT
+        * *did* emit a wrapper, we'd be looking at infinite recursion since
+        * the wrapper would call the icall which would call the wrapper and
+        * so on.
+        */
+       register_icall (mono_profiler_method_enter, "mono_profiler_method_enter", "void ptr", TRUE);
+       register_icall (mono_profiler_method_leave, "mono_profiler_method_leave", "void ptr", TRUE);
+
        register_icall (mono_trace_enter_method, "mono_trace_enter_method", NULL, TRUE);
        register_icall (mono_trace_leave_method, "mono_trace_leave_method", NULL, TRUE);
        register_icall (mono_get_lmf_addr, "mono_get_lmf_addr", "ptr", TRUE);