Fix x86 GC holes in xchg/rorx and GC suspension of spinning threads

src/coreclr/jit/codegenxarch.cpp

@@ -5660,15 +5660,8 @@ void CodeGen::genCodeForSwap(GenTreeOp* tree)
     varDsc1->SetRegNum(oldOp2Reg);
     varDsc2->SetRegNum(oldOp1Reg);
 
-    // Do the xchg
-    emitAttr size = EA_PTRSIZE;
-    if (varTypeIsGC(type1) != varTypeIsGC(type2))
-    {
-        // If the type specified to the emitter is a GC type, it will swap the GC-ness of the registers.
-        // Otherwise it will leave them alone, which is correct if they have the same GC-ness.
-        size = EA_GCREF;
-    }
-    inst_RV_RV(INS_xchg, oldOp1Reg, oldOp2Reg, TYP_I_IMPL, size);
+    // Do the xchg. The emitter swaps the GC-ness of the registers on its own, so the attribute doesn't matter.
+    inst_RV_RV(INS_xchg, oldOp1Reg, oldOp2Reg, TYP_I_IMPL, EA_PTRSIZE);
 
     // Update the gcInfo.
     // Manually remove these regs for the gc sets (mostly to avoid confusing duplicative dump output)

src/coreclr/jit/emitxarch.cpp

@@ -1919,12 +1919,9 @@ void emitter::emitHandleGCrefRegs(BYTE* dst, instrDesc* id)
 
         case IF_RRW_RRW:
         {
-            // This must be "xchg reg1, reg2"
+            // This must be "xchg reg1, reg2", swap the GC-ness of the registers if it differs.
             assert(id->idIns() == INS_xchg);
 
-            // If we got here, the GC-ness of the registers doesn't match, so we have to "swap" them in the GC
-            // register pointer mask.
-
             GCtype gc1, gc2;
 
             gc1 = emitRegGCtype(reg1);
@@ -15545,8 +15542,14 @@ BYTE* emitter::emitOutputSV(BYTE* dst, instrDesc* id, code_t code, CnsVal* addc)
             {
                 case IF_RWR_SRD: // Register Write, Stack Read
                 case IF_RRW_SRD: // Register Read/Write, Stack Read
+                case IF_RWR_SRD_CNS:
+                case IF_RRW_SRD_CNS:
+                case IF_RWR_SRD_RRD:
+                case IF_RRW_SRD_RRD:
                 case IF_RWR_RRD_SRD:
                 case IF_RRW_RRD_SRD:
+                case IF_RWR_RRD_SRD_CNS:
+                case IF_RWR_RRD_SRD_RRD:
                 {
                     emitGCregDeadUpd(id->idReg1(), dst);
                     break;
@@ -16026,8 +16029,15 @@ BYTE* emitter::emitOutputCV(BYTE* dst, instrDesc* id, code_t code, CnsVal* addc)
             {
                 case IF_RWR_MRD:
                 case IF_RRW_MRD:
+                case IF_RWR_MRD_CNS:
+                case IF_RRW_MRD_CNS:
+                case IF_RWR_MRD_RRD:
+                case IF_RRW_MRD_RRD:
                 case IF_RWR_RRD_MRD:
                 case IF_RRW_RRD_MRD:
+                case IF_RWR_RRD_MRD_CNS:
+                case IF_RWR_RRD_MRD_RRD:
+                case IF_RWR_MRD_OFF:
                 {
                     emitGCregDeadUpd(id->idReg1(), dst);
                     break;
@@ -16708,8 +16718,9 @@ BYTE* emitter::emitOutputRR(BYTE* dst, instrDesc* id)
         dst += emitOutputByte(dst, (0xC0 | regCode));
     }
 
-    // Does this instruction operate on a GC ref value?
-    if (id->idGCref())
+    // Does this instruction operate on a GC ref value? "xchg reg1, reg2" always swaps
+    // the GC-ness of its registers, regardless of the attribute it was emitted with.
+    if (id->idGCref() || (id->idInsFmt() == IF_RRW_RRW))
     {
         emitHandleGCrefRegs(dst, id);
     }

src/coreclr/vm/stackwalk.cpp

@@ -2537,7 +2537,7 @@ void StackFrameIterator::ProcessCurrentFrame(void)
             // Cache values which may be updated by CheckForSkippedFrames()
             m_cachedCodeInfo = m_crawl.codeInfo;
 
-            // On non-X86, we want to process the skipped explicit frames before the managed stack frame
+            // We want to process the skipped explicit frames before the managed stack frame
             // containing them.
             if (CheckForSkippedFrames())
             {
@@ -2573,10 +2573,8 @@ void StackFrameIterator::ProcessCurrentFrame(void)
 //    Returns true if there are skipped frames.
 //
 // Notes:
-//    x86 wants to stop at the skipped stack frames after the containing managed stack frame, but
-//    WIN64 wants to stop before.  I don't think x86 actually has any good reason for this, except
-//    because it doesn't unwind one frame ahead of time like WIN64 does.  This means that we don't
-//    have the caller SP on x86.
+//    Skipped frames are reported before the containing managed stack frame. On x86 this includes
+//    the InlinedCallFrame, which stays linked for the whole method.
 //
 
 BOOL StackFrameIterator::CheckForSkippedFrames(void)

src/coreclr/vm/threadsuspend.cpp

@@ -4620,6 +4620,13 @@ StackWalkAction SWCB_GetExecutionState(CrawlFrame *pCF, VOID *pData)
     }
 #endif // TARGET_X86
 
+    // On 32-bit targets and in IL stubs the InlinedCallFrame stays linked for the whole method, and
+    // it is reported before the method that contains it. Skip it when it has no active call.
+    if (!pCF->IsFrameless() && !InlinedCallFrame::FrameHasActiveCall(pCF->GetFrame()))
+    {
+        return SWA_CONTINUE;
+    }
+
     // This will help factor out some repeated code.
     bool notJittedCase = false;