Fix GCStress issue on x86 and Arm (#25445)

* Fix GCStress issue on x86 and Arm32
  When we write one of the new gc stress instruction, such as INTERRUPT_INSTR_PROTECT_FIRST_RET
  we might be writing it in the epilog region on x86 or ARM as a direct call can be the last
  instruction before an epilog.  (This isn't allowed on x64)
  This fix expands the set of instructions we are allowed by IsMarkerInstr() toinclude these
  newly added gc stress instructions.
  added comment to GetGcMarkerExceptionCode
  Fix DAC build

* Code review feedback

* Additional code review chnages

diff --git a/src/vm/CMakeLists.txt b/src/vm/CMakeLists.txt
index 9bdbee3..f341566 100644 (file)
--- a/src/vm/CMakeLists.txt
+++ b/src/vm/CMakeLists.txt
@@ -177,6 +177,7 @@ set(VM_HEADERS_DAC_AND_WKS_COMMON
     field.h
     fptrstubs.h
     frames.h
+    gccover.h
     gctoclreventsink.h
     gcheaputilities.h
     generics.h
@@ -465,7 +466,6 @@ set(VM_HEADERS_WKS
     fieldmarshaler.h
     finalizerthread.h
     frameworkexceptionloader.h
-    gccover.h
     gcenv.h
     gcenv.ee.h
     gcenv.os.h

diff --git a/src/vm/eetwain.cpp b/src/vm/eetwain.cpp
index 2be3781..f973523 100644 (file)
--- a/src/vm/eetwain.cpp
+++ b/src/vm/eetwain.cpp
@@ -15,6 +15,10 @@
 #include "gcinfodecoder.h"
 #endif
 
+#ifdef HAVE_GCCOVER
+#include "gccover.h"
+#endif // HAVE_GCCOVER
+
 #include "argdestination.h"
 
 #define X86_INSTR_W_TEST_ESP            0x4485  // test [esp+N], eax
@@ -2915,13 +2919,28 @@ void    TRASH_CALLEE_UNSAVED_REGS(PREGDISPLAY pContext)
 
 bool IsMarkerInstr(BYTE val)
 {
-    SUPPORTS_DAC; 
+    SUPPORTS_DAC;
+
 #ifdef _DEBUG
-    return (val == X86_INSTR_INT3) || // Debugger might stomp with an int3
-           (val == X86_INSTR_HLT && GCStress<cfg_any>::IsEnabled()); // GcCover might stomp with a Hlt
-#else
+    if (val == X86_INSTR_INT3)
+    {
+        return true;
+    }
+#ifdef HAVE_GCCOVER
+    else // GcCover might have stomped on the instruction
+    {
+        if (GCStress<cfg_any>::IsEnabled())
+        {
+            if (IsGcCoverageInterruptInstructionVal(val))
+            {
+                return true;
+            }
+        }
+    }
+#endif // HAVE_GCCOVER
+#endif // _DEBUG
+
     return false;
-#endif
 }
 
 /* Check if the given instruction opcode is the one we expect.

diff --git a/src/vm/excep.cpp b/src/vm/excep.cpp
index ae4f76a..16e247d 100644 (file)
--- a/src/vm/excep.cpp
+++ b/src/vm/excep.cpp
@@ -52,6 +52,9 @@
 // Support for extracting MethodDesc of a delegate.
 #include "comdelegate.h"
 
+#ifdef HAVE_GCCOVER
+#include "gccover.h"
+#endif // HAVE_GCCOVER
 
 #ifndef FEATURE_PAL
 // Windows uses 64kB as the null-reference area
@@ -6549,6 +6552,8 @@ DWORD GetGcMarkerExceptionCode(LPVOID ip)
 #if defined(HAVE_GCCOVER)
     WRAPPER_NO_CONTRACT;
 
+    // IsGcCoverageInterrupt(ip) requires "gccover.h"
+    //
     if (GCStress<cfg_any>::IsEnabled() && IsGcCoverageInterrupt(ip))
     {
         return STATUS_CLR_GCCOVER_CODE;

diff --git a/src/vm/gccover.cpp b/src/vm/gccover.cpp
index c5cbdb0..7b8c474 100644 (file)
--- a/src/vm/gccover.cpp
+++ b/src/vm/gccover.cpp
@@ -72,64 +72,27 @@ static MethodDesc* getTargetMethodDesc(PCODE target)
     return nullptr;
 }
 
-
 bool IsGcCoverageInterruptInstruction(PBYTE instrPtr)
 {
+    UINT32 instrVal;
+
 #if defined(_TARGET_ARM64_)
-    UINT32 instrVal = *reinterpret_cast<UINT32*>(instrPtr);
-    switch (instrVal)
-    {
-    case INTERRUPT_INSTR:
-    case INTERRUPT_INSTR_CALL:
-    case INTERRUPT_INSTR_PROTECT_RET:
-        return true;
-    default:
-        return false;
-    }
+    instrVal = *reinterpret_cast<UINT32*>(instrPtr);
 #elif defined(_TARGET_ARM_)
-    
     size_t instrLen = GetARMInstructionLength(instrPtr);
     if (instrLen == 2)
     {
-        UINT16 instrVal = *reinterpret_cast<UINT16*>(instrPtr);
-        switch (instrVal)
-        {
-        case INTERRUPT_INSTR:
-        case INTERRUPT_INSTR_CALL:
-        case INTERRUPT_INSTR_PROTECT_RET:
-            return true;
-        default:
-            return false;
-        }
+        instrVal = *reinterpret_cast<UINT16*>(instrPtr);
     }
     else
     {
-        _ASSERTE(instrLen == 4);
-        UINT32 instrVal = *reinterpret_cast<UINT32*>(instrPtr);
-        switch (instrVal)
-        {
-        case INTERRUPT_INSTR_32:
-        case INTERRUPT_INSTR_CALL_32:
-        case INTERRUPT_INSTR_PROTECT_RET_32:
-            return true;
-        default:
-            return false;
-        }
+        instrVal = *reinterpret_cast<UINT32*>(instrPtr);
     }
 #else // x64 and x86
-    UINT8 instrVal = *reinterpret_cast<UINT8*>(instrPtr);
-    switch (instrVal)
-    {
-    case INTERRUPT_INSTR:
-    case INTERRUPT_INSTR_CALL:
-    case INTERRUPT_INSTR_PROTECT_FIRST_RET:
-    case INTERRUPT_INSTR_PROTECT_SECOND_RET:
-    case INTERRUPT_INSTR_PROTECT_BOTH_RET:
-        return true;
-    default:
-        return false;
-    }
+    instrVal = *instrPtr;
 #endif
+
+    return IsGcCoverageInterruptInstructionVal(instrVal);
 }
 
 bool IsOriginalInstruction(PBYTE instrPtr, GCCoverageInfo* gcCover, DWORD offset)
@@ -876,7 +839,7 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID
     {
         instructionIsACallThroughRegister = TRUE;
     }
-#endif
+#endif  // _TARGET_XXXX_
     // safe point must always be after a call instruction 
     // and cannot be both call by register & immediate
     // The safe points are also marked at jump calls( a special variant of 
@@ -893,7 +856,7 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID
         *((WORD*)instrPtr - 1) = INTERRUPT_INSTR_CALL;
 #elif defined(_TARGET_ARM64_)
         *((DWORD*)instrPtr - 1) = INTERRUPT_INSTR_CALL;
-#endif
+#endif // _TARGET_XXXX_
     }
     else if(instructionIsACallThroughImmediate)
     {
@@ -946,7 +909,7 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID
         }
     }
 }
-#endif
+#endif // PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED
 
 //Replaces the provided interruptible range with corresponding 2 or 4 byte gcStress illegal instruction
 bool replaceInterruptibleRangesWithGcStressInstr (UINT32 startOffset, UINT32 stopOffset, LPVOID pGCCover)
@@ -997,7 +960,6 @@ bool replaceInterruptibleRangesWithGcStressInstr (UINT32 startOffset, UINT32 sto
         PBYTE instrPtr = rangeStart;
         while(instrPtr < rangeStop)
         {
-
             // The instruction about to be replaced cannot already be a gcstress instruction
             _ASSERTE(!IsGcCoverageInterruptInstruction(instrPtr));
 #if defined(_TARGET_ARM_)
@@ -1018,7 +980,7 @@ bool replaceInterruptibleRangesWithGcStressInstr (UINT32 startOffset, UINT32 sto
             if(!isCallToStopForGCJitHelper(instrPtr))
                 *((DWORD*)instrPtr) = INTERRUPT_INSTR;
             instrPtr += 4;
-#endif
+#endif // TARGET_XXXX_
 
         }
 
@@ -1033,7 +995,7 @@ bool replaceInterruptibleRangesWithGcStressInstr (UINT32 startOffset, UINT32 sto
     }
     return FALSE;
 }
-#endif
+#endif // defined(_TARGET_ARM_) || defined(_TARGET_ARM64_)
 
 // Is this a call instruction to JIT_RareDisableHelper()
 // We cannot insert GCStress instruction at this call
@@ -1484,7 +1446,6 @@ void DoGcStress (PCONTEXT regs, MethodDesc *pMD)
 
     Thread *pThread = GetThread();
 
-
     if (!IsGcCoverageInterruptInstruction(instrPtr))
     {
         // This assert can fail if another thread changed original instruction to 

diff --git a/src/vm/gccover.h b/src/vm/gccover.h
index 6588975..e560c0d 100644 (file)
--- a/src/vm/gccover.h
+++ b/src/vm/gccover.h
@@ -65,7 +65,6 @@ public:
 #pragma warning(pop)
 #endif // _MSC_VER
 
-
 #if defined(_TARGET_X86_) || defined(_TARGET_AMD64_)
 
 #define INTERRUPT_INSTR                        0xF4    // X86 HLT instruction (any 1 byte illegal instruction will do)
@@ -108,6 +107,75 @@ public:
 
 #endif // _TARGET_*
 
+// The body of this method is in this header file to allow
+// mscordaccore.dll to link without getting an unsat symbol
+//
+inline bool IsGcCoverageInterruptInstructionVal(UINT32 instrVal)
+{
+#if defined(_TARGET_ARM64_)
+
+    switch (instrVal)
+    {
+    case INTERRUPT_INSTR:
+    case INTERRUPT_INSTR_CALL:
+    case INTERRUPT_INSTR_PROTECT_RET:
+        return true;
+    default:
+        return false;
+    }
+
+#elif defined(_TARGET_ARM_)
+
+    UINT16 instrVal16 = static_cast<UINT16>(instrVal);
+    size_t instrLen = GetARMInstructionLength(instrVal16);
+
+    if (instrLen == 2)
+    {
+        switch (instrVal16)
+        {
+        case INTERRUPT_INSTR:
+        case INTERRUPT_INSTR_CALL:
+        case INTERRUPT_INSTR_PROTECT_RET:
+            return true;
+        default:
+            return false;
+        }
+    }
+    else
+    {
+        _ASSERTE(instrLen == 4);
+
+        switch (instrVal)
+        {
+        case INTERRUPT_INSTR_32:
+        case INTERRUPT_INSTR_CALL_32:
+        case INTERRUPT_INSTR_PROTECT_RET_32:
+            return true;
+        default:
+            return false;
+        }
+    }
+
+#else // x64 and x86
+
+    switch (instrVal)
+    {
+    case INTERRUPT_INSTR:
+    case INTERRUPT_INSTR_CALL:
+    case INTERRUPT_INSTR_PROTECT_FIRST_RET:
+    case INTERRUPT_INSTR_PROTECT_SECOND_RET:
+    case INTERRUPT_INSTR_PROTECT_BOTH_RET:
+        return true;
+    default:
+        return false;
+    }
+
+#endif  // _TARGET_XXXX_
+}
+
+bool IsGcCoverageInterruptInstruction(PBYTE instrPtr);
+bool IsGcCoverageInterrupt(LPVOID ip);
+
 #endif // HAVE_GCCOVER
 
 #endif // !__GCCOVER_H__

diff --git a/src/vm/jitinterface.h b/src/vm/jitinterface.h
index 11b0b7f..c5f7953 100644 (file)
--- a/src/vm/jitinterface.h
+++ b/src/vm/jitinterface.h
@@ -1650,7 +1650,6 @@ void *GenFastGetSharedStaticBase(bool bCheckCCtor);
 #ifdef HAVE_GCCOVER
 void SetupGcCoverage(MethodDesc* pMD, BYTE* nativeCode);
 void SetupGcCoverageForNativeImage(Module* module);
-bool IsGcCoverageInterrupt(LPVOID ip);
 BOOL OnGcCoverageInterrupt(PT_CONTEXT regs);
 void DoGcStress (PT_CONTEXT regs, MethodDesc *pMD);
 #endif //HAVE_GCCOVER