Merge pull request #2264 from adityamandaleeka/dbg_exec_alloc

[WIP] Fix usage of executable heap memory

4 files changed, 408 insertions, 101 deletions

diff --git a/src/debug/ee/controller.cpp b/src/debug/ee/controller.cpp
index 4e1ab6557a..f3819af4a2 100644
--- a/src/debug/ee/controller.cpp
+++ b/src/debug/ee/controller.cpp
@@ -8445,9 +8445,9 @@ DebuggerFuncEvalComplete::DebuggerFuncEvalComplete(Thread *thread,
   : DebuggerController(thread, NULL)
 {
 #ifdef _TARGET_ARM_
-    m_pDE = reinterpret_cast<DebuggerEval*>(((DWORD)dest) & ~THUMB_CODE);
+    m_pDE = reinterpret_cast<DebuggerEvalBreakpointInfoSegment*>(((DWORD)dest) & ~THUMB_CODE)->m_associatedDebuggerEval;
 #else
-    m_pDE = reinterpret_cast<DebuggerEval*>(dest);
+    m_pDE = reinterpret_cast<DebuggerEvalBreakpointInfoSegment*>(dest)->m_associatedDebuggerEval;
 #endif
 
     // Add an unmanaged patch at the destination.
diff --git a/src/debug/ee/debugger.cpp b/src/debug/ee/debugger.cpp
index ddf2fadf14..12a7473e8e 100644
--- a/src/debug/ee/debugger.cpp
+++ b/src/debug/ee/debugger.cpp
@@ -40,7 +40,6 @@
 #include "dbgtransportsession.h"
 #endif // FEATURE_DBGIPC_TRANSPORT_VM
 
-
 #ifdef TEST_DATA_CONSISTENCY
 #include "datatest.h"
 #endif // TEST_DATA_CONSISTENCY
@@ -1405,11 +1404,14 @@ DebuggerEval::DebuggerEval(CONTEXT * pContext, DebuggerIPCE_FuncEvalInfo * pEval
 {
     WRAPPER_NO_CONTRACT;
 
+    // Allocate the breakpoint instruction info in executable memory.
+    m_bpInfoSegment = new (interopsafeEXEC, nothrow) DebuggerEvalBreakpointInfoSegment(this);
+
     // This must be non-zero so that the saved opcode is non-zero, and on IA64 we want it to be 0x16
     // so that we can have a breakpoint instruction in any slot in the bundle.
-    m_breakpointInstruction[0] = 0x16;
+    m_bpInfoSegment->m_breakpointInstruction[0] = 0x16;
 #if defined(_TARGET_ARM_)
-    USHORT *bp = (USHORT*)&m_breakpointInstruction;
+    USHORT *bp = (USHORT*)&m_bpInfoSegment->m_breakpointInstruction;
     *bp = CORDbg_BREAK_INSTRUCTION;
 #endif // _TARGET_ARM_
     m_thread = pEvalInfo->vmThreadToken.GetRawPtr();
@@ -1483,9 +1485,12 @@ DebuggerEval::DebuggerEval(CONTEXT * pContext, Thread * pThread, Thread::ThreadA
 {
     WRAPPER_NO_CONTRACT;
 
+    // Allocate the breakpoint instruction info in executable memory.
+    m_bpInfoSegment = new (interopsafeEXEC, nothrow) DebuggerEvalBreakpointInfoSegment(this);
+
     // This must be non-zero so that the saved opcode is non-zero, and on IA64 we want it to be 0x16
     // so that we can have a breakpoint instruction in any slot in the bundle.
-    m_breakpointInstruction[0] = 0x16;
+    m_bpInfoSegment->m_breakpointInstruction[0] = 0x16;
     m_thread = pThread;
     m_evalType = DB_IPCE_FET_RE_ABORT;
     m_methodToken = mdMethodDefNil;
@@ -15370,7 +15375,7 @@ HRESULT Debugger::FuncEvalSetup(DebuggerIPCE_FuncEvalInfo *pEvalInfo,
 
     // Create a DebuggerEval to hold info about this eval while its in progress. Constructor copies the thread's
     // CONTEXT.
-    DebuggerEval *pDE = new (interopsafeEXEC, nothrow) DebuggerEval(filterContext, pEvalInfo, fInException);
+    DebuggerEval *pDE = new (interopsafe, nothrow) DebuggerEval(filterContext, pEvalInfo, fInException);
 
     if (pDE == NULL)
     {
@@ -15502,7 +15507,7 @@ HRESULT Debugger::FuncEvalSetupReAbort(Thread *pThread, Thread::ThreadAbortReque
 
     // Create a DebuggerEval to hold info about this eval while its in progress. Constructor copies the thread's
     // CONTEXT.
-    DebuggerEval *pDE = new (interopsafeEXEC, nothrow) DebuggerEval(filterContext, pThread, requester);
+    DebuggerEval *pDE = new (interopsafe, nothrow) DebuggerEval(filterContext, pThread, requester);
 
     if (pDE == NULL)
     {
@@ -15692,7 +15697,8 @@ HRESULT Debugger::FuncEvalCleanup(DebuggerEval *debuggerEvalKey)
     LOG((LF_CORDB, LL_INFO1000, "D::FEC: pDE:%08x 0x%08x, id=0x%x\n",
          pDE, pDE->m_thread, GetThreadIdHelper(pDE->m_thread)));
 
-    DeleteInteropSafeExecutable(pDE);
+    DeleteInteropSafeExecutable(pDE->m_bpInfoSegment);
+    DeleteInteropSafe(pDE);
 
     return S_OK;
 }
@@ -16573,6 +16579,155 @@ void Debugger::ReleaseDebuggerDataLock(Debugger *pDebugger)
 #endif // DACCESS_COMPILE
 
 /* ------------------------------------------------------------------------ *
+ * Functions for DebuggerHeap executable memory allocations
+ * ------------------------------------------------------------------------ */
+
+DebuggerHeapExecutableMemoryAllocator::~DebuggerHeapExecutableMemoryAllocator()
+{
+    while (m_pages != NULL)
+    {
+        DebuggerHeapExecutableMemoryPage *temp = m_pages->GetNextPage();
+
+        // Free this page
+        INDEBUG(BOOL ret =) VirtualFree(m_pages, 0, MEM_RELEASE);
+        ASSERT(ret == TRUE);
+
+        m_pages = temp;
+    }
+
+    ASSERT(m_pages == NULL);
+}
+
+void* DebuggerHeapExecutableMemoryAllocator::Allocate(DWORD numberOfBytes)
+{
+    if (numberOfBytes > DBG_MAX_EXECUTABLE_ALLOC_SIZE)
+    {
+        ASSERT(!"Allocating more than DBG_MAX_EXECUTABLE_ALLOC_SIZE at once is unsupported and breaks our assumptions.");
+        return NULL;
+    }
+
+    if (numberOfBytes == 0)
+    {
+        // Should we allocate anything in this case?
+        ASSERT(!"Allocate called with 0 for numberOfBytes!");
+        return NULL;
+    }
+
+    CrstHolder execMemAllocCrstHolder(&m_execMemAllocMutex);
+
+    int chunkToUse = -1;
+    DebuggerHeapExecutableMemoryPage *pageToAllocateOn = NULL;
+    for (DebuggerHeapExecutableMemoryPage *currPage = m_pages; currPage != NULL; currPage = currPage->GetNextPage())
+    {
+        if (CheckPageForAvailability(currPage, &chunkToUse))
+        {
+            pageToAllocateOn = currPage;
+            break;
+        }
+    }
+
+    if (pageToAllocateOn == NULL)
+    {
+        // No existing page had availability, so create a new page and use that.
+        pageToAllocateOn = AddNewPage();
+        if (pageToAllocateOn == NULL)
+        {
+            ASSERT(!"Call to AddNewPage failed!");
+            return NULL;
+        }
+
+        if (!CheckPageForAvailability(pageToAllocateOn, &chunkToUse))
+        {
+            ASSERT(!"No availability on new page?");
+            return NULL;
+        }
+    }
+
+    return ChangePageUsage(pageToAllocateOn, chunkToUse, ChangePageUsageAction::ALLOCATE);
+}
+
+int DebuggerHeapExecutableMemoryAllocator::Free(void* addr)
+{
+    ASSERT(addr != NULL);
+
+    CrstHolder execMemAllocCrstHolder(&m_execMemAllocMutex);
+
+    DebuggerHeapExecutableMemoryPage *pageToFreeIn = static_cast<DebuggerHeapExecutableMemoryChunk*>(addr)->data.startOfPage;
+
+    if (pageToFreeIn == NULL)
+    {
+        ASSERT(!"Couldn't locate page in which to free!");
+        return -1;
+    }
+
+    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);
+
+    ChangePageUsage(pageToFreeIn, chunkNum, ChangePageUsageAction::FREE);
+
+    return 0;
+}
+
+DebuggerHeapExecutableMemoryPage* DebuggerHeapExecutableMemoryAllocator::AddNewPage()
+{
+    void* newPageAddr = VirtualAlloc(NULL, sizeof(DebuggerHeapExecutableMemoryPage), MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE);
+
+    DebuggerHeapExecutableMemoryPage *newPage = new (newPageAddr) DebuggerHeapExecutableMemoryPage;
+    newPage->SetNextPage(m_pages);
+
+    // Add the new page to the linked list of pages
+    m_pages = newPage;
+    return newPage;
+}
+
+bool DebuggerHeapExecutableMemoryAllocator::CheckPageForAvailability(DebuggerHeapExecutableMemoryPage* page, /* _Out_ */ int* chunkToUse)
+{
+    uint64_t occupancy = page->GetPageOccupancy();
+    bool available = occupancy != UINT64_MAX;
+
+    if (!available)
+    {
+        if (chunkToUse)
+        {
+            *chunkToUse = -1;
+        }
+
+        return false;
+    }
+
+    if (chunkToUse)
+    {
+        // Start i at 62 because first chunk is reserved
+        for (int i = 62; i >= 0; i--)
+        {
+            uint64_t mask = ((uint64_t)1 << i);
+            if ((mask & occupancy) == 0)
+            {
+                *chunkToUse = 64 - i - 1;
+                break;
+            }
+        }
+    }
+
+    return true;
+}
+
+void* DebuggerHeapExecutableMemoryAllocator::ChangePageUsage(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action)
+{
+    ASSERT(action == ChangePageUsageAction::ALLOCATE || action == ChangePageUsageAction::FREE);
+
+    uint64_t mask = (uint64_t)0x1 << (64 - chunkNumber - 1);
+
+    uint64_t prevOccupancy = page->GetPageOccupancy();
+    uint64_t newOccupancy = (action == ChangePageUsageAction::ALLOCATE) ? (prevOccupancy | mask) : (prevOccupancy ^ mask);
+    page->SetPageOccupancy(newOccupancy);
+
+    return page->GetPointerToChunk(chunkNumber);
+}
+
+/* ------------------------------------------------------------------------ *
  * DebuggerHeap impl
  * ------------------------------------------------------------------------ */
 
@@ -16584,7 +16739,6 @@ DebuggerHeap::DebuggerHeap()
     m_fExecutable = FALSE;
 }
 
-
 DebuggerHeap::~DebuggerHeap()
 {
     CONTRACTL
@@ -16607,6 +16761,12 @@ void DebuggerHeap::Destroy()
         m_hHeap = NULL;
     }
 #endif
+#ifdef FEATURE_PAL
+    if (m_execMemAllocator != NULL)
+    {
+        delete m_execMemAllocator;
+    }
+#endif
 }
 
 bool DebuggerHeap::IsInit()
@@ -16654,6 +16814,16 @@ HRESULT DebuggerHeap::Init(BOOL fExecutable)
         return HRESULT_FROM_GetLastError();
     }
 #endif
+
+#ifdef FEATURE_PAL
+    m_execMemAllocator = new (nothrow) DebuggerHeapExecutableMemoryAllocator();
+    ASSERT(m_execMemAllocator != NULL);
+    if (m_execMemAllocator == NULL)
+    {
+        return E_OUTOFMEMORY;
+    }
+#endif
+
     return S_OK;
 }
 
@@ -16736,17 +16906,33 @@ void *DebuggerHeap::Alloc(DWORD size)
     ret = ::HeapAlloc(m_hHeap, HEAP_ZERO_MEMORY, size);
 #else // USE_INTEROPSAFE_HEAP
 
-#ifndef FEATURE_PAL
-    HANDLE hExecutableHeap  = ClrGetProcessExecutableHeap();
-#else // !FEATURE_PAL
-    HANDLE hExecutableHeap  = ClrGetProcessHeap();
-#endif // !FEATURE_PAL
+    bool allocateOnHeap = true;
+    HANDLE hExecutableHeap = NULL;
 
-    if (hExecutableHeap == NULL)
+#ifdef FEATURE_PAL
+    if (m_fExecutable)
     {
-        return NULL;
+        allocateOnHeap = false;
+        ret = m_execMemAllocator->Allocate(size);
     }
-    ret = ClrHeapAlloc(hExecutableHeap, NULL, S_SIZE_T(size));
+    else
+    {
+        hExecutableHeap  = ClrGetProcessHeap();
+    }
+#else // FEATURE_PAL
+    hExecutableHeap  = ClrGetProcessExecutableHeap();
+#endif
+
+    if (allocateOnHeap)
+    {
+        if (hExecutableHeap == NULL)
+        {
+            return NULL;
+        }
+
+        ret = ClrHeapAlloc(hExecutableHeap, NULL, S_SIZE_T(size));
+    }
+
 #endif // USE_INTEROPSAFE_HEAP
 
 #ifdef USE_INTEROPSAFE_CANARY
@@ -16758,25 +16944,9 @@ void *DebuggerHeap::Alloc(DWORD size)
     ret = pCanary->GetUserAddr();
 #endif
 
-#ifdef FEATURE_PAL
-    if (m_fExecutable)
-    {
-        // We don't have executable heap in PAL, but we still need to allocate 
-        // executable memory, that's why have change protection level for 
-        // each allocation. 
-        // UNIXTODO: We need to look how JIT solves this problem.
-        DWORD unusedFlags;
-        if (!VirtualProtect(ret, size, PAGE_EXECUTE_READWRITE, &unusedFlags))
-        {
-            _ASSERTE(!"VirtualProtect failed to make this memory executable");
-        }
-    }
-#endif // FEATURE_PAL
-
     return ret;
 }
 
-
 // Realloc memory.
 // If this fails, the original memory is still valid.
 void *DebuggerHeap::Realloc(void *pMem, DWORD newSize, DWORD oldSize)
@@ -16793,7 +16963,7 @@ void *DebuggerHeap::Realloc(void *pMem, DWORD newSize, DWORD oldSize)
     _ASSERTE(newSize != 0);
     _ASSERTE(oldSize != 0);
 
-#if defined(USE_INTEROPSAFE_HEAP) && !defined(USE_INTEROPSAFE_CANARY)
+#if defined(USE_INTEROPSAFE_HEAP) && !defined(USE_INTEROPSAFE_CANARY) && !defined(FEATURE_PAL)
     // No canaries in this case.
     // Call into realloc.
     void *ret;
@@ -16815,22 +16985,6 @@ void *DebuggerHeap::Realloc(void *pMem, DWORD newSize, DWORD oldSize)
     this->Free(pMem);
 #endif
 
-
-#ifdef FEATURE_PAL
-    if (m_fExecutable)
-    {    
-        // We don't have executable heap in PAL, but we still need to allocate 
-        // executable memory, that's why have change protection level for 
-        // each allocation. 
-        // UNIXTODO: We need to look how JIT solves this problem.
-        DWORD unusedFlags;
-        if (!VirtualProtect(ret, newSize, PAGE_EXECUTE_READWRITE, &unusedFlags))
-        {
-            _ASSERTE(!"VirtualProtect failed to make this memory executable");
-        }
-    }
-#endif // FEATURE_PAL    
-
     return ret;
 }
 
@@ -16864,12 +17018,22 @@ void DebuggerHeap::Free(void *pMem)
     if (pMem != NULL)
     {
 #ifndef FEATURE_PAL
-        HANDLE hExecutableHeap  = ClrGetProcessExecutableHeap();
+        HANDLE hProcessExecutableHeap  = ClrGetProcessExecutableHeap();
+        _ASSERTE(hProcessExecutableHeap != NULL);
+        ClrHeapFree(hProcessExecutableHeap, NULL, pMem);
 #else // !FEATURE_PAL
-        HANDLE hExecutableHeap  = ClrGetProcessHeap();
+        if(!m_fExecutable)
+        {
+            HANDLE hProcessHeap  = ClrGetProcessHeap();
+            _ASSERTE(hProcessHeap != NULL);
+            ClrHeapFree(hProcessHeap, NULL, pMem);
+        }
+        else
+        {
+            INDEBUG(int ret =) m_execMemAllocator->Free(pMem);
+            _ASSERTE(ret == 0);
+        }
 #endif // !FEATURE_PAL
-        _ASSERTE(hExecutableHeap != NULL);
-        ClrHeapFree(hExecutableHeap, NULL, pMem);
     }
 #endif
 }
diff --git a/src/debug/ee/debugger.h b/src/debug/ee/debugger.h
index 9d25140cc0..b471fba601 100644
--- a/src/debug/ee/debugger.h
+++ b/src/debug/ee/debugger.h
@@ -1072,6 +1072,139 @@ protected:
     bool m_fHasInstrumentedILMap;
 };
 
