From 1c383a27ce2c83d41b135cc149e422d15cfeb4e5 Mon Sep 17 00:00:00 2001 From: Juan Hoyos Date: Mon, 28 Jun 2021 14:33:51 -0700 Subject: [PATCH] Fix setting breakpoints on AVX 256 instructions and other 32 byte immediate instructions (#54786) --- src/coreclr/debug/ee/controller.cpp | 15 +++++++---- src/coreclr/debug/ee/controller.h | 4 ++- src/coreclr/debug/ee/debugger.cpp | 41 +++++++++++++++++------------ src/coreclr/debug/ee/debugger.h | 38 +++++++++++++++++--------- 4 files changed, 62 insertions(+), 36 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 831dc2dd5b495..b17ae8f115002 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/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(targetFixup), bufferBypass, 16); + case 32: + memcpy(reinterpret_cast(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 diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 9bcfc8682f7b2..12b1106f7a4b2 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/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; diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 4706790dd3d7f..53ee5555ace43 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/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,9 +16315,9 @@ void DebuggerHeapExecutableMemoryAllocator::Free(void* addr) int chunkNum = static_cast(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() @@ -16324,6 +16325,7 @@ DebuggerHeapExecutableMemoryPage* DebuggerHeapExecutableMemoryAllocator::AddNewP 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 diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 9d80fb3eec77e..f16f8cd6d9d9d 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/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,15 +1121,16 @@ 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 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); } @@ -1131,8 +1138,8 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage { ExecutableWriterHolder 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