Move metadata off the executable heaps #52912
Conversation
This change moves metadata structures that manage blocks of heap memory out of the heaps in preparation for the W^X changes that will make the heap memory read-execute only and modifying the metadata would require unnecessary mappings and unmappings of the memory as read-write. The structures moved in this change are the following: * LoaderHeapBlock * FreeBlock * HeapList
src/coreclr/utilcode/loaderheap.cpp
Outdated
| @@ -1179,7 +1184,7 @@ BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit) | |||
| pCurBlock = pCurBlock->pNext; | |||
|
|
|||
| if (pCurBlock != NULL) | |||
| m_pCurBlock->pNext = pNewBlock; | |||
| pCurBlock->pNext = pNewBlock; | |||
There was a problem hiding this comment.
This fixes a bug that I've discovered while modifying the surrounding code.
There was a problem hiding this comment.
This linked list looks overengineered. I do not see a good reason why this maintains both m_pCurBlock and m_pFirstBlock. I think a single pointer that points to the head would be enough.
| #if defined(TARGET_AMD64) || defined(TARGET_ARM64) | ||
| return (TADDR)CLRPersonalityRoutine; | ||
| #else | ||
| return (TADDR)mapBase; |
There was a problem hiding this comment.
Why not always return mapBase here, even on AMD64 and ARM64?
There was a problem hiding this comment.
These values are different for dynamic methods. The HostCodeHeap prefixes all allocations storage by the TrackAllocation instance, so the CLRPersonalityRoutine is stored at an offset from the mapBase.
The ExecutionManager::GetCLRPersonalityRoutineValue() needs to return the same offset for both cases so that we don't need to check whether we are getting it for dynamic method or regular code (whether it came from the HostCodeHeap or LoaderHeap).
src/coreclr/utilcode/loaderheap.cpp
Outdated
| @@ -1179,7 +1184,7 @@ BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit) | |||
| pCurBlock = pCurBlock->pNext; | |||
|
|
|||
| if (pCurBlock != NULL) | |||
| m_pCurBlock->pNext = pNewBlock; | |||
| pCurBlock->pNext = pNewBlock; | |||
There was a problem hiding this comment.
This linked list looks overengineered. I do not see a good reason why this maintains both m_pCurBlock and m_pFirstBlock. I think a single pointer that points to the head would be enough.
I agree. Moreover, the m_pCurBlock is not used for anything else than adding a new block to the end of the list with O(1) complexity. But we can easily just always add the block to the beginning of the list to get the same effect. |
* Replace malloc with new (nothrow) and add explicit contract violation marker * Remove unnecessary m_pCurBlock from the UnlockedLoaderHeap
|
There seems to be some issue with this change and accessing dynamic function table. WinDbg at some points displays many "Unable to read dynamic function table entries" messages. |
janvorli
added
the
NO-MERGE
janvorli
removed
the
NO-MERGE
|
I've found and fixed the problem. The DAC |
janvorli
added
the
NO-MERGE
janvorli
removed
the
NO-MERGE
ghost
locked as resolved and limited conversation to collaborators
This change moves metadata structures that manage blocks of heap memory
out of the heaps in preparation for the W^X changes that will make the
heap memory read-execute only and modifying the metadata would require
unnecessary mappings and unmappings of the memory as read-write.
The structures moved in this change are the following:
Contributes to #50391