Update SKIP_ALLOC_FRAME to match 3.1, 5.0 and 6.0 instruction sequences in function prolog by echesakov · Pull Request #45184 · dotnet/runtime

echesakov · 2020-11-25T04:57:55Z

~~First, let's confirm that the added test fails.~~ It does

The added test is designed in a way that exposes an issue as in #45090.

In order to do this, AllocLocal methods must have esp-based frames. Most of the methods would require to call a stack probe helper, but some would use inlined stack probing sequence as it is demonstrated in an example below.

There are three cases that should be considered in SKIP_ALLOC_FRAME in order for the unwinder to work properly:

frameSize <= 0xC00 - a function prolog contains sub esp, frameSize - no probing necessary

; Assembly listing for method Runtime_45090._00000800:AllocLocal(int):int
; Emitting BLENDED_CODE for Pentium 4 - Windows
; optimized code
; esp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  3,  3   )     int  ->  ecx
;  V01 loc0         [V01,T01] (  1,  1   )  struct (2052) [esp+0x00]   do-not-enreg[SF] ld-addr-op
;
; Lcl frame size = 2052

G_M11545_IG01:              ;; offset=0000H
       81EC04080000 sub      esp, 0x804
                                                ;; bbWeight=1    PerfScore 0.25
G_M11545_IG02:              ;; offset=0006H
       8BC1         mov      eax, ecx
       030424       add      eax, dword ptr [esp]
                                                ;; bbWeight=1    PerfScore 1.25
G_M11545_IG03:              ;; offset=000BH
       81C404080000 add      esp, 0x804
       C3           ret
                                                ;; bbWeight=1    PerfScore 1.25

; Total bytes of code 18, prolog size 6, PerfScore 4.55, instruction count 5 (MethodHash=ccc1d2e6) for method Runtime_45090._00000800:AllocLocal(int):int
; ============================================================

0xC00 < frameSize < 0x1000 - a function prolog contains sub esp, frameSize followed by test eax, [esp] - the latter ensure that [esp+1024] always points to either already committed page or guard page.

; Assembly listing for method Runtime_45090._00000E00:AllocLocal(int):int
; Emitting BLENDED_CODE for Pentium 4 - Windows
; optimized code
; esp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  3,  3   )     int  ->  ecx
;  V01 loc0         [V01,T01] (  1,  1   )  struct (3588) [esp+0x00]   do-not-enreg[SF] ld-addr-op
;
; Lcl frame size = 3588

G_M17060_IG01:              ;; offset=0000H
       81EC040E0000 sub      esp, 0xE04
       850424       test     eax, dword ptr [esp]
                                                ;; bbWeight=1    PerfScore 2.25
G_M17060_IG02:              ;; offset=0009H
       8BC1         mov      eax, ecx
       030424       add      eax, dword ptr [esp]
                                                ;; bbWeight=1    PerfScore 1.25
G_M17060_IG03:              ;; offset=000EH
       81C4040E0000 add      esp, 0xE04
       C3           ret
                                                ;; bbWeight=1    PerfScore 1.25

; Total bytes of code 21, prolog size 9, PerfScore 6.85, instruction count 6 (MethodHash=9d80bd5b) for method Runtime_45090._00000E00:AllocLocal(int):int
; ============================================================

frameSize > 0x1000 - a function prolog contains a call to stack probe helper

; Assembly listing for method Runtime_45090._00005000:AllocLocal(int):int
; Emitting BLENDED_CODE for Pentium 4 - Windows
; optimized code
; esp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  3,  3   )     int  ->  ecx
;  V01 loc0         [V01,T01] (  1,  1   )  struct (20484) [esp+0x00]   do-not-enreg[SF] ld-addr-op
;
; Lcl frame size = 20484

G_M55284_IG01:              ;; offset=0000H
       8D8424FCAFFFFF lea      eax, [esp-5004H]
       E8CADD6E5A   call     CORINFO_HELP_STACK_PROBE
       8BE0         mov      esp, eax
                                                ;; bbWeight=1    PerfScore 1.75
G_M55284_IG02:              ;; offset=000EH
       8BC1         mov      eax, ecx
       030424       add      eax, dword ptr [esp]
                                                ;; bbWeight=1    PerfScore 1.25
G_M55284_IG03:              ;; offset=0013H
       81C404500000 add      esp, 0x5004
       C3           ret
                                                ;; bbWeight=1    PerfScore 1.25

