Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move metadata off the executable heaps #52912

Merged
merged 4 commits into from
May 20, 2021

Conversation

janvorli
Copy link
Member

@janvorli janvorli commented May 18, 2021

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

Contributes to #50391

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
@janvorli janvorli added this to the 6.0.0 milestone May 18, 2021
@janvorli janvorli requested a review from jkotas May 18, 2021 14:09
@janvorli janvorli self-assigned this May 18, 2021
@@ -1179,7 +1184,7 @@ BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit)
pCurBlock = pCurBlock->pNext;

if (pCurBlock != NULL)
m_pCurBlock->pNext = pNewBlock;
pCurBlock->pNext = pNewBlock;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug that I've discovered while modifying the surrounding code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/coreclr/vm/codeman.cpp Show resolved Hide resolved
#if defined(TARGET_AMD64) || defined(TARGET_ARM64)
return (TADDR)CLRPersonalityRoutine;
#else
return (TADDR)mapBase;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not always return mapBase here, even on AMD64 and ARM64?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -1179,7 +1184,7 @@ BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit)
pCurBlock = pCurBlock->pNext;

if (pCurBlock != NULL)
m_pCurBlock->pNext = pNewBlock;
pCurBlock->pNext = pNewBlock;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@janvorli
Copy link
Member Author

janvorli commented May 18, 2021

This linked list looks overengineered. I do not see a good reason why this maintains both m_pCurBlock and m_pFirstBlock.

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
@janvorli
Copy link
Member Author

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.
I need to look into it before merging in the change. I am not 100% sure if it is related to this change or some WinDbg quirk, but I want to be sure.

@janvorli janvorli added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 18, 2021
@janvorli janvorli removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 19, 2021
@janvorli
Copy link
Member Author

I've found and fixed the problem. The DAC OutOfProcessFunctionTableCallback_JIT function was assuming that the HeapList is at the beginning of the heap.

@janvorli janvorli added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 19, 2021
@janvorli janvorli removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 19, 2021
@janvorli janvorli merged commit 636f89d into dotnet:main May 20, 2021
@janvorli janvorli deleted the move-metadata-off-executable-heaps branch May 20, 2021 09:02
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants