x86 jitstress / gcstress failure with methods that do not return

[AndyAyersMS]

Finally think I have gotten to the root cause of the raft of failures seen in x86 jitstress=2 gcstress=0xC zapdisable=1 runs discussed over in dotnet/coreclr#17330. The case I have been looking at is baseservices\threading\waithandle\waitall\waitallex3 -- the other waitall tests should fail for similar reasons.

The key method here is System.Threading.WaitHandle:WaitAll(ref,int,bool):bool. This method is normally prejitted but with zapdisable=1 it gets jitted during this run. Jitstress kicks in and via STRESS_BB_PROFILE ends up altering the block layout. So we end up not moving BBJ_THROW blocks to the end of the method.

That leaves us with this sequence of generated code:

07150225 8b4304          mov     eax,dword ptr [ebx+4]      << -- ESI dead here per gc info
07150228 0580000000      add     eax,80h
0715022d 3bc6            cmp     eax,esi
0715022f 7ec3            jle     System_Private_CoreLib!System.Threading.WaitHandle.WaitAll(System.Threading.WaitHandle[], Int32, Boolean)+0x174 (071501f4)
;; jit knows this call does not return
07150231 ff151c1e4205    call    dword ptr ds:[5421E1Ch] (System.Threading.WaitHandle.ThrowAbandonedMutexException(), mdToken: 06002692)
;; join
07150237 8945f0          mov     dword ptr [ebp-10h],eax            <<< ESI live here
0715023a 8b548608        mov     edx,dword ptr [esi+eax*4+8]

This method also has fully interruptible GC reporting. The jit knows ThrowAbandonedMutexException does not return and reflects that in its liveness calculations. So ESI is not live at the call but becomes live right after the call.

During the run at the point of failure, this method has just called ThrowAbandonedMutexException
GC stress kicks off a GC in the prestub. The VM walks the stack for GC and reports that the EIP for this frame is 07150237. When enumerating GC roots, it reports ESI as live. But when in the callee, ESI is not in fact live and contains the scalar value 0x80. So the reporting triggers an assert for an invalid objref.

The logic in EECodeManager::EnumGcRefs knows that for stack locations liveness can change across a call, but it (implicitly?) assumes that for callee-saves registers that liveness before/after the call should be the same. So when reporting it looks at the after call liveness state.

Possible fixes here include:

• modify EnumGcRefs to make the same curOffs adjustment for non-active fully interruptible callee save registers that it does for stack locations, to use their "in call" liveness state.
• modify the jit to pad after normal user calls that it knows do not return so that the before and after liveness state of callee saves registers is the same. I have this prototyped and suspect it will be pretty surgical.

There are still a few loose ends to chase down:

• why we didn't notice this long ago, as the does not return logic has been in the jit a while
• why this only impacts x86
• whether or not this situation can arise without jitstress

[AndyAyersMS]

cc @RussKeldorph @dotnet/jit-contrib

[mikedn]

Hrm, this looks pretty ugly.

During the run at the point of failure, this method has just called ThrowAbandonedMutexException
GC stress kicks off a GC in the prestub. The VM walks the stack for GC and reports that the EIP for this frame is 07150237. When enumerating GC roots, it reports ESI as live. But when in the callee, ESI is not in fact live and contains the scalar value 0x80. So the reporting triggers an assert for an invalid objref.

Presumably this does not happen when the throw helper call is invoked directly because the VM somehow recognizes it and understands that no registers can be live after it?

modify the jit to pad after normal user calls that it knows do not return so that the before and after liveness state of callee saves registers is the same. I have this prototyped and suspect it will be pretty surgical.

By "pad" do you mean emitting an int 3?

[AndyAyersMS]

There is some logic I don't fully understand in codegenlinear to emit 'int3' padding after BBJ_THROW blocks that tends to place the padding between the throw blocks that move to the end of the method for x86.

https://github.com/dotnet/coreclr/blob/d1f49cc4b26d28fdb1c4f3bf147668e13b8995eb/src/jit/codegenlinear.cpp#L587-L606

For example:

; Assembly listing for method Microsoft.Win32.RegistryKey:InternalGetValue(ref,ref,bool,bool):ref:this
...
G_M38396_IG59:
       mov      ecx, gword ptr [esi+8]
       mov      edx, 42
       call     System.ThrowHelper:ThrowObjectDisposedException(ref,int)
       int3     