+// ------------------------------------------------------------------------ *
+// Executable code memory management for the debugger heap.
+//
+//     Rather than allocating memory that needs to be executable on the process heap (which
+//     is forbidden on some flavors of SELinux and is generally a bad idea), we use the
+//     allocator below. It will handle allocating and managing the executable memory in a
+//     different part of the address space (not on the heap).
+// ------------------------------------------------------------------------ */
+
+#define DBG_MAX_EXECUTABLE_ALLOC_SIZE 48
+
+// Forward declaration
+struct DebuggerHeapExecutableMemoryPage;
+
+// ------------------------------------------------------------------------ */
+// DebuggerHeapExecutableMemoryChunk
+//
+// Each DebuggerHeapExecutableMemoryPage is divided into 64 of these chunks.
+// The first chunk is a BookkeepingChunk used for bookkeeping information
+// 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 {
+
+    struct DataChunk
+    {
+        char data[DBG_MAX_EXECUTABLE_ALLOC_SIZE];
+
+        DebuggerHeapExecutableMemoryPage *startOfPage;
+
+        // The chunk number within the page.
+        uint8_t chunkNumber;
+
+    } data;
+
+    struct BookkeepingChunk
+    {
+        DebuggerHeapExecutableMemoryPage *nextPage;
+
+        uint64_t pageOccupancy;
+
+    } bookkeeping;
+};
+
+static_assert(sizeof(DebuggerHeapExecutableMemoryChunk) == 64, "DebuggerHeapExecutableMemoryChunk is expect to be 64 bytes.");
+
+// ------------------------------------------------------------------------ */
+// DebuggerHeapExecutableMemoryPage
+//
+// We allocate the size of DebuggerHeapExecutableMemoryPage each time we need
+// more memory and divide each page into DebuggerHeapExecutableMemoryChunks for
+// use. The pages are self describing; the first chunk contains information
+// about which of the other chunks are used/free as well as a pointer to 
+// the next page.
+// ------------------------------------------------------------------------ */
+struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
+{
+    inline DebuggerHeapExecutableMemoryPage* GetNextPage()
+    {
+        return chunks[0].bookkeeping.nextPage;
+    }
+
+    inline void SetNextPage(DebuggerHeapExecutableMemoryPage* nextPage)
+    {
+        chunks[0].bookkeeping.nextPage = nextPage;
+    }
+
+    inline uint64_t GetPageOccupancy() const
+    {
+        return chunks[0].bookkeeping.pageOccupancy;
+    }
+
+    inline void SetPageOccupancy(uint64_t newOccupancy)
+    {
+        // Can't unset first bit of occupancy!
+        ASSERT((newOccupancy & 0x8000000000000000) != 0);
+
+        chunks[0].bookkeeping.pageOccupancy = newOccupancy;
+    }
+
+    inline void* GetPointerToChunk(int chunkNum) const
+    {
+        return (char*)this + chunkNum * sizeof(DebuggerHeapExecutableMemoryChunk);
+    }
+
+    DebuggerHeapExecutableMemoryPage()
+    {
+        SetPageOccupancy(0x8000000000000000); // only the first bit is set.
+        for (uint8_t i = 1; i < sizeof(chunks)/sizeof(chunks[0]); i++)
+        {
+            ASSERT(i != 0);
+            chunks[i].data.startOfPage = this;
+            chunks[i].data.chunkNumber = i;
+        }
+    }
+
+private:
+    DebuggerHeapExecutableMemoryChunk chunks[64];
+};
+
+// ------------------------------------------------------------------------ */
+// DebuggerHeapExecutableMemoryAllocator class
+// Handles allocation and freeing (and all necessary bookkeeping) for 
+// executable memory that the DebuggerHeap class needs. This is especially
+// useful on systems (like SELinux) where having executable code on the
+// heap is explicity disallowed for security reasons.
+// ------------------------------------------------------------------------ */
+
+class DebuggerHeapExecutableMemoryAllocator
+{
+public:
+    DebuggerHeapExecutableMemoryAllocator()
+    : m_pages(NULL)
+    , m_execMemAllocMutex(CrstDebuggerHeapExecMemLock, (CrstFlags)(CRST_UNSAFE_ANYMODE | CRST_REENTRANCY | CRST_DEBUGGER_THREAD))
+    { }
+
+    ~DebuggerHeapExecutableMemoryAllocator();
+
+    void* Allocate(DWORD numberOfBytes);
+    int Free(void* addr);
+
+private:
+    enum class ChangePageUsageAction {ALLOCATE, FREE};
+
+    DebuggerHeapExecutableMemoryPage* AddNewPage();
+    bool CheckPageForAvailability(DebuggerHeapExecutableMemoryPage* page, /* _Out_ */ int* chunkToUse);
+    void* ChangePageUsage(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action);
+
+private:
+    // Linked list of pages that have been allocated
+    DebuggerHeapExecutableMemoryPage* m_pages;
+    Crst m_execMemAllocMutex;
+};
 
 // ------------------------------------------------------------------------ *
 // DebuggerHeap class
