Separate DebuggerEval into executable and non-executable portions.

4 files changed, 116 insertions, 103 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 1ee88ab228..1959d50d67 100644
--- a/src/debug/ee/debugger.cpp
+++ b/src/debug/ee/debugger.cpp
@@ -1390,11 +1390,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();
@@ -1468,9 +1471,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;
@@ -15354,7 +15360,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)
     {
@@ -15486,7 +15492,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)
     {
@@ -15676,7 +15682,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;
 }
@@ -16562,33 +16569,32 @@ void Debugger::ReleaseDebuggerDataLock(Debugger *pDebugger)
 
 DebuggerHeapExecutableMemoryAllocator::~DebuggerHeapExecutableMemoryAllocator()
 {
-    DebuggerHeapExecutableMemoryPage *currPage = pages;
-    while (currPage != NULL)
+    while (m_pages != NULL)
     {
-        currPage = currPage->GetNextPage();
+        DebuggerHeapExecutableMemoryPage *temp = m_pages->GetNextPage();
 
         // Free this page
-        INDEBUG(BOOL ret =) VirtualFree(currPage, 0, MEM_RELEASE);
+        INDEBUG(BOOL ret =) VirtualFree(m_pages, 0, MEM_RELEASE);
         ASSERT(ret == TRUE);
 
-        pages = currPage;
+        m_pages = temp;
     }
 
-    ASSERT(pages == NULL);
+    ASSERT(m_pages == NULL);
 }
 
-void* DebuggerHeapExecutableMemoryAllocator::Allocate(DWORD numBytes)
+void* DebuggerHeapExecutableMemoryAllocator::Allocate(DWORD numberOfBytes)
 {
-    if (numBytes > DBG_MAX_EXECUTABLE_ALLOC_SIZE)
+    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 (numBytes == 0)
+    if (numberOfBytes == 0)
     {
         // Should we allocate anything in this case?
-        ASSERT(!"Allocate called with 0 for numBytes!");
+        ASSERT(!"Allocate called with 0 for numberOfBytes!");
         return NULL;
     }
 
@@ -16596,7 +16602,7 @@ void* DebuggerHeapExecutableMemoryAllocator::Allocate(DWORD numBytes)
 
     int chunkToUse = -1;
     DebuggerHeapExecutableMemoryPage *pageToAllocateOn = NULL;
-    for (DebuggerHeapExecutableMemoryPage *currPage = pages; currPage != NULL; currPage = currPage->GetNextPage())
+    for (DebuggerHeapExecutableMemoryPage *currPage = m_pages; currPage != NULL; currPage = currPage->GetNextPage())
     {
         if (CheckPageForAvailability(currPage, &chunkToUse))
         {
@@ -16622,15 +16628,16 @@ void* DebuggerHeapExecutableMemoryAllocator::Allocate(DWORD numBytes)
         }
     }
 
-    void* addr = ChangePageUsage(pageToAllocateOn, chunkToUse, true /* allocate */);
-    return addr;
+    return ChangePageUsage(pageToAllocateOn, chunkToUse, ChangePageUsageAction::ALLOCATE);
 }
 
 int DebuggerHeapExecutableMemoryAllocator::Free(void* addr)
 {
+    ASSERT(addr != NULL);
+
     CrstHolder execMemAllocCrstHolder(&m_execMemAllocMutex);
 
-    DebuggerHeapExecutableMemoryPage *pageToFreeIn = static_cast<DebuggerHeapExecutableMemoryChunk*>(addr)->d.startOfPage;
+    DebuggerHeapExecutableMemoryPage *pageToFreeIn = static_cast<DebuggerHeapExecutableMemoryChunk*>(addr)->data.startOfPage;
 
     if (pageToFreeIn == NULL)
     {
@@ -16638,12 +16645,12 @@ int DebuggerHeapExecutableMemoryAllocator::Free(void* addr)
         return -1;
     }
 
-    int chunkNum = static_cast<DebuggerHeapExecutableMemoryChunk*>(addr)->d.chunkNumber;
+    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, false /* indicates free */);
+    ChangePageUsage(pageToFreeIn, chunkNum, ChangePageUsageAction::FREE);
 
     return 0;
 }
