From: Bernhard Urban-Forster Date: Thu, 10 Oct 2019 09:21:23 +0000 (+0200) Subject: [mini] trace snippet should restore return value (mono/mono#17251) X-Git-Tag: submit/tizen/20210909.063632~10331^2~5^2~373 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=61e1eaae05214892c856d18acc67fb50bf01383c;p=platform%2Fupstream%2Fdotnet%2Fruntime.git [mini] trace snippet should restore return value (mono/mono#17251) * [mini] trace snippet should restore return value This is a problem on architectures with a floating point stack (x86). Without this PR and without `--trace` the managed-to-native wrapper for `Math.Ceiling` has a basic block like this at the end: ``` AFTER LOWER-VTYPE-OPTS 4: [IN: BB6(5) BB3(1), OUT: BB1(3) ] fmove R34 <- R16 ldaddr R35 <- R10 load_membase R37 <- [R35 + 0x0] store_membase_reg [R36] <- R37 il_seq_point il: 0x2d ``` `R16` contains the return value. `ldaddr`/`load_membase`/`store_membase_reg` is the sequence for LMF popping. `mono_spill_global_vars` does this with it: ``` SPILL BLOCK 4: fmove R34 <- R16 ff 34 16 0 loadr8_membase R34 <- [%ebp + 0xffffffd4] 1 nop ldaddr R35 <- R10 ii 35 5 1 add_imm R35 <- %ebp [-36] clobbers: 1 load_membase R37 <- [R35 + 0x0] ii 37 35 1 load_membase R37 <- [R35 + 0x0] store_membase_reg [R36] <- R37 ii -1 37 1 store_membase_reg [%edi] <- R37 il_seq_point il: 0x2d -1 1 il_seq_point il: 0x2d ``` The local register allocator takes care of the FP stack handling: ``` processing: 0 loadr8_membase %fr0 <- [%ebp + 0xffffffd4] [%fr0] processing: 0 load_membase %eax <- [%ebp + 0xffffffdc] [%fr0] processing: 0 store_membase_reg [%edi] <- %eax [%fr0] processing: 0 il_seq_point il: 0x2d [%fr0] ``` At the end of the basic block we have one value left on the FP stack. That's good as we are in the `cfg->bb_exit` basic block, so this is ought to be the return value. Let's enable `--trace=M:System.Math:Ceiling`. It will insert some code in order to copy arguments/return value to some datastructure that can then be printed by the trace code: ``` processing: 0 loadr8_membase %fr0 <- [%ebp + 0xffffffd4] [%fr0] processing: 0 iconst %eax <- [11116832] [%fr0] processing: 0 iconst %eax <- [52] [%fr0] processing: 0 localloc_imm %eax <- [%fr0] processing: 0 localloc_imm %ecx <- [%fr0] processing: 0 store_membase_reg [%eax + 0x30] <- %ecx [%fr0] processing: 0 x86_lea_membase %edx <- %ebp [%fr0] processing: 0 store_membase_reg [%ecx] <- %edx [%fr0] processing: 0 store_membase_imm [%eax + 0x28] <- [11116832] [%fr0] processing: 0 storer8_membase_reg [%ebp + 0xffffffcc] <- %fr0 [] processing: 0 x86_lea_membase %ecx <- %ebp [] processing: 0 store_membase_reg [%eax + 0x2c] <- %ecx [] processing: 0 store_membase_reg [%esp + 0x4] <- %eax [] processing: 0 store_membase_imm [%esp] <- [11116832] [] processing: 0 voidcall [mono_trace_leave_method] clobbers: c [] processing: 0 x86_lea_membase %eax <- %ebp [] processing: 0 load_membase %eax <- [%ebp + 0xffffffdc] [] processing: 0 store_membase_reg [%edi] <- %eax [] processing: 0 il_seq_point il: 0x2d [] ``` At the very beginning we push some value on the FP stack, same as before. However the code inserted by the tracing code will pop that value. Now, when returning to the, it will pop again something from the FP stack, but alas it's gonna be garbage. With this PR it looks like this: ``` processing: 0 loadr8_membase %fr0 <- [%ebp + 0xffffffd4] [%fr0] processing: 0 iconst %eax <- [11116832] [%fr0] processing: 0 iconst %eax <- [52] [%fr0] processing: 0 localloc_imm %eax <- [%fr0] processing: 0 localloc_imm %ecx <- [%fr0] processing: 0 store_membase_reg [%eax + 0x30] <- %ecx [%fr0] processing: 0 x86_lea_membase %edx <- %ebp [%fr0] processing: 0 store_membase_reg [%ecx] <- %edx [%fr0] processing: 0 store_membase_imm [%eax + 0x28] <- [11116832] [%fr0] processing: 0 storer8_membase_reg [%ebp + 0xffffffcc] <- %fr0 [] processing: 0 x86_lea_membase %ecx <- %ebp [] processing: 0 store_membase_reg [%eax + 0x2c] <- %ecx [] processing: 0 loadr8_membase %fr0 <- [%ebp + 0xffffffcc] [%fr0] processing: 0 store_membase_reg [%esp + 0x4] <- %eax [%fr0] processing: 0 store_membase_imm [%esp] <- [11116832] [%fr0] processing: 0 voidcall [mono_trace_leave_method] clobbers: c [%fr0] processing: 0 x86_lea_membase %eax <- %ebp [%fr0] processing: 0 load_membase %eax <- [%ebp + 0xffffffdc] [%fr0] processing: 0 store_membase_reg [%edi] <- %eax [%fr0] processing: 0 il_seq_point il: 0x2d [%fr0] ``` Leaving us the return value on the FP stack before returning. Fixes https://github.com/mono/mono/issues/16410 Commit migrated from https://github.com/mono/mono/commit/6ebf07f2cd4f71218c6a4a2ed5f855a5ac9ce9e3 --- diff --git a/src/mono/mono/mini/mini-profiler.c b/src/mono/mono/mini/mini-profiler.c index 6b2e676..e677dd6 100644 --- a/src/mono/mono/mini/mini-profiler.c +++ b/src/mono/mono/mini/mini-profiler.c @@ -52,13 +52,21 @@ emit_fill_call_ctx (MonoCompile *cfg, MonoInst *method, MonoInst *ret) MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STORE_MEMBASE_REG, alloc->dreg, MONO_STRUCT_OFFSET (MonoProfilerCallContext, method), method->dreg); if (ret) { - MonoInst *var = mono_compile_create_var (cfg, mono_method_signature_internal (cfg->method)->ret, OP_LOCAL); + MonoType *ret_type = mono_method_signature_internal (cfg->method)->ret; + MonoInst *var = mono_compile_create_var (cfg, ret_type, OP_LOCAL); MonoInst *store, *addr; EMIT_NEW_TEMPSTORE (cfg, store, var->inst_c0, ret); EMIT_NEW_VARLOADA (cfg, addr, var, NULL); MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STORE_MEMBASE_REG, alloc->dreg, MONO_STRUCT_OFFSET (MonoProfilerCallContext, return_value), addr->dreg); + + /* Work around a limitation of the register allocator regarding + * FP stack, see https://github.com/mono/mono/pull/17251 */ + if (cfg->backend->use_fpstack && (ret_type->type == MONO_TYPE_R8 || ret_type->type == MONO_TYPE_R4)) { + MonoInst *move_ret_back; + EMIT_NEW_VARSTORE (cfg, move_ret_back, ret, ret_type, var); + } } return alloc;