Fix setting breakpoints on AVX 256 instructions and other 32 byte immediate instructions (#54786)

Author: hoyosjs

src/coreclr/debug/ee/controller.cpp

@@ -1393,7 +1393,7 @@ bool DebuggerController::ApplyPatch(DebuggerControllerPatch *patch)
             _ASSERTE(!"VirtualProtect of code page failed");
             return false;
         }
-#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)        
+#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)
     }
 // TODO: : determine if this is needed for AMD64
 #if defined(TARGET_X86) //REVISIT_TODO what is this?!
@@ -1496,7 +1496,7 @@ bool DebuggerController::UnapplyPatch(DebuggerControllerPatch *patch)
             _ASSERTE(!"VirtualProtect of code page failed");
             return false;
         }
-#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)        
+#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)
     }
     else
     {
@@ -4352,6 +4352,7 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread,
 
     m_pSharedPatchBypassBuffer = patch->GetOrCreateSharedPatchBypassBuffer();
     BYTE* patchBypass = m_pSharedPatchBypassBuffer->PatchBypass;
+    LOG((LF_CORDB, LL_INFO10000, "DPS::DPS: Patch skip for opcode 0x%.4x at address %p buffer allocated at 0x%.8x\n", patch->opcode, patch->address, m_pSharedPatchBypassBuffer));
 
     // Copy the instruction block over to the patch skip
     // WARNING: there used to be an issue here because CopyInstructionBlock copied the breakpoint from the
@@ -4412,8 +4413,9 @@ DebuggerPatchSkip::DebuggerPatchSkip(Thread *thread,
         }
         else
         {
+            _ASSERTE(m_instrAttrib.m_cOperandSize <= SharedPatchBypassBuffer::cbBufferBypass);
             // Copy the data into our buffer.
-            memcpy(bufferBypass, patch->address + m_instrAttrib.m_cbInstr + dwOldDisp, SharedPatchBypassBuffer::cbBufferBypass);
+            memcpy(bufferBypass, patch->address + m_instrAttrib.m_cbInstr + dwOldDisp, m_instrAttrib.m_cOperandSize);
 
             if (m_instrAttrib.m_fIsWrite)
             {
@@ -4901,11 +4903,14 @@ bool DebuggerPatchSkip::TriggerSingleStep(Thread *thread, const BYTE *ip)
             break;
 
         case 16:
-            memcpy(reinterpret_cast<void*>(targetFixup), bufferBypass, 16);
+        case 32:
+            memcpy(reinterpret_cast<void*>(targetFixup), bufferBypass, fixupSize);
             break;
 
         default:
-            _ASSERTE(!"bad operand size");
+            _ASSERTE(!"bad operand size. If you hit this and it was because we need to process instructions with larger \
+                relative immediates, make sure to update the SharedPatchBypassBuffer size, the DebuggerHeapExecutableMemoryAllocator, \
+                and structures depending on DBG_MAX_EXECUTABLE_ALLOC_SIZE.");
         }
     }
 #endif

src/coreclr/debug/ee/controller.h

@@ -288,7 +288,9 @@ class SharedPatchBypassBuffer
     // "PatchBypass" must be the first field of this class for alignment to be correct.
     BYTE    PatchBypass[MAX_INSTRUCTION_LENGTH];
 #if defined(TARGET_AMD64)
-    const static int cbBufferBypass = 0x10;
+    // If you update this value, make sure that it fits in the data payload of a
+    // DebuggerHeapExecutableMemoryChunk. This will need to be bumped to 0x40 for AVX 512 support.
+    const static int cbBufferBypass = 0x20;
     BYTE    BypassBuffer[cbBufferBypass];
 
     UINT_PTR                RipTargetFixup;

src/coreclr/debug/ee/debugger.cpp

@@ -365,10 +365,10 @@ void Debugger::DoNotCallDirectlyPrivateLock(void)
     //
     Thread * pThread;
     bool fIsCooperative;
-    
+
     pThread = g_pEEInterface->GetThread();
     fIsCooperative = (pThread != NULL) && (pThread->PreemptiveGCDisabled());
-    
+
     if (m_fShutdownMode && !fIsCooperative)
     {
         // The big fear is that some other random thread will take the debugger-lock and then block on something else,
@@ -16299,7 +16299,8 @@ void* DebuggerHeapExecutableMemoryAllocator::Allocate(DWORD numberOfBytes)
         }
     }
 