; Total bytes of code 26, prolog size 14, PerfScore 6.85, instruction count 7 (MethodHash=3727280b) for method Runtime_45090._00005000:AllocLocal(int):int
; ============================================================

Dotnet-GitSync-Bot · 2020-11-25T04:57:58Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

echesakov · 2020-11-25T19:45:12Z

@janvorli @jkotas Can you please take a look? What would you suggest for testing the change other than running GCStress test legs?

jkotas · 2020-11-25T21:46:35Z

src/coreclr/src/vm/eetwain.cpp

Should this be hard-coded number instead of the OS page size to avoid problem with OS page size changing between AOT time and JIT time?

Yes, it should. Thanks for catching this.

Hmm, if I take an approach as discussed in another PR - that this should match target page size for JIT-ed code and 0x1000 for R2R code, right?

The runtime assumes that the unwinding algorithm is same for all code flavors.

jkotas · 2020-11-25T21:50:24Z

src/coreclr/src/vm/eetwain.cpp

The unwinding algorithm is part of R2R contract. This will require bumping the major R2R format version.

I never changed the R2R format version in the past. Is there any guideline I should follow to accomplish that?

It is about bumping the version number in https://github.com/dotnet/runtime/blob/master/src/coreclr/inc/readytorun.h and other places this file points to.

There are two ways how to evolve the R2R contract:

Backward compatible: It means keeping both old and new version of the unwinding algorithm, and choosing the right algortihm based on the R2R format version in the given image

Non-backward compatible (ie breaking): We will fallback to JIT for the older R2R images.
We prefer the backward compatible evolution when we have a choice.

@dotnet/crossgen-contrib Do you have thoughts on whether we want to do R2R version break for 6.0?

Actually, the simplest thing to do for this one may be to do support both old and new unwind sequences. The unwinding inspects the instruction stream and so it should be possible to handle both without passing any explicit versions around.

You will still want to change these numbers to:

#define READYTORUN_MAJOR_VERSION 0x0005 #define READYTORUN_MINOR_VERSION 0x0001

but that will still be backward compatible change.

Thank you for suggestion, Jan! I will rework this and add support for the old unwind sequence.

I am still validating the changes but so far I haven't seen any issues with 3.1 and 5.0 R2R images.

Don't forget to also include READYTORUN_MAJOR_VERSION bump that I have mentioned above.

Thanks for reminding me to do the bump - I updated both readytorun.h and ModuleHeaders.cs.

jkotas · 2020-11-25T21:54:31Z

src/tests/JIT/Regression/JitBlue/Runtime_45090/Runtime_45090.tt

What is special about this test that it cannot be written in C#?

It probably can. But it was simpler for me to express conditions for AllocLocal in order to have esp-based frame.

I rewrote the test in C# - turned out that it was enough to have a method as virtual to make inliner cooperate and not inline the method while keeping its frame esp based (I couldn't use NoInlining attribute since the method would have JIT_FLAG_FRAMED flag set and becomes ebp based).

echesakov · 2020-11-25T21:54:37Z

The test seem to expose some limitation in Mono runtime

  Starting:    JIT.Regression.XUnitWrapper
    JIT/Regression/JitBlue/Runtime_45090/Runtime_45090/Runtime_45090.sh [FAIL]
      
      Unhandled Exception:
      System.InvalidProgramException: Unable to run method 'int Runtime_45090.Program:Main (string[])': locals size too big.
      [ERROR] FATAL UNHANDLED EXCEPTION: System.InvalidProgramException: Unable to run method 'int Runtime_45090.Program:Main (string[])': locals size too big.
      
      Return code:      1
      Raw output file:      /private/tmp/helix/working/A65B09FB/w/ADB70993/e/JIT/Regression/Reports/JIT.Regression/JitBlue/Runtime_45090/Runtime_45090/Runtime_45090.output.txt
      Raw output:
      BEGIN EXECUTION
      /tmp/helix/working/A65B09FB/p/corerun Runtime_45090.dll ''
      Expected: 100
      Actual: 1
      END EXECUTION - FAILED

jkotas · 2020-11-25T21:55:40Z

What would you suggest for testing the change other than running GCStress test legs?

You can try to build a test that hits this crash even without GC stress.

echesakov · 2020-12-01T22:26:18Z

What would you suggest for testing the change other than running GCStress test legs?