G_M38396_IG60:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     

But seemingly, BB stress messes with that logic.

My codegen-based prototype fix just extends this to include calls that do not return:

                // Do likewise for blocks that end in DOES_NOT_RETURN calls
                // that were not caught by the above rules.
                else
                {
                    GenTree* call = block->lastNode();

                    if ((call != nullptr) && (call->gtOper == GT_CALL))
                    {
                        if ((call->gtCall.gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0)
                        {
                            instGen(INS_BREAKPOINT); // This should never get executed
                        }
                    }
                }

[AndyAyersMS]

Linking this to #9964 for cross-ref purposes, since I'm close to closing that out as the issues there have been parcelled out...

[AndyAyersMS]

Here is a repro that does not require any stress; it fails on x86 checked when compiled with jit optimizations enabled:

using System;
using System.Runtime.CompilerServices;

// Repro case for CoreCLR 17398

class X
{
    static int v;

    string s;

    public override string ToString() => s;

    [MethodImpl(MethodImplOptions.NoInlining)]
    X(int x)
    {
        s = "String" + x;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void F() { }

    public static void T0(object o, int x)
    {
        GC.Collect(2);
        throw new Exception(o.ToString());
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static void Test()
    {
        object x1 = new X(1);
        object x2 = new X(2);
        
        if (v == 1)
        {
            // Generate enough pressure here to 
            // kill ESI so in linear flow it is dead
            // at the call to T0
            int w = v;
            int x = v;
            int y = v;
            int z = v;

            // Unbounded loop here forces fully interruptible GC
            for (int i = 0; i < v; i++)
            {
                w++;
            }

            T0(x2, w + x + y + z);
        }

        // Encourage x1 to be in callee save (ESI)
        F();

        if (v == 2)
        {
            T0(x1, 0);
        }
    }

    public static int Main()
    {
        v = 1;
        int r = 0;

        try
        {
            Test();
        }
        catch (Exception e)
        {
            Console.WriteLine(e.Message);
            r = 100;
        }

        return r;
    }
}

the problematic stretch of code is at the end:

G_M27632_IG06:
       03F3         add      esi, ebx
       03F2         add      esi, edx
       8BD6         mov      edx, esi
       0355F0       add      edx, dword ptr [ebp-10H]
       8BCF         mov      ecx, edi
       FF15B4413602 call     [X:T0(ref,int)]   <-- gc here asserts; ESI is reported live

G_M27632_IG07:
       8BCE         mov      ecx, esi
       33D2         xor      edx, edx
       FF15B4413602 call     [X:T0(ref,int)]
       CC           int3

with the following error:

Assert failure(PID 9280 [0x00002440], Thread: 5992 [0x1768]): 
!CREATE_CHECK_STRING(!"Detected use of a corrupted OBJECTREF. Possible GC hole.")

CORECLR! Object::ValidateInner + 0x55D (0x0f7b58fd)
CORECLR! Object::Validate + 0x148 (0x0f7b52f8)
CORECLR! Object::ValidatePromote + 0xB (0x0f7b5956)
CORECLR! WKS::GCHeap::Promote + 0x7A (0x0fcba63a)

[mikedn]

why this only impacts x86

It looks like at least in your (great!) repro this is exactly due to

modify EnumGcRefs to make the same curOffs adjustment for non-active fully interruptible callee save registers that it does for stack locations, to use their "in call" liveness state.

It seems odd that x86 doesn't use the same technique. Though this results in rcx and rdi being reported as live when they're really aren't. That seems to be correct but a bit more conservative.

[briansull]

Yes, the reporting of register liveness for fully interruptible regions is incorrect. What should be reported is the registers that are alive before the call was made, not what is live at the return address. (i.e after the call)

[erozenfeld]

I tried a fix that modifies the x86 verison of EECodeManager::EnumGcRefs (the one under #ifndef USE_GC_INFO_DECODER) to do the curOffs adjustment for callee saves that are not on active stack frame (similar to what's already done for stack vars). That fixed the repro case @AndyAyersMS provided above but waitallex3 still failed with

Assert failure(PID 17752 [0x00004558], Thread: 18344 [0x47a8]): !CREATE_CHECK_STRING(!"Detected use of a corrupted OBJECTREF. Possible GC hole.")

CORECLR! Object::ValidateInner + 0x3CB (0x0f2a4d9b)
CORECLR! Object::Validate + 0xD3 (0x0f2a48a3)
CORECLR! Object::ValidatePromote + 0x15 (0x0f2a4e25)
CORECLR! WKS::GCHeap::Promote + 0xC9 (0x0faa5ae9)
CORECLR! GcEnumObject + 0x96 (0x0f982686)
CORECLR! EECodeManager::EnumGcRefs + 0xD8E (0x0f6cdc8e)
CORECLR! GcStackCrawlCallBack + 0x1F8 (0x0f982a78)
CORECLR! Thread::MakeStackwalkerCallback + 0x53 (0x0f42bc33)
CORECLR! Thread::StackWalkFramesEx + 0x1A0 (0x0f42e610)
CORECLR! Thread::StackWalkFrames + 0x2C3 (0x0f42e453)
    File: f:\coreclr4\src\vm\object.cpp Line: 806
    Image: F:\coreclr4\bin\tests\Windows_NT.x86.Checked\Tests\Core_Root\CoreRun.exe

but at a different point. GC happened when WaitAllEx:CreateArray was calling System.Threading.EventWaitHandle:.ctor
and the root causing the assert was one of the stack locations (containing 0xffffffff)

G_M7962_IG29:
       6A01         push     1

G_M7962_IG30:
       89442414     mov      gword ptr [esp+10H+04H], eax

G_M7962_IG31:
       8BC8         mov      ecx, eax

G_M7962_IG32:
       33D2         xor      edx, edx

G_M7962_IG33:
       FF15A8C45D09 call     [System.Threading.EventWaitHandle:.ctor(bool,int):this]

I'm investigating this.
@AndyAyersMS please let me know if you've seen this while investigating the GCStress failures.

[AndyAyersMS]

No, I haven't seen this kind of failure.

[mikedn]

I tried a fix that modifies the x86 verison of EECodeManager::EnumGcRefs (the one under #ifndef USE_GC_INFO_DECODER) to do the curOffs adjustment for callee saves that are not on active stack frame (similar to what's already done for stack vars).

Did you adjust curOffs ONLY when reporting registers? The current code handles both registers and pushed args and it looks like the adjustment must not be applied to pushed args, otherwise the same reference will be reported twice.

[erozenfeld]

otherwise the same reference will be reported twice.

I don't see why. Can you explain?

[mikedn]

What I tried was to use curOffs - 1 for inactive functions here: https://github.com/dotnet/coreclr/blob/a788a72d5ac56ceb8285d14c87f9ccddc229be34/src/vm/eetwain.cpp#L4439

Callee saved registers were reported correctly (well, it seemed so) but waitallex3 still failed, though in a different place than you show in your previous post. For me it failed in FillStringChecked due to an object reference that was pointing to bogus memory.

The broken reference was the last parameter of FillStringChecked, the one that gets passed through the stack. When curOffs - 1 is used, that parameter stack location gets reported twice - once from inside (active) FillStringChecked and once from its caller (GetUniqueName) because it thinks that it's still alive. This does not happen if curOffs is not adjusted.

As far as I can tell it's not correct to report the same object reference location twice. It works during promotion but it fails during relocation because the first time it is reported the reference is relocated correctly and the second time the GC attempt to relocate it again and sends it to Mars...

I got waitallex3 to pass by using curOffs - 1 only for register reporting and curOffs for argument reporting. Basically I made a second call to scanArgRegTableI using curOffs before reporting the arguments.

[erozenfeld]

I did something similar. I replicated this logic:
https://github.com/dotnet/coreclr/blob/a788a72d5ac56ceb8285d14c87f9ccddc229be34/src/vm/eetwain.cpp#L4793-L4809
to set curOffs on this line: https://github.com/dotnet/coreclr/blob/a788a72d5ac56ceb8285d14c87f9ccddc229be34/src/vm/eetwain.cpp#L4439

[mikedn]

Yep, I did the same thing (except the willContinueExecution else which I suspect that it is not necessary). See my updated post, I pressed enter too early and you may have read an incomplete version of it.