Fix memory leak in SuperPMI #34523
Fix memory leak in SuperPMI #34523
Conversation
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
|
@sandreenko PTAL |
| @@ -41,6 +41,16 @@ CompileResult::~CompileResult() | |||
|
|
|||
| if (CallTargetTypes != nullptr) | |||
There was a problem hiding this comment.
Nit: You can delete the null checks for these - to make the style consistent.
|
superpmi test is failing |
Introduced by change to Heap APIs. Also fixes a long-existing memory leak on Linux. Introduce a small, simple class to keep track of memory allocations associated with the CompileResult that we need to free. In the replay case, SuperPMI allocates these (such as for the JIT calling allocMem). In the case of collection, the VM allocates memory for allocMem (and related), so that memory doesn't need to be tracked.
BruceForstall
force-pushed
the
FixSuperPMIMemoryLeak
branch
from
51288f7 to
e0f5444
Compare
|
In the replay case, the previous code was correct. However, in the collection case the memory from |
| void* allocate(size_t sizeInBytes) | ||
| { | ||
| BYTE* pNew = new BYTE[sizeInBytes]; | ||
| m_pHead = new MemoryNode(pNew, m_pHead); // Prepend this new one to the tracked memory list. |
There was a problem hiding this comment.
Nit: This has memory leak if new MemoryNode throws, but I assume that we do not care about hardening like this in superpmi.
There was a problem hiding this comment.
Yes, I'm not too worried about hardening superpmi for OOM
ghost
locked as resolved and limited conversation to collaborators
Introduced on Windows by change to Heap APIs (#34289).
Also fixes a long-existing memory leak on Linux where we never had
HeapDestroy().