You can try to build a test that hits this crash even without GC stress.

@jkotas Do you have any suggestion how to approach this? I was thinking about spawning a large number of threads all running AllocLocal methods in a loop and have a dedicated thread that would trigger GC.Collect once in a while, so the unwinder would walk the stacks of stopped AllocLocal methods.

jkotas · 2020-12-01T22:28:10Z

Yes, this sounds like the right approach.

ViktorHofer · 2020-12-08T14:50:43Z

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

echesakov · 2020-12-11T03:27:37Z

Yes, this sounds like the right approach.

I tried this approach by running methods allocating a local frame in different threads while triggering GC at the same time.
Although such test does fail sometimes, I could not make it deterministic and fail always, so the current implementation of the test still uses GCStress.

I also extended the test cases and I believe I covered all the different frame allocation methods that JIT can use on win-x86:
PushReg

G_M21059_IG01:              ;; offset=0000H
       50           push     eax
       33C0         xor      eax, eax
       890424       mov      dword ptr [esp], eax

NoProbe

G_M27846_IG01:              ;; offset=0000H
       57           push     edi
       81EC00010000 sub      esp, 256

InlineProbe

G_M31598_IG01:              ;; offset=0000H
       57           push     edi
       81EC000E0000 sub      esp, 0xE00
       850424       test     eax, dword ptr [esp]

HelperProbe

G_M56481_IG01:              ;; offset=0000H
       57           push     edi
       8D842400F0FFFF lea      eax, [esp-1000H]
       E849E90673   call     CORINFO_HELP_STACK_PROBE
       8BE0         mov      esp, eax

JulieLeeMSFT · 2020-12-16T20:43:36Z

Waiting for #10184 fix for full validation.

echesakov · 2020-12-18T05:48:41Z

Given that GCStress=8 doesn't currently work with R2R methods I tested the change manually.

Test methodology is to call one of the implementations of VirtMethodEspBasedFrame method in a thread and trigger GC from another thread. Then wait for debugger to stop at SKIP_ALLOC_FRAME and check that the final base+offset computed by that function points at an instruction after the stack allocation/probing instructions in a prolog. Repeat this for all implementations of the method and for R2R code produced by 3.1, 5.0 and master - total 24 scenarios.

echesakov · 2020-12-18T05:50:38Z

Triggered runtime-coreclr gcstress0x3-gcstress0xc pipeline to validate the change and that it fixes #45090

echesakov · 2020-12-18T19:25:57Z

gcstress windows x86 is clean - suggesting that the change fixes the original issue.

There are failures in gcstress windows x64 and gcstress linux arm pipelines - I triggered another gcstress on top of master.
Those were probably already fixed and appear in the first run since I triggered that first gcstress run manually in AzDO and hence didn't do rebasing on top of master.

jkotas

LGTM. Thank you!

jkotas · 2020-12-20T08:05:49Z

src/coreclr/jit/codegenxarch.cpp

Is JIT_FLAG_PROF_REJIT_NOPS still going to work with this change?

JIT_FLAG_PROF_REJIT_NOPS is not used currently. It may be best to delete it instead of keeping partially broken implementation around.

JIT_FLAG_PROF_REJIT_NOPS is not used currently. It may be best to delete it instead of keeping partially broken implementation around.

@jkotas Would it be fine with you if I undo the change to the jit file in this PR and come back with another PR removing JIT_FLAG_PROF_REJIT_NOPS flag and all the related infrastructure?

Fine with me.

Opened #46560 to address this

…jit/codegenxarch.cpp

…OC_FRAME in src/coreclr/vm/eetwain.cpp

…f R2R code in src/coreclr/vm/eetwain.cpp

…for 3.1 stack probing loop

…/tools/Common/Internal/Runtime/ModuleHeaders.cs

…coreclr/jit/codegenxarch.cpp" This reverts commit 0b88d71.

echesakov · 2021-01-04T23:03:30Z

Rebased on top of latest master and reverted the change src/coreclr/jit/codegenxarch.cpp.

echesakov added area-VM-coreclr arch-x86 os-windows labels Nov 25, 2020

echesakov linked an issue Nov 25, 2020 that may be closed by this pull request

Test failure: CheckInstrBytePattern(base[offset ] & 0xFD, 0x81, base[offset]) && CheckInstrBytePattern(base[offset+1] & 0xC0, 0xC0, base[offset+1]) #45090