-    return ChangePageUsage(pageToAllocateOn, chunkToUse, ChangePageUsageAction::ALLOCATE);
+    ASSERT(chunkToUse >= 1 && (uint)chunkToUse < CHUNKS_PER_DEBUGGERHEAP);
+    return GetPointerToChunkWithUsageUpdate(pageToAllocateOn, chunkToUse, ChangePageUsageAction::ALLOCATE);
 }
 
 void DebuggerHeapExecutableMemoryAllocator::Free(void* addr)
@@ -16314,16 +16315,17 @@ void DebuggerHeapExecutableMemoryAllocator::Free(void* addr)
     int chunkNum = static_cast<DebuggerHeapExecutableMemoryChunk*>(addr)->data.chunkNumber;
 
     // Sanity check: assert that the address really represents the start of a chunk.
-    ASSERT(((uint64_t)addr - (uint64_t)pageToFreeIn) % 64 == 0);
+    ASSERT(((uint64_t)addr - (uint64_t)pageToFreeIn) % EXPECTED_CHUNKSIZE == 0);
 
-    ChangePageUsage(pageToFreeIn, chunkNum, ChangePageUsageAction::FREE);
+    GetPointerToChunkWithUsageUpdate(pageToFreeIn, chunkNum, ChangePageUsageAction::FREE);
 }
 
 DebuggerHeapExecutableMemoryPage* DebuggerHeapExecutableMemoryAllocator::AddNewPage()
 {
     void* newPageAddr = VirtualAlloc(NULL, sizeof(DebuggerHeapExecutableMemoryPage), MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE);
 
     DebuggerHeapExecutableMemoryPage *newPage = new (newPageAddr) DebuggerHeapExecutableMemoryPage;
+    CrstHolder execMemAllocCrstHolder(&m_execMemAllocMutex);
     newPage->SetNextPage(m_pages);
 
     // Add the new page to the linked list of pages
@@ -16333,8 +16335,9 @@ DebuggerHeapExecutableMemoryPage* DebuggerHeapExecutableMemoryAllocator::AddNewP
 
 bool DebuggerHeapExecutableMemoryAllocator::CheckPageForAvailability(DebuggerHeapExecutableMemoryPage* page, /* _Out_ */ int* chunkToUse)
 {
+    CrstHolder execMemAllocCrstHolder(&m_execMemAllocMutex);
     uint64_t occupancy = page->GetPageOccupancy();
-    bool available = occupancy != UINT64_MAX;
+    bool available = occupancy != MAX_CHUNK_MASK;
 
     if (!available)
     {
@@ -16348,13 +16351,13 @@ bool DebuggerHeapExecutableMemoryAllocator::CheckPageForAvailability(DebuggerHea
 
     if (chunkToUse)
     {
-        // Start i at 62 because first chunk is reserved
-        for (int i = 62; i >= 0; i--)
+        // skip the first bit, as that's used by the booking chunk.
+        for (int i = CHUNKS_PER_DEBUGGERHEAP - 2; i >= 0; i--)
         {
-            uint64_t mask = ((uint64_t)1 << i);
+            uint64_t mask = (1ull << i);
             if ((mask & occupancy) == 0)
             {
-                *chunkToUse = 64 - i - 1;
+                *chunkToUse = CHUNKS_PER_DEBUGGERHEAP - i - 1;
                 break;
             }
         }
@@ -16363,12 +16366,12 @@ bool DebuggerHeapExecutableMemoryAllocator::CheckPageForAvailability(DebuggerHea
     return true;
 }
 
-void* DebuggerHeapExecutableMemoryAllocator::ChangePageUsage(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action)
+void* DebuggerHeapExecutableMemoryAllocator::GetPointerToChunkWithUsageUpdate(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action)
 {
     ASSERT(action == ChangePageUsageAction::ALLOCATE || action == ChangePageUsageAction::FREE);
+    uint64_t mask = 1ull << (CHUNKS_PER_DEBUGGERHEAP - chunkNumber - 1);
 
-    uint64_t mask = (uint64_t)0x1 << (64 - chunkNumber - 1);
-
+    CrstHolder execMemAllocCrstHolder(&m_execMemAllocMutex);
     uint64_t prevOccupancy = page->GetPageOccupancy();
     uint64_t newOccupancy = (action == ChangePageUsageAction::ALLOCATE) ? (prevOccupancy | mask) : (prevOccupancy ^ mask);
     page->SetPageOccupancy(newOccupancy);
@@ -16459,11 +16462,15 @@ HRESULT DebuggerHeap::Init(BOOL fExecutable)
 #endif
 
 #ifndef HOST_WINDOWS
-    m_execMemAllocator = new (nothrow) DebuggerHeapExecutableMemoryAllocator();
-    ASSERT(m_execMemAllocator != NULL);
-    if (m_execMemAllocator == NULL)
+    m_execMemAllocator = NULL;
+    if (m_fExecutable)
     {
-        return E_OUTOFMEMORY;
+        m_execMemAllocator = new (nothrow) DebuggerHeapExecutableMemoryAllocator();
+        ASSERT(m_execMemAllocator != NULL);
+        if (m_execMemAllocator == NULL)
+        {
+            return E_OUTOFMEMORY;
+        }
     }
 #endif
 

src/coreclr/debug/ee/debugger.h

@@ -1047,7 +1047,12 @@ class DebuggerMethodInfo
 //     different part of the address space (not on the heap).
 // ------------------------------------------------------------------------ */
 
-#define DBG_MAX_EXECUTABLE_ALLOC_SIZE 48
+constexpr uint64_t DBG_MAX_EXECUTABLE_ALLOC_SIZE=112;
+constexpr uint64_t EXPECTED_CHUNKSIZE=128;
+constexpr uint64_t DEBUGGERHEAP_PAGESIZE=4096;
+constexpr uint64_t CHUNKS_PER_DEBUGGERHEAP=(DEBUGGERHEAP_PAGESIZE / EXPECTED_CHUNKSIZE);
+constexpr uint64_t MAX_CHUNK_MASK=((1ull << CHUNKS_PER_DEBUGGERHEAP) - 1);
+constexpr uint64_t BOOKKEEPING_CHUNK_MASK (1ull << (CHUNKS_PER_DEBUGGERHEAP - 1));
 
 // Forward declaration
 struct DebuggerHeapExecutableMemoryPage;
@@ -1060,7 +1065,7 @@ struct DebuggerHeapExecutableMemoryPage;
 // for the page, and the remaining ones are DataChunks and are handed out
 // by the allocator when it allocates memory.
 // ------------------------------------------------------------------------ */
-union DECLSPEC_ALIGN(64) DebuggerHeapExecutableMemoryChunk {
+union DECLSPEC_ALIGN(EXPECTED_CHUNKSIZE) DebuggerHeapExecutableMemoryChunk {
 
     struct DataChunk
     {
@@ -1078,13 +1083,14 @@ union DECLSPEC_ALIGN(64) DebuggerHeapExecutableMemoryChunk {
         DebuggerHeapExecutableMemoryPage *nextPage;
 
         uint64_t pageOccupancy;
+        static_assert(CHUNKS_PER_DEBUGGERHEAP <= sizeof(pageOccupancy) * 8,
+            "Our interfaces assume the chunks in a page can be masken on this field");
 
     } bookkeeping;
 
-    char _alignpad[64];
+    char _alignpad[EXPECTED_CHUNKSIZE];
 };
-
-static_assert(sizeof(DebuggerHeapExecutableMemoryChunk) == 64, "DebuggerHeapExecutableMemoryChunk is expect to be 64 bytes.");
+static_assert(sizeof(DebuggerHeapExecutableMemoryChunk) == EXPECTED_CHUNKSIZE, "DebuggerHeapExecutableMemoryChunk is expect to be EXPECTED_CHUNKSIZE bytes.");
 
 // ------------------------------------------------------------------------ */
 // DebuggerHeapExecutableMemoryPage
@@ -1095,7 +1101,7 @@ static_assert(sizeof(DebuggerHeapExecutableMemoryChunk) == 64, "DebuggerHeapExec
 // about which of the other chunks are used/free as well as a pointer to
 // the next page.
 // ------------------------------------------------------------------------ */
-struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
+struct DECLSPEC_ALIGN(DEBUGGERHEAP_PAGESIZE) DebuggerHeapExecutableMemoryPage
 {
     inline DebuggerHeapExecutableMemoryPage* GetNextPage()
     {
@@ -1115,24 +1121,25 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
 
     inline void SetPageOccupancy(uint64_t newOccupancy)
     {
-        // Can't unset first bit of occupancy!
-        ASSERT((newOccupancy & 0x8000000000000000) != 0);
-
+        // Can't unset the bookmark chunk!
+        ASSERT((newOccupancy & BOOKKEEPING_CHUNK_MASK) != 0);
+        ASSERT(newOccupancy <= MAX_CHUNK_MASK);
         ExecutableWriterHolder<DebuggerHeapExecutableMemoryPage> debuggerHeapPageWriterHolder(this, sizeof(DebuggerHeapExecutableMemoryPage));
         debuggerHeapPageWriterHolder.GetRW()->chunks[0].bookkeeping.pageOccupancy = newOccupancy;
     }
 
     inline void* GetPointerToChunk(int chunkNum) const
     {
+        ASSERT(chunkNum >= 0 && (uint)chunkNum < CHUNKS_PER_DEBUGGERHEAP);
         return (char*)this + chunkNum * sizeof(DebuggerHeapExecutableMemoryChunk);
     }
 
     DebuggerHeapExecutableMemoryPage()
     {
         ExecutableWriterHolder<DebuggerHeapExecutableMemoryPage> debuggerHeapPageWriterHolder(this, sizeof(DebuggerHeapExecutableMemoryPage));
 
-        SetPageOccupancy(0x8000000000000000); // only the first bit is set.
-        for (uint8_t i = 1; i < sizeof(chunks)/sizeof(chunks[0]); i++)
+        SetPageOccupancy(BOOKKEEPING_CHUNK_MASK); // only the first bit is set.
+        for (uint8_t i = 1; i < CHUNKS_PER_DEBUGGERHEAP; i++)
         {
             ASSERT(i != 0);
             debuggerHeapPageWriterHolder.GetRW()->chunks[i].data.startOfPage = this;
@@ -1141,8 +1148,13 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
     }
 
 private:
-    DebuggerHeapExecutableMemoryChunk chunks[64];
+    DebuggerHeapExecutableMemoryChunk chunks[CHUNKS_PER_DEBUGGERHEAP];
+    static_assert(sizeof(chunks) == DEBUGGERHEAP_PAGESIZE,
+        "Expected DebuggerHeapExecutableMemoryPage to have DEBUGGERHEAP_PAGESIZE bytes worth of chunks.");
+
 };
+static_assert(sizeof(DebuggerHeapExecutableMemoryPage) == DEBUGGERHEAP_PAGESIZE,
+    "DebuggerHeapExecutableMemoryPage exceeded the expected size.");
 
 // ------------------------------------------------------------------------ */
 // DebuggerHeapExecutableMemoryAllocator class
@@ -1170,7 +1182,7 @@ class DebuggerHeapExecutableMemoryAllocator
 
     DebuggerHeapExecutableMemoryPage* AddNewPage();
     bool CheckPageForAvailability(DebuggerHeapExecutableMemoryPage* page, /* _Out_ */ int* chunkToUse);
-    void* ChangePageUsage(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action);
+    void* GetPointerToChunkWithUsageUpdate(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action);
 
 private:
     // Linked list of pages that have been allocated