GCStress: Fix a race-condition

[swaroop-sridhar]

This change ensures that calls to CORINFO_HELP_STOP_FOR_GC() themselves
are not converted to GC-Stress traps -- thus preventing the race between
the GC thread, and a managed thread under GCStress.

Identification of calls to CORINFO_HELP_STOP_FOR_GC():
Since this is a GCStress only requirement, its not worth special identification in the GcInfo
Since CORINFO_HELP_STOP_FOR_GC() calls are realized as indirect calls by the JIT, we cannot identify
them by address at the time of SprinkleBreakpoints().
So, we actually let the SprinkleBreakpoints() replace the call to CORINFO_HELP_STOP_FOR_GC()
with a trap, and revert it back to the original instruction the first time we hit the trap in
OnGcCoverageInterrupt().

Upside: No change to GCInfo or JIT is necessary
Downside: Need to decode a few bytes on every GCStress interrupt.

Local testing with COMPlus_GCStress=0xC and COMPlus_GCConcurrent=1 did not encounter the race condition in #4794

Fixes #4794

[swaroop-sridhar]

@sergiy-k / @jkotas please review.
+CC: @RussKeldorph

[jkotas]

Instead of doing all this call target decoding, would it be better to just check whether we are in preemptive mode?

The call target decoding will need ongoing tweaks, e.g. I hope to add support for interop transitions for ReadyToRun (based on CORJIT_FLG2_USE_PINVOKE_HELPERS) - that will use a different helper.

[swaroop-sridhar]

I just spoke with @jkotas.
The check for PremptiveMode will not work, because when returning from a PInvoke epilog, the thread is already in cooperative mode.

[jkotas]

Can we also delete m_fPreemptiveGCDisabledForGCStress now? It solving the same problem - it should be unnecessary now with this fix.

[swaroop-sridhar]

Yes, we can remove the m_fPreemptiveGCDisabledForGCStress functionality, separate from this fix.

In the case of PInvoke,

mov      byte  ptr[rsi + 12], 0   // Switch to preemptive mode [thread->premptiveGcDisabled = 0]
cli          // INTERRUPT_INSTR_CALL in place of the actual native call 
mov      byte  ptr[rsi + 12], 1   // Switch the thread to Cooperative mode
cmp      dword ptr[(reloc 0x7ffd1bb77148)], 0  // if(g_TrapReturningThreads)
je       SHORT G_M40565_IG05
cli       // INTERRUPT_INSTR_CALL in place of COREINFO_STOP_FOR_GC()

We currently do two GCStress collections:

1. At the native call
2. At the call to COREINFO_STOP_FOR_GC()

(1) is not wrong, but is unnecessary coverage, because that code will never trigger a GC during normal execution.
(2) is definitely wrong and will be fixed by this change,

So, I agree that its a good idea to remove this m_fPreemptiveGCDisabledForGCStress and simplify the gccoverage implemetation. I think it is a good idea to do it as a separate checkin.

[swaroop-sridhar]

@dotnet-bot test Windows_NT x64 Checked gcstress0xc

[swaroop-sridhar]

@dotnet-bot test Windows_NT Checked arm64

[swaroop-sridhar]

The latest commit is to fix an X86 build break, by fixing an extern "C" declaration here: https://github.com/dotnet/coreclr/pull/5356/files#diff-4cbc7cebee869048dc921baad12c9240R420

[swaroop-sridhar]

@dotnet-bot test Windows_NT x64 Checked gcstress0xc

[swaroop-sridhar]

@dotnet-bot test Windows_NT Checked arm64

[swaroop-sridhar]

@dotnet-bot test Windows_NT Checked arm

[swaroop-sridhar]

@dotnet-bot test Windows_NT Checked x64 gcstress0xc

[swaroop-sridhar]

@dotnet-bot test Windows_NT gcstress0xc

[swaroop-sridhar]

@dotnet-bot test Windows_NT arm64 Checked

[swaroop-sridhar]

@dotnet-bot test Windows_NT arm Checked

[swaroop-sridhar]

@jkotas: Checking if you had any more corrections.
Most of the testing has passed. The GCStress leg is still running, and hasn't hit the race.

[swaroop-sridhar]

@dotnet-bot test Windows_NT gcstress0xc

[swaroop-sridhar]

The failures in GCStress are pres-existing / timeouts.

[jkotas]

LGTM

[swaroop-sridhar]

Looks like this change also fixes https://github.com/dotnet/coreclr/issues/5310 and https://github.com/dotnet/coreclr/issues/2785