@@ -1104,6 +1237,9 @@ protected:
     HANDLE m_hHeap;
 #endif
     BOOL m_fExecutable;
+
+private:
+    DebuggerHeapExecutableMemoryAllocator *m_execMemAllocator;
 };
 
 class DebuggerJitInfo;
@@ -3206,6 +3342,26 @@ public:
 #endif
 };
 
+class DebuggerEvalBreakpointInfoSegment
+{
+public:
+    // DebuggerEvalBreakpointInfoSegment contains just the breakpoint 
+    // instruction and a pointer to the associated DebuggerEval. It makes
+    // it easy to go from the instruction to the corresponding DebuggerEval
+    // object. It has been separated from the rest of the DebuggerEval 
+    // because it needs to be in a section of memory that's executable, 
+    // while the rest of DebuggerEval does not. By having it separate, we
+    // don't need to have the DebuggerEval contents in executable memory.
+    BYTE          m_breakpointInstruction[CORDbg_BREAK_INSTRUCTION_SIZE];
+    DebuggerEval *m_associatedDebuggerEval;
+
+    DebuggerEvalBreakpointInfoSegment(DebuggerEval* dbgEval)
+    : m_associatedDebuggerEval(dbgEval)
+    {
+        ASSERT(dbgEval != NULL);
+    }
+};
+
 /* ------------------------------------------------------------------------ *
  * DebuggerEval class
  *
@@ -3242,38 +3398,35 @@ public:
         FE_ABORT_RUDE = 2
     };
 
-    // Note: this first field must be big enough to hold a breakpoint
-    // instruction, and it MUST be the first field. (This
-    // is asserted in debugger.cpp)
-    BYTE                           m_breakpointInstruction[CORDbg_BREAK_INSTRUCTION_SIZE];
-    T_CONTEXT                      m_context;
-    Thread                        *m_thread;
-    DebuggerIPCE_FuncEvalType      m_evalType;
-    mdMethodDef                    m_methodToken;
-    mdTypeDef                      m_classToken;
-    ADID                           m_appDomainId;       // Safe even if AD unloaded
-    PTR_DebuggerModule             m_debuggerModule;     // Only valid if AD is still around
-    RSPTR_CORDBEVAL                m_funcEvalKey;
-    bool                           m_successful;        // Did the eval complete successfully
-    Debugger::AreValueTypesBoxed   m_retValueBoxing;        // Is the return value boxed?
-    unsigned int                   m_argCount;
-    unsigned int                   m_genericArgsCount;
-    unsigned int                   m_genericArgsNodeCount;
-    SIZE_T                         m_stringSize;
-    BYTE                          *m_argData;
-    MethodDesc                    *m_md;
-    PCODE                          m_targetCodeAddr;
-    INT64                          m_result;
-    TypeHandle                     m_resultType;
-    SIZE_T                         m_arrayRank;
-    FUNC_EVAL_ABORT_TYPE           m_aborting;          // Has an abort been requested, and what type.
-    bool                           m_aborted;           // Was this eval aborted
-    bool                           m_completed;          // Is the eval complete - successfully or by aborting
-    bool                           m_evalDuringException;
-    bool                           m_rethrowAbortException;
-    Thread::ThreadAbortRequester   m_requester;         // For aborts, what kind?
-    VMPTR_OBJECTHANDLE             m_vmObjectHandle;
-    TypeHandle                     m_ownerTypeHandle;
+    T_CONTEXT                          m_context;
+    Thread                            *m_thread;
+    DebuggerIPCE_FuncEvalType          m_evalType;
+    mdMethodDef                        m_methodToken;
+    mdTypeDef                          m_classToken;
+    ADID                               m_appDomainId;       // Safe even if AD unloaded
+    PTR_DebuggerModule                 m_debuggerModule;     // Only valid if AD is still around
+    RSPTR_CORDBEVAL                    m_funcEvalKey;
+    bool                               m_successful;        // Did the eval complete successfully
+    Debugger::AreValueTypesBoxed       m_retValueBoxing;        // Is the return value boxed?
+    unsigned int                       m_argCount;
+    unsigned int                       m_genericArgsCount;
+    unsigned int                       m_genericArgsNodeCount;
+    SIZE_T                             m_stringSize;
+    BYTE                              *m_argData;
+    MethodDesc                        *m_md;
+    PCODE                              m_targetCodeAddr;
+    INT64                              m_result;
+    TypeHandle                         m_resultType;
+    SIZE_T                             m_arrayRank;
+    FUNC_EVAL_ABORT_TYPE               m_aborting;          // Has an abort been requested, and what type.
+    bool                               m_aborted;           // Was this eval aborted
+    bool                               m_completed;          // Is the eval complete - successfully or by aborting
+    bool                               m_evalDuringException;
+    bool                               m_rethrowAbortException;
+    Thread::ThreadAbortRequester       m_requester;         // For aborts, what kind?
+    VMPTR_OBJECTHANDLE                 m_vmObjectHandle;
+    TypeHandle                         m_ownerTypeHandle;
+    DebuggerEvalBreakpointInfoSegment* m_bpInfoSegment;
 
     DebuggerEval(T_CONTEXT * pContext, DebuggerIPCE_FuncEvalInfo * pEvalInfo, bool fInException);
 
@@ -3282,11 +3435,10 @@ public:
 
     bool Init()
     {
-        _ASSERTE(DbgIsExecutable(&m_breakpointInstruction, sizeof(m_breakpointInstruction)));
+        _ASSERTE(DbgIsExecutable(&m_bpInfoSegment->m_breakpointInstruction, sizeof(m_bpInfoSegment->m_breakpointInstruction)));
         return true;
     }
 
-
     // The m_argData buffer holds both the type arg data (for generics) and the main argument data.
     //
     // For DB_IPCE_FET_NEW_STRING it holds the data specifying the string to create.
diff --git a/src/debug/ee/funceval.cpp b/src/debug/ee/funceval.cpp
index c7ec48d20c..5f9b6999b9 100644
--- a/src/debug/ee/funceval.cpp
+++ b/src/debug/ee/funceval.cpp
@@ -3872,21 +3872,12 @@ void * STDCALL FuncEvalHijackWorker(DebuggerEval *pDE)
     if (!pDE->m_evalDuringException)
     {
         // Signal to the helper thread that we're done with our func eval.  Start by creating a DebuggerFuncEvalComplete
-        // object. Give it an address at which to create the patch, which is a chunk of memory inside of our
+        // object. Give it an address at which to create the patch, which is a chunk of memory specified by our
         // DebuggerEval big enough to hold a breakpoint instruction.
 #ifdef _TARGET_ARM_
-        dest = (BYTE*)((DWORD)&(pDE->m_breakpointInstruction) | THUMB_CODE);
+        dest = (BYTE*)((DWORD)&(pDE->m_bpInfoSegment->m_breakpointInstruction) | THUMB_CODE);
 #else
-        dest = &(pDE->m_breakpointInstruction);
-#endif
-
-        // Here is kind of a cheat... we make sure that the address that we patch and jump to is actually also the ptr
-        // to our DebuggerEval. This works because m_breakpointInstruction is the first field of the DebuggerEval
-        // struct.
-#ifdef _TARGET_ARM_
-        _ASSERTE((((DWORD)dest) & ~THUMB_CODE) == (DWORD)pDE);
-#else
-        _ASSERTE(dest == pDE);
+        dest = &(pDE->m_bpInfoSegment->m_breakpointInstruction);
 #endif
 
         //