@@ -16652,14 +16659,11 @@ DebuggerHeapExecutableMemoryPage* DebuggerHeapExecutableMemoryAllocator::AddNewP
 {
     void* newPageAddr = VirtualAlloc(NULL, sizeof(DebuggerHeapExecutableMemoryPage), MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE);
 
-    // Make sure new page is aligned to 4096.
-    ASSERT(((uint64_t)newPageAddr & 0xFFF) == 0);
-
     DebuggerHeapExecutableMemoryPage *newPage = new (newPageAddr) DebuggerHeapExecutableMemoryPage;
-    newPage->SetNextPage(pages);
+    newPage->SetNextPage(m_pages);
 
     // Add the new page to the linked list of pages
-    pages = newPage;
+    m_pages = newPage;
     return newPage;
 }
 
@@ -16683,10 +16687,10 @@ bool DebuggerHeapExecutableMemoryAllocator::CheckPageForAvailability(DebuggerHea
         // Start i at 62 because first chunk is reserved
         for (int i = 62; i >= 0; i--)
         {
-            uint64_t mask = ((unsigned long long)1 << i);
+            uint64_t mask = ((uint64_t)1 << i);
             if ((mask & occupancy) == 0)
             {
-                *chunkToUse = 64-i-1;
+                *chunkToUse = 64 - i - 1;
                 break;
             }
         }
@@ -16695,15 +16699,17 @@ bool DebuggerHeapExecutableMemoryAllocator::CheckPageForAvailability(DebuggerHea
     return true;
 }
 
-void* DebuggerHeapExecutableMemoryAllocator::ChangePageUsage(DebuggerHeapExecutableMemoryPage* page, int chunkNum, bool allocateOrFree)
+void* DebuggerHeapExecutableMemoryAllocator::ChangePageUsage(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action)
 {
-    uint64_t mask = (uint64_t)0x1 << (64 - chunkNum - 1);
+    ASSERT(action == ChangePageUsageAction::ALLOCATE || action == ChangePageUsageAction::FREE);
+
+    uint64_t mask = (uint64_t)0x1 << (64 - chunkNumber - 1);
 
     uint64_t prevOccupancy = page->GetPageOccupancy();
-    uint64_t newOccupancy = allocateOrFree ? (prevOccupancy | mask) : (prevOccupancy ^ mask);
+    uint64_t newOccupancy = (action == ChangePageUsageAction::ALLOCATE) ? (prevOccupancy | mask) : (prevOccupancy ^ mask);
     page->SetPageOccupancy(newOccupancy);
 
-    return page->GetPointerToChunk(chunkNum);
+    return page->GetPointerToChunk(chunkNumber);
 }
 
 /* ------------------------------------------------------------------------ *
@@ -16795,7 +16801,7 @@ HRESULT DebuggerHeap::Init(BOOL fExecutable)
 #endif
 
 #ifdef FEATURE_PAL
-    m_execMemAllocator = new DebuggerHeapExecutableMemoryAllocator();
+    m_execMemAllocator = new (nothrow) DebuggerHeapExecutableMemoryAllocator();
     ASSERT(m_execMemAllocator != NULL);
     if (m_execMemAllocator == NULL)
     {
diff --git a/src/debug/ee/debugger.h b/src/debug/ee/debugger.h
index 71801e79a3..b471fba601 100644
--- a/src/debug/ee/debugger.h
+++ b/src/debug/ee/debugger.h
@@ -1098,14 +1098,14 @@ union DECLSPEC_ALIGN(64) DebuggerHeapExecutableMemoryChunk {
 
     struct DataChunk
     {
-        char data[48];
+        char data[DBG_MAX_EXECUTABLE_ALLOC_SIZE];
 
         DebuggerHeapExecutableMemoryPage *startOfPage;
 
         // The chunk number within the page.
         uint8_t chunkNumber;
 
-    } d;
+    } data;
 
     struct BookkeepingChunk
     {
@@ -1113,11 +1113,11 @@ union DECLSPEC_ALIGN(64) DebuggerHeapExecutableMemoryChunk {
 
         uint64_t pageOccupancy;
 
-        char padding[48];
-
-    } b;
+    } bookkeeping;
 };
 
+static_assert(sizeof(DebuggerHeapExecutableMemoryChunk) == 64, "DebuggerHeapExecutableMemoryChunk is expect to be 64 bytes.");
+
 // ------------------------------------------------------------------------ */
 // DebuggerHeapExecutableMemoryPage
 //
@@ -1131,17 +1131,17 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
 {
     inline DebuggerHeapExecutableMemoryPage* GetNextPage()
     {
-        return chunks[0].b.nextPage;
+        return chunks[0].bookkeeping.nextPage;
     }
 
     inline void SetNextPage(DebuggerHeapExecutableMemoryPage* nextPage)
     {
-        chunks[0].b.nextPage = nextPage;
+        chunks[0].bookkeeping.nextPage = nextPage;
     }
 
-    inline uint64_t GetPageOccupancy()
+    inline uint64_t GetPageOccupancy() const
     {
-        return chunks[0].b.pageOccupancy;
+        return chunks[0].bookkeeping.pageOccupancy;
     }
 
     inline void SetPageOccupancy(uint64_t newOccupancy)
@@ -1149,10 +1149,10 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
         // Can't unset first bit of occupancy!
         ASSERT((newOccupancy & 0x8000000000000000) != 0);
 
-        chunks[0].b.pageOccupancy = newOccupancy;
+        chunks[0].bookkeeping.pageOccupancy = newOccupancy;
     }
 
-    inline void* GetPointerToChunk(int chunkNum)
+    inline void* GetPointerToChunk(int chunkNum) const
     {
         return (char*)this + chunkNum * sizeof(DebuggerHeapExecutableMemoryChunk);
     }
@@ -1163,14 +1163,13 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
         for (uint8_t i = 1; i < sizeof(chunks)/sizeof(chunks[0]); i++)
         {
             ASSERT(i != 0);
-            chunks[i].d.startOfPage = this;
-            chunks[i].d.chunkNumber = i;
+            chunks[i].data.startOfPage = this;
+            chunks[i].data.chunkNumber = i;
         }
     }
 
 private:
     DebuggerHeapExecutableMemoryChunk chunks[64];
-
 };
 
 // ------------------------------------------------------------------------ */
@@ -1185,24 +1184,25 @@ class DebuggerHeapExecutableMemoryAllocator
 {
 public:
     DebuggerHeapExecutableMemoryAllocator()
-    : m_execMemAllocMutex(CrstDebuggerHeapExecMemLock, (CrstFlags)(CRST_UNSAFE_ANYMODE | CRST_REENTRANCY | CRST_DEBUGGER_THREAD))
-    {
-        pages = NULL;
-    }
+    : m_pages(NULL)
+    , m_execMemAllocMutex(CrstDebuggerHeapExecMemLock, (CrstFlags)(CRST_UNSAFE_ANYMODE | CRST_REENTRANCY | CRST_DEBUGGER_THREAD))
+    { }
 
     ~DebuggerHeapExecutableMemoryAllocator();
 
-    void* Allocate(DWORD numBytes);
+    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 chunkNum, bool allocateOrFree);
+    void* ChangePageUsage(DebuggerHeapExecutableMemoryPage* page, int chunkNumber, ChangePageUsageAction action);
 
 private:
     // Linked list of pages that have been allocated
-    DebuggerHeapExecutableMemoryPage* pages;
+    DebuggerHeapExecutableMemoryPage* m_pages;
     Crst m_execMemAllocMutex;
 };
 
@@ -3342,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
  *
@@ -3378,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);
 
@@ -3418,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
 
         //