Skip to content

Commit cb10fa9

Browse files
authored
Remove unnecessary LoaderHeap asserts (#74995)
There were two asserts in the LoaderHeap code that were checking successful reservation / committing of memory. However, these can fail in oom situation and the caller is prepared to handle such case. This assert was adding noise in CI testing. I have already removed one such assert at another place recently and I haven't noticed that we have other ones. The code at the two places was identical, so I have also refactored the committing code into a separate function.
1 parent 243cf9f commit cb10fa9

File tree

2 files changed

+33
-37
lines changed

2 files changed

+33
-37
lines changed

src/coreclr/inc/loaderheap.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,9 @@ class UnlockedLoaderHeap
327327
// has run out, reserve another set of pages
328328
BOOL GetMoreCommittedPages(size_t dwMinSize);
329329

330+
// Commit memory pages starting at the specified adress
331+
BOOL CommitPages(void* pData, size_t dwSizeToCommitPart);
332+
330333
protected:
331334
// Reserve some pages at any address
332335
BOOL UnlockedReservePages(size_t dwCommitBlockSize);

src/coreclr/utilcode/loaderheap.cpp

Lines changed: 30 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,6 +1087,33 @@ void ReleaseReservedMemory(BYTE* value)
10871087

10881088
using ReservedMemoryHolder = SpecializedWrapper<BYTE, ReleaseReservedMemory>;
10891089

1090+
BOOL UnlockedLoaderHeap::CommitPages(void* pData, size_t dwSizeToCommitPart)
1091+
{
1092+
// Commit first set of pages, since it will contain the LoaderHeapBlock
1093+
void *pTemp = ExecutableAllocator::Instance()->Commit(pData, dwSizeToCommitPart, IsExecutable());
1094+
if (pTemp == NULL)
1095+
{
1096+
return FALSE;
1097+
}
1098+
1099+
if (IsInterleaved())
1100+
{
1101+
_ASSERTE(dwSizeToCommitPart == GetOsPageSize());
1102+
1103+
void *pTemp = ExecutableAllocator::Instance()->Commit((BYTE*)pData + dwSizeToCommitPart, dwSizeToCommitPart, FALSE);
1104+
if (pTemp == NULL)
1105+
{
1106+
return FALSE;
1107+
}
1108+
1109+
ExecutableWriterHolder<BYTE> codePageWriterHolder((BYTE*)pData, GetOsPageSize());
1110+
m_codePageGenerator(codePageWriterHolder.GetRW(), (BYTE*)pData);
1111+
FlushInstructionCache(GetCurrentProcess(), pData, GetOsPageSize());
1112+
}
1113+
1114+
return TRUE;
1115+
}
1116+
10901117
BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit)
10911118
{
10921119
CONTRACTL
@@ -1166,32 +1193,11 @@ BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit)
11661193
dwSizeToCommitPart /= 2;
11671194
}
11681195

1169-
// Commit first set of pages, since it will contain the LoaderHeapBlock
1170-
void *pTemp = ExecutableAllocator::Instance()->Commit(pData, dwSizeToCommitPart, IsExecutable());
1171-
if (pTemp == NULL)
1196+
if (!CommitPages(pData, dwSizeToCommitPart))
11721197
{
1173-
_ASSERTE(!"Unable to commit a loaderheap code page");
1174-
11751198
return FALSE;
11761199
}
11771200

1178-
if (IsInterleaved())
1179-
{
1180-
_ASSERTE(dwSizeToCommitPart == GetOsPageSize());
1181-
1182-
void *pTemp = ExecutableAllocator::Instance()->Commit((BYTE*)pData + dwSizeToCommitPart, dwSizeToCommitPart, FALSE);
1183-
if (pTemp == NULL)
1184-
{
1185-
_ASSERTE(!"Unable to commit a loaderheap data page");
1186-
1187-
return FALSE;
1188-
}
1189-
1190-
ExecutableWriterHolder<BYTE> codePageWriterHolder(pData, GetOsPageSize());
1191-
m_codePageGenerator(codePageWriterHolder.GetRW(), pData);
1192-
FlushInstructionCache(GetCurrentProcess(), pData, GetOsPageSize());
1193-
}
1194-
11951201
// Record reserved range in range list, if one is specified
11961202
// Do this AFTER the commit - otherwise we'll have bogus ranges included.
11971203
if (m_pRangeList != NULL)
@@ -1301,26 +1307,13 @@ BOOL UnlockedLoaderHeap::GetMoreCommittedPages(size_t dwMinSize)
13011307
dwSizeToCommitPart /= 2;
13021308
}
13031309

1304-
// Yes, so commit the desired number of reserved pages
1305-
void *pData = ExecutableAllocator::Instance()->Commit(m_pPtrToEndOfCommittedRegion, dwSizeToCommitPart, IsExecutable());
1306-
if (pData == NULL)
1310+
if (!CommitPages(m_pPtrToEndOfCommittedRegion, dwSizeToCommitPart))
13071311
{
13081312
return FALSE;
13091313
}
13101314

13111315
if (IsInterleaved())
13121316
{
1313-
// Commit a data page after the code page
1314-
void* pDataRW = ExecutableAllocator::Instance()->Commit(m_pPtrToEndOfCommittedRegion + dwSizeToCommitPart, dwSizeToCommitPart, FALSE);
1315-
if (pDataRW == NULL)
1316-
{
1317-
return FALSE;
1318-
}
1319-
1320-
ExecutableWriterHolder<BYTE> codePageWriterHolder((BYTE*)pData, GetOsPageSize());
1321-
m_codePageGenerator(codePageWriterHolder.GetRW(), (BYTE*)pData);
1322-
FlushInstructionCache(GetCurrentProcess(), pData, GetOsPageSize());
1323-
13241317
// If the remaining bytes are large enough to allocate data of the allocation granularity, add them to the free
13251318
// block list.
13261319
// Otherwise the remaining bytes that are available will be wasted.
@@ -1335,7 +1328,7 @@ BOOL UnlockedLoaderHeap::GetMoreCommittedPages(size_t dwMinSize)
13351328

13361329
// For interleaved heaps, further allocations will start from the newly committed page as they cannot
13371330
// cross page boundary.
1338-
m_pAllocPtr = (BYTE*)pData;
1331+
m_pAllocPtr = (BYTE*)m_pPtrToEndOfCommittedRegion;
13391332
}
13401333

13411334
m_pPtrToEndOfCommittedRegion += dwSizeToCommitPart;

0 commit comments

Comments
 (0)