Closed

echesakov mentioned this pull request Nov 25, 2020

JIT inline decisions might lead to StackOverflow during execution time #45186

Open

runfoapp bot mentioned this pull request Nov 25, 2020

Inability to unzip assets during build on Unix x64 #32805

Closed

echesakov marked this pull request as ready for review November 25, 2020 19:45

jkotas reviewed Nov 25, 2020

View reviewed changes

echesakov force-pushed the Runtime_45090 branch from fca8c7b to 4539436 Compare December 11, 2020 03:12

echesakov closed this Dec 11, 2020

echesakov reopened this Dec 11, 2020

runfoapp bot mentioned this pull request Dec 17, 2020

Can_Run_App_With_StatiHost installer test failure in CI #42101

Closed

echesakov changed the title ~~Update SKIP_ALLOC_FRAME to match new intrstruction sequence in function prolog~~ Update SKIP_ALLOC_FRAME to match 3.1, 5.0 and 6.0 instruction sequences in function prolog Dec 18, 2020

jkotas approved these changes Dec 20, 2020

View reviewed changes

echesakov added 2 commits January 4, 2021 13:41

Add regression tests for dotnet#45090

d73ae98

Remove padding for ReJit in CodeGen::genAllocLclFrame in src/coreclr/…

0b88d71

…jit/codegenxarch.cpp

echesakov added 10 commits January 4, 2021 13:41

Add instruction opcodes in src/coreclr/vm/eetwain.cpp

fdd1b78

Add SKIP_LEA_EAX_ESP in src/coreclr/vm/eetwain.cpp

07c0cb8

Add SKIP_HELPER_CALL in src/coreclr/vm/eetwain.cpp

1b0636e

Update SKIP_ALLOC_FRAME in src/coreclr/vm/eetwain.cpp

478c9e6

Use constant (0x1000) rather than calling GetOsPageSize() in SKIP_ALL…

c2ce8da

…OC_FRAME in src/coreclr/vm/eetwain.cpp

Draft of the changes that are back-compatible with earlier versions o…

3220654

…f R2R code in src/coreclr/vm/eetwain.cpp

Update src/coreclr/vm/eetwain.cpp and include missing SKIP_ARITH_REG …

d864022

…for 3.1 stack probing loop

Update test and include more scenarios

b5e1744

Update R2R version to 5.1 in src/coreclr/inc/readytorun.h src/coreclr…

3fd8d14

…/tools/Common/Internal/Runtime/ModuleHeaders.cs

Revert "Remove padding for ReJit in CodeGen::genAllocLclFrame in src/…

a2f2cc7

…coreclr/jit/codegenxarch.cpp" This reverts commit 0b88d71.

echesakov force-pushed the Runtime_45090 branch from 9221151 to a2f2cc7 Compare January 4, 2021 22:30

echesakov mentioned this pull request Jan 5, 2021

Remove JIT_FLAG_PROF_REJIT_NOPS #46560

Merged

echesakov merged commit 8b8a3fc into dotnet:master Jan 5, 2021

echesakov deleted the Runtime_45090 branch January 5, 2021 01:07

ghost locked as resolved and limited conversation to collaborators Feb 4, 2021

Conversation

echesakov commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dotnet-GitSync-Bot commented Nov 25, 2020

Uh oh!

echesakov commented Nov 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echesakov Dec 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echesakov Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echesakov Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echesakov commented Nov 25, 2020

Uh oh!

jkotas commented Nov 25, 2020

Uh oh!

echesakov commented Dec 1, 2020

Uh oh!

jkotas commented Dec 1, 2020

Uh oh!

ViktorHofer commented Dec 8, 2020

Uh oh!

echesakov commented Dec 11, 2020

Uh oh!

JulieLeeMSFT commented Dec 16, 2020

Uh oh!

echesakov commented Dec 18, 2020

Uh oh!

echesakov commented Dec 18, 2020

Uh oh!

echesakov commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echesakov commented Jan 4, 2021

Uh oh!

Reviewers

Assignees

echesakov commented Nov 25, 2020 •

edited

Loading

echesakov Dec 12, 2020 •

edited

Loading

echesakov Dec 18, 2020 •

edited

Loading

echesakov Dec 11, 2020 •

edited

Loading

echesakov commented Dec 18, 2020 •

edited

Loading