RyuJIT x86: EBP frame is sometimes required if overflow operations are present

[mikedn]

Found in dotnet/coreclr#4986. An assert fires in test JIT\Regression\JitBlue\DevDiv_200492\DevDiv_200492\DevDiv_200492.cmd when an overflow operation is used as a call argument:

G_M30135_IG02:
       33C0         xor      eax, eax
       50           push     eax                                   ; push loResult
       83FA00       cmp      edx, 0                                ; overflow check for hiResult
       7C0A         jl       SHORT G_M30135_IG04                   ; exit bb
       8BC2         mov      eax, edx
       50           push     eax                                   ; push hiResult
       FF15FC7BF902 call     [TestUlongAttribute:Store(long):this]
G_M30135_IG03:
       C3           ret      
G_M30135_IG04:
       E87199AA50   call     CORINFO_HELP_OVERFLOW
       CC           int3     

I doubt that it's valid to throw when esp doesn't point to the current frame and that's probably what the assert is trying to tell. Hoisting the overflow check out of the call is unlikely to work in the general case as that will affect evaluation order so I guess that such methods require a ebp frame.

[mikedn]

@RussKeldorph Tags?

[RussKeldorph]

@mikedn Missed it. Thanks for the heads up.

[mikedn]

so I guess that such methods require a ebp frame

Turns out that JIT32 solves this by adjusting esp before throwing:

G_M55886_IG02:
       8D4101       lea      eax, [ecx+1]
       50           push     eax
       8D4202       lea      eax, [edx+2]
       50           push     eax
       8BC1         mov      eax, ecx
       03C2         add      eax, edx
       7008         jo       SHORT G_M55886_IG04
       50           push     eax
       FF159C51CD00 call     [Program:Sink(int,int,int,int,int):int]

G_M55886_IG03:
       C3           ret

G_M55886_IG04:
       83C408       add      esp, 8 ; pop the 2 arguments that were already pushed
       E882F15453   call     CORINFO_HELP_OVERFLOW
       CC           int3

This adjustment is performed by a piece of code that exists in the legacy version of genCodeForBBlist but not in the RyuJIT version.

And of course, this is not specific to long, same problem happens if you use only int expressions:

class Program
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Sink(int a, int b, int c, int d, int e) => a + b + c + d + e;

    static int Test(int a, int b) => Sink(a, b, (a + 1), (b + 2), checked(a + b));

    static void Main(string[] args) => Test(2, int.MaxValue);
}

But this example generates bad code without generating an assert. So long appears to have some sort of problem of its own.

[mikedn]

So long appears to have some sort of problem of its own.

Yeah, for long the stack level of the throw block is correctly determined during morphing but morph doesn't know that the throw block will be jumped to when only half of the argument has been pushed onto the stack.

So, we actually have two rather different issues:

• Codegen doesn't adjust the stack pointer when needed and this may silently lead to an incomplete exception call stack and who knows what other issues. This seems easy to fix, just copy the adjustment code from the legacy codegen.
• When passing long arguments the argument should be fully computed before its lo and hi parts are pushed. This is very similar to dotnet/coreclr#5957 and probably both issues can be fixed at the same time.