[mini] trace snippet should restore return value (mono/mono#17251)
authorBernhard Urban-Forster <lewurm@gmail.com>
Thu, 10 Oct 2019 09:21:23 +0000 (11:21 +0200)
committerGitHub <noreply@github.com>
Thu, 10 Oct 2019 09:21:23 +0000 (11:21 +0200)
commit61e1eaae05214892c856d18acc67fb50bf01383c
tree9bbeca42cabeb9823b226ad40a341acbe5e5134e
parente4b717a1ad151450bfae0f54106877b795b87ebd
[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
src/mono/mono/mini/mini-profiler.c