Skip to content

Fix edge case of code heap initial request #71213

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.

Sign up for GitHub

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

Merged
janvorli merged 1 commit into from
Jun 23, 2022
Merged

Fix edge case of code heap initial request #71213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/coreclr/inc/loaderheap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,8 @@ class UnlockedLoaderHeap
BOOL IsExecutable();
BOOL IsInterleaved();

size_t AllocMem_TotalSize(size_t dwRequestedSize);

public:
#ifdef _DEBUG
void DumpFreeList();
Expand Down
26 changes: 12 additions & 14 deletions src/coreclr/utilcode/loaderheap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,8 +667,6 @@ class LoaderHeapSniffer
#endif


size_t AllocMem_TotalSize(size_t dwRequestedSize, UnlockedLoaderHeap *pHeap);

//=====================================================================================
// This freelist implementation is a first cut and probably needs to be tuned.
// It should be tuned with the following assumptions:
Expand Down Expand Up @@ -713,10 +711,10 @@ struct LoaderHeapFreeBlock
// It's illegal to insert a free block that's smaller than the minimum sized allocation -
// it may stay stranded on the freelist forever.
#ifdef _DEBUG
if (!(dwTotalSize >= AllocMem_TotalSize(1, pHeap)))
if (!(dwTotalSize >= pHeap->AllocMem_TotalSize(1)))
{
LoaderHeapSniffer::ValidateFreeList(pHeap);
_ASSERTE(dwTotalSize >= AllocMem_TotalSize(1, pHeap));
_ASSERTE(dwTotalSize >= pHeap->AllocMem_TotalSize(1));
}

if (!(0 == (dwTotalSize & ALLOC_ALIGN_CONSTANT)))
Expand Down Expand Up @@ -782,7 +780,7 @@ struct LoaderHeapFreeBlock
delete pCur;
break;
}
else if (dwCurSize > dwSize && (dwCurSize - dwSize) >= AllocMem_TotalSize(1, pHeap))
else if (dwCurSize > dwSize && (dwCurSize - dwSize) >= pHeap->AllocMem_TotalSize(1))
{
// Partial match. Ok...
pResult = pCur->m_pBlockAddress;
Expand Down Expand Up @@ -878,21 +876,21 @@ struct LoaderHeapFreeBlock
//=====================================================================================

// Convert the requested size into the total # of bytes we'll actually allocate (including padding)
inline size_t AllocMem_TotalSize(size_t dwRequestedSize, UnlockedLoaderHeap *pHeap)
size_t UnlockedLoaderHeap::AllocMem_TotalSize(size_t dwRequestedSize)
{
LIMITED_METHOD_CONTRACT;

size_t dwSize = dwRequestedSize;

// Interleaved heap cannot ad any extra to the requested size
if (!pHeap->IsInterleaved())
if (!IsInterleaved())
{
#ifdef _DEBUG
dwSize += LOADER_HEAP_DEBUG_BOUNDARY;
dwSize = ((dwSize + ALLOC_ALIGN_CONSTANT) & (~ALLOC_ALIGN_CONSTANT));
#endif

if (!pHeap->m_fExplicitControl)
if (!m_fExplicitControl)
{
#ifdef _DEBUG
dwSize += sizeof(LoaderHeapValidationTag);
Expand Down Expand Up @@ -1347,7 +1345,7 @@ BOOL UnlockedLoaderHeap::GetMoreCommittedPages(size_t dwMinSize)
// block list.
// Otherwise the remaining bytes that are available will be wasted.
size_t unusedRemainder = (size_t)(m_pPtrToEndOfCommittedRegion - m_pAllocPtr);
if (unusedRemainder >= AllocMem_TotalSize(m_dwGranularity, this))
if (unusedRemainder >= AllocMem_TotalSize(m_dwGranularity))
{
LoaderHeapFreeBlock::InsertFreeBlock(&m_pFirstFreeBlock, m_pAllocPtr, unusedRemainder, this);
}
Expand Down Expand Up @@ -1440,7 +1438,7 @@ void *UnlockedLoaderHeap::UnlockedAllocMem_NoThrow(size_t dwSize
dwSize += s_random.Next() % 256;
#endif

dwSize = AllocMem_TotalSize(dwSize, this);
dwSize = AllocMem_TotalSize(dwSize);

again:

Expand Down Expand Up @@ -1624,7 +1622,7 @@ void UnlockedLoaderHeap::UnlockedBackoutMem(void *pMem,
}
#endif

size_t dwSize = AllocMem_TotalSize(dwRequestedSize, this);
size_t dwSize = AllocMem_TotalSize(dwRequestedSize);

#ifdef _DEBUG
if ((m_dwDebugFlags & kCallTracing) && !IsInterleaved())
Expand Down Expand Up @@ -1745,7 +1743,7 @@ void *UnlockedLoaderHeap::UnlockedAllocAlignedMem_NoThrow(size_t dwRequestedSiz
// know whether the allocation will fit within the current reserved range.
//
// Thus, we'll request as much heap growth as is needed for the worst case (extra == alignment)
size_t dwRoomSize = AllocMem_TotalSize(dwRequestedSize + alignment, this);
size_t dwRoomSize = AllocMem_TotalSize(dwRequestedSize + alignment);
if (dwRoomSize > GetBytesAvailCommittedRegion())
{
if (!GetMoreCommittedPages(dwRoomSize))
Expand Down Expand Up @@ -1777,7 +1775,7 @@ void *UnlockedLoaderHeap::UnlockedAllocAlignedMem_NoThrow(size_t dwRequestedSiz
RETURN NULL;
}

size_t dwSize = AllocMem_TotalSize( cbAllocSize.Value(), this);
size_t dwSize = AllocMem_TotalSize( cbAllocSize.Value());
m_pAllocPtr += dwSize;


Expand Down Expand Up @@ -2126,7 +2124,7 @@ void LoaderHeapSniffer::ValidateFreeList(UnlockedLoaderHeap *pHeap)
}

size_t dwSize = pFree->m_dwSize;
if (dwSize < AllocMem_TotalSize(1, pHeap) ||
if (dwSize < pHeap->AllocMem_TotalSize(1) ||
0 != (dwSize & ALLOC_ALIGN_CONSTANT))
{
// Size is not a valid value (out of range or unaligned.)
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/vm/codeman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2517,7 +2517,11 @@ HeapList* LoaderCodeHeap::CreateCodeHeap(CodeHeapRequestInfo *pInfo, LoaderHeap
DWORD dwSizeAcquiredFromInitialBlock = 0;
bool fAllocatedFromEmergencyJumpStubReserve = false;

pBaseAddr = (BYTE *)pInfo->m_pAllocator->GetCodeHeapInitialBlock(loAddr, hiAddr, (DWORD)initialRequestSize, &dwSizeAcquiredFromInitialBlock);
size_t allocationSize = pCodeHeap->m_LoaderHeap.AllocMem_TotalSize(initialRequestSize);
#if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64)

@am11 am11 Jun 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we use #ifdef TARGET_64BIT instead (as done below on line 2600)?

@janvorli janvorli Jun 23, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, I can do that. I've just copied the same ifdef that we have below where we allocate the personality routine. But there are many more places in the code where we have this same compound #if. So maybe it would be worth a separate cleanup instead.

@janvorli janvorli Jun 23, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Btw even the CLRPersonalityRoutine declaration is under this compound ifdef

@am11 am11 Jun 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sure. Some of the usage of HOST_64BIT in VM can also be reviewed as part of that work, because it looks like the correct macro for them (in most places) is TARGET_64BIT (but it will require an in-depth analysis: #2256 (comment)).

@janvorli janvorli Jun 23, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've created issue #71220 for that.

allocationSize += pCodeHeap->m_LoaderHeap.AllocMem_TotalSize(JUMP_ALLOCATE_SIZE);
#endif
pBaseAddr = (BYTE *)pInfo->m_pAllocator->GetCodeHeapInitialBlock(loAddr, hiAddr, (DWORD)allocationSize, &dwSizeAcquiredFromInitialBlock);
if (pBaseAddr != NULL)
{
pCodeHeap->m_LoaderHeap.SetReservedRegion(pBaseAddr, dwSizeAcquiredFromInitialBlock, FALSE);
Expand Down