From 8c161d8f33afbb2947a695a275cfc508a2c6804f Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Sat, 26 Jun 2021 14:50:21 -0700 Subject: [PATCH 1/4] Partial changes --- src/coreclr/debug/ee/controller.cpp | 8 +++++-- src/coreclr/debug/ee/controller.h | 4 +++- src/coreclr/debug/ee/debugger.cpp | 30 ++++++++++++++---------- src/coreclr/debug/ee/debugger.h | 36 +++++++++++++++++++---------- 4 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 831dc2dd5b495..7a580a8bdc22d 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -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 @@ -4901,11 +4902,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..ad30de8ab1d72 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -16314,7 +16314,7 @@ 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); } @@ -16324,6 +16324,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 +16334,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 +16350,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; } } @@ -16366,9 +16368,9 @@ bool DebuggerHeapExecutableMemoryAllocator::CheckPageForAvailability(DebuggerHea void* DebuggerHeapExecutableMemoryAllocator::ChangePageUsage(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 +16461,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..51941f9e5967d 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 BOOKMARK_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 & BOOKMARK_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 < 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(BOOKMARK_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 From a1520806f406e7e61d6bc5d02d7f235a3b471265 Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Sat, 26 Jun 2021 19:47:19 -0700 Subject: [PATCH 2/4] Limit copy size --- src/coreclr/debug/ee/controller.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 7a580a8bdc22d..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 { @@ -4413,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) { From 52996c9baae9e6bc41f8f7d91c3ab34fdb8c49b9 Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Sat, 26 Jun 2021 21:34:55 -0700 Subject: [PATCH 3/4] Fix GCC build --- src/coreclr/debug/ee/debugger.cpp | 11 ++++++----- src/coreclr/debug/ee/debugger.h | 6 +++--- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index ad30de8ab1d72..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) @@ -16316,7 +16317,7 @@ void DebuggerHeapExecutableMemoryAllocator::Free(void* addr) // Sanity check: assert that the address really represents the start of a chunk. ASSERT(((uint64_t)addr - (uint64_t)pageToFreeIn) % EXPECTED_CHUNKSIZE == 0); - ChangePageUsage(pageToFreeIn, chunkNum, ChangePageUsageAction::FREE); + GetPointerToChunkWithUsageUpdate(pageToFreeIn, chunkNum, ChangePageUsageAction::FREE); } DebuggerHeapExecutableMemoryPage* DebuggerHeapExecutableMemoryAllocator::AddNewPage() @@ -16365,7 +16366,7 @@ 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); diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 51941f9e5967d..ff376ed4d52ab 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -1130,7 +1130,7 @@ struct DECLSPEC_ALIGN(DEBUGGERHEAP_PAGESIZE) DebuggerHeapExecutableMemoryPage inline void* GetPointerToChunk(int chunkNum) const { - ASSERT(chunkNum < CHUNKS_PER_DEBUGGERHEAP); + ASSERT(chunkNum >= 0 && (uint)chunkNum < CHUNKS_PER_DEBUGGERHEAP); return (char*)this + chunkNum * sizeof(DebuggerHeapExecutableMemoryChunk); } @@ -1151,7 +1151,7 @@ struct DECLSPEC_ALIGN(DEBUGGERHEAP_PAGESIZE) DebuggerHeapExecutableMemoryPage 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."); @@ -1182,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 From 1269683059cc6f91ed58575c20c5b67d85f5fc50 Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Sat, 26 Jun 2021 21:36:10 -0700 Subject: [PATCH 4/4] Change constant name to what I actually meant --- src/coreclr/debug/ee/debugger.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index ff376ed4d52ab..f16f8cd6d9d9d 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -1052,7 +1052,7 @@ 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 BOOKMARK_CHUNK_MASK (1ull << (CHUNKS_PER_DEBUGGERHEAP - 1)); +constexpr uint64_t BOOKKEEPING_CHUNK_MASK (1ull << (CHUNKS_PER_DEBUGGERHEAP - 1)); // Forward declaration struct DebuggerHeapExecutableMemoryPage; @@ -1122,7 +1122,7 @@ struct DECLSPEC_ALIGN(DEBUGGERHEAP_PAGESIZE) DebuggerHeapExecutableMemoryPage inline void SetPageOccupancy(uint64_t newOccupancy) { // Can't unset the bookmark chunk! - ASSERT((newOccupancy & BOOKMARK_CHUNK_MASK) != 0); + ASSERT((newOccupancy & BOOKKEEPING_CHUNK_MASK) != 0); ASSERT(newOccupancy <= MAX_CHUNK_MASK); ExecutableWriterHolder debuggerHeapPageWriterHolder(this, sizeof(DebuggerHeapExecutableMemoryPage)); debuggerHeapPageWriterHolder.GetRW()->chunks[0].bookkeeping.pageOccupancy = newOccupancy; @@ -1138,7 +1138,7 @@ struct DECLSPEC_ALIGN(DEBUGGERHEAP_PAGESIZE) DebuggerHeapExecutableMemoryPage { ExecutableWriterHolder debuggerHeapPageWriterHolder(this, sizeof(DebuggerHeapExecutableMemoryPage)); - SetPageOccupancy(BOOKMARK_CHUNK_MASK); // only the first bit is set. + SetPageOccupancy(BOOKKEEPING_CHUNK_MASK); // only the first bit is set. for (uint8_t i = 1; i < CHUNKS_PER_DEBUGGERHEAP; i++) { ASSERT(i != 0);