JitStress STRESS_OPT_BOOLS_GC transformation questionable on xarch in face of GC Stress

[AndyAyersMS]

Under JitStress, optOptimizeBoolsGcStress may transform x == null into (x | x) == 0 or (x & x) == 0 for gc-typed x. However the casting out of GC is done in a way that can lead to a hole in GC, as the xarch code generator will implement t = x | x; t == 0 as t = x; t |= x; t == 0, and since t is typed non-gc a GC interrupt between the first two instructions can relocate x and so change the sense of the comparison.

This may explain many of the GC stress failures seen in the x86 gcstress=0xC jitstress=2 legs, for instance this one.

I have not yet confirmed this is the cause of the stress failures in those tests, but will update this bug if/when I do.

One such example (with zapdisable is) System.Threading.WaitHandle:WaitAll(ref,int,bool):bool on x86, where there is a null check right at the start of the method, and the jit generates:

; prolog
push    ebp
mov     ebp,esp
push    edi
push    esi
push    ebx
push    eax
mov     esi,ecx
mov     edi,edx          // GC info +ESI
; start of method body
mov     ecx,esi
or      ecx,esi          // GC interrupt here can change ESI and not ECX
jne     ...

This is split off from the ongoing efforts to triage and fix up gc stress in #9964 / dotnet/coreclr#17330.

cc @RussKeldorph @dotnet/jit-contrib

category:testing
theme:testing
skill-level:expert
cost:small

[mikedn]

and since t is typed non-gc a GC interrupt between the first two instructions can relocate x and so change the sense of the comparison.

Hmm, I don't follow

• The GC can't turn a null reference into a non-null reference
• The GC can't turn a non-null reference into a null reference, except in the very special case of weak references and as far as I can tell their implementation is unlikely to cause problems in this case

So how would changing ESI but not ECX would affect the result of the comparison?

[AndyAyersMS]

I should update the example. There's a 50/50 chance jit stress will use and instead of or for this, and in the and case it can produce a different result.

But I haven't proved this is what leads to the jit stress failure yet, and it does seem like a stretch, as if most of these null checks guard throws, so if we get the sense wrong, we should see unexpected exceptions, not

Assert failure(PID 8832 [0x00002280], Thread: 14752 [0x39a0]): 
!CREATE_CHECK_STRING(!"Detected use of a corrupted OBJECTREF. Possible GC hole.")

In the case I am looking at ESI is corrupted while WaitAll is running, but this corruption is detected later on in the method, well after this null check.

[mikedn]

There's a 50/50 chance jit stress will use and instead of or for this, and in the and case it can produce a different result.

Hmm, and will never work in this case, with or without GC updating references.

In the case I am looking at ESI is corrupted while WaitAll is running, but this corruption is detected later on in the method, well after this null check.

Could it be that the result of OR is somehow reported as a GC reference?

[mikedn]

Hmm, and will never work in this case, with or without GC updating references.

Well, it works in the specific case of x & x but in the general case & only works for 0/1 values and optOptimizeBools accounts for that: https://github.com/dotnet/coreclr/blob/55f162d21d396a7741a91f00568d48a2f3fab603/src/jit/optimizer.cpp#L9149-L9154

So optOptimizeBoolsGcStress's random use of AND seems pointless, such IR cannot be normally generated (unless imported from invalid IL).

[AndyAyersMS]

The test I'm looking at --- baseservices\threading\waithandle\waitall\waitallex3 -- still fails with this bit of stress disabled.

So pushing this back out to future.