Unify StubLinker stub memory management with JIT code allocator#131903
Conversation
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR refactors CoreCLR stub generation so StubLinker-produced stubs (including shuffle thunks) allocate executable memory via the JIT’s code-fragment allocator rather than a dedicated LoaderAllocator::m_pStubHeap, and updates stub identification/tracing plus DAC descriptors accordingly.
Changes:
- Reworks
StubLinker::Link/Stub::NewStubto allocate stub memory viaEEJitManager::AllocCodeFragmentBlock, keyed by newStubCodeBlockKindvalues. - Removes
LoaderAllocator’s dedicated stub heap field/plumbing and updates call sites to passLoaderAllocator*. - Updates stub manager routing (
RangeSectionStubManager⇄StubLinkStubManager) and cDAC descriptors for the removed stub heap field.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/stubmgr.h | Removes StubLinkStubManager’s range list plumbing (moves identification to range-section kind). |
| src/coreclr/vm/stubmgr.cpp | Routes STUBLINK/SHUFFLE kinds through RangeSectionStubManager and forwards tracing to StubLinkStubManager. |
| src/coreclr/vm/stublink.h | Changes StubLinker::Link to take LoaderAllocator*; updates Stub::NewStub signature. |
| src/coreclr/vm/stublink.cpp | Allocates stub memory using AllocCodeFragmentBlock and tags with new stub kinds. |
| src/coreclr/vm/stubcache.h | Updates stub cache to retain a LoaderAllocator* instead of a LoaderHeap*. |
| src/coreclr/vm/stubcache.cpp | Passes LoaderAllocator* through to StubLinker::Link. |
| src/coreclr/vm/prestub.cpp | Updates stub linker call sites to pass LoaderAllocator*. |
| src/coreclr/vm/loaderallocator.hpp | Removes m_pStubHeap and its accessor; updates cDAC offsets accordingly. |
| src/coreclr/vm/loaderallocator.cpp | Removes stub heap initialization/termination/enumeration/size accounting; adjusts shuffle thunk cache initialization. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Drops the LoaderAllocator.StubHeap cDAC field. |
| src/coreclr/vm/comdelegate.h | Updates ShuffleThunkCache to use LoaderAllocator*. |
| src/coreclr/vm/comdelegate.cpp | Updates global shuffle thunk cache init; changes DelegateEEClass::GetStubHeap to return LoaderAllocator*. |
| src/coreclr/vm/codeman.h | Adds STUB_CODE_BLOCK_STUBLINK and STUB_CODE_BLOCK_SHUFFLE_THUNK kinds and string mappings. |
| src/coreclr/vm/class.h | Updates DelegateEEClass::GetStubHeap return type to LoaderAllocator*. |
| src/coreclr/vm/assembly.hpp | Removes Assembly::GetStubHeap declaration. |
| src/coreclr/vm/assembly.cpp | Removes Assembly::GetStubHeap implementation. |
| src/coreclr/vm/appdomain.hpp | Removes AppDomain::GetStubHeap declaration. |
| src/coreclr/vm/appdomain.inl | Removes AppDomain::GetStubHeap inline implementation. |
| src/coreclr/debug/daccess/request.cpp | Updates DAC exposure of “stub heap” data and heap-name list for loader allocator heap enumeration. |
|
Tagging subscribers to this area: @agocke |
…EAP_* Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/coreclr/vm/comdelegate.cpp:824
- The comment says this method returns a
LoaderHeap, but it now returns aLoaderAllocator*. Updating the comment avoids misleading future readers about what lifetime guarantee is being relied on here.
// We need a LoaderHeap that lives at least as long as the DelegateEEClass, but ideally no longer
LoaderAllocator *DelegateEEClass::GetStubHeap()
src/coreclr/vm/class.h:1925
- This comment still refers to returning a
LoaderHeap, but the method now returns aLoaderAllocator*. Keeping the comment accurate is important since this is the declaration most readers will see.
// We need a LoaderHeap that lives at least as long as the DelegateEEClass, but ideally no longer
LoaderAllocator *GetStubHeap();
src/coreclr/debug/daccess/request.cpp:3686
LoaderAllocatorLoaderHeapNamesis out of sync withGetLoaderAllocatorHeaps(): the names array includes both "ExecutableStubHeap" and "ExecutableHeap", butGetLoaderAllocatorHeaps()now only returns one executable heap pointer at that slot. This makesGetLoaderAllocatorHeapNames()report a different count/order than the returned heap addresses, which can confuse SOS/DAC consumers.
"ExecutableStubHeap",
"ExecutableHeap",
"FixupPrecodeHeap",
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/vm/eventtrace.cpp:3627
StubInitializednow reports every helper as size1, which makes ETW helper MethodLoad events effectively unusable (tools won’t be able to map IPs within the helper body to the helper name). We should report a realistic size (e.g., the write-barrier group size on x86, and at least a page-sized upper bound elsewhere) rather than a constant.
DWORD dwHelperSize = 1;
ETW::MethodLog::SendHelperEvent(ullHelperStartAddress, dwHelperSize, pHelperName);
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/vm/eventtrace.cpp:3626
StubInitializednow reports a hard-coded helper size of 1 byte, which looks like a placeholder after removingStub::RecoverStubAndSize. This will produce incorrect ETW MethodLoad events (size is part of the payload) for write-barrier helpers and any other helpers that call into this path.
Consider restoring a real size calculation (e.g., change StubInitialized to take a size parameter and pass the known helper size from the call sites), or otherwise derive the helper size from the allocation metadata instead of a constant.
DWORD dwHelperSize = 1;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/coreclr/vm/stubcache.h:47
StubCacheBaseallows a defaultNULLloader allocator, butStubLinker::Linkultimately callsEEJitManager::AllocCodeFragmentBlock, which has aPRECONDITION(pLoaderAllocator != NULL). This makes the API foot-gun: any future (or accidental) default construction would crash/assert. Make the requirement explicit by removing the default argument (and optionally assert non-null in the constructor).
StubCacheBase(LoaderAllocator *pLoaderAllocator = NULL);
src/coreclr/vm/eventtrace.cpp:3627
dwHelperSizeis now hardcoded to1, so ETWMethodLoadVerbosewill report every helper stub as a 1-byte method. That’s very likely misleading for diagnostics tooling that expects size/range information. If the size is no longer recoverable from aStubheader, consider plumbing the size from the call site (e.g., write-barrier helper generation knows sizes) or adding a helper to derive the real stub/code-fragment size from allocation metadata; if neither is possible, please add an explicit comment explaining why reporting size=1 is acceptable.
DWORD dwHelperSize = 1;
ETW::MethodLog::SendHelperEvent(ullHelperStartAddress, dwHelperSize, pHelperName);
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/coreclr/vm/codeman.h:131
GetStubCodeBlockKindStringno longer returns accurate names for several stub kinds:STUB_CODE_BLOCK_DYNAMICHELPERandSTUB_CODE_BLOCK_FIXUPPRECODEboth return "MethodCallThunk", andSTUB_CODE_BLOCK_STUBPRECODEis not handled at all (falls through to "Unknown"). This breaks perfmap/event naming for these stubs and makes it harder to diagnose stub allocations.
case STUB_CODE_BLOCK_JUMPSTUB:
return "JumpStub";
case STUB_CODE_BLOCK_DYNAMICHELPER:
return "MethodCallThunk";
case STUB_CODE_BLOCK_FIXUPPRECODE:
return "MethodCallThunk";
#ifdef FEATURE_VIRTUAL_STUB_DISPATCH
src/coreclr/vm/eventtrace.cpp:3628
StubInitializednow reports a hardcoded helper size of1byte. This value is emitted intoFireEtwMethodLoadVerbose_V1and will produce incorrect ETW method-load metadata for helper stubs (notably the write barrier helpers reported fromthreads.cpp). The size should either be computed from the actual stub allocation or passed in by the caller that knows the exact size.
if(ETW_TRACING_CATEGORY_ENABLED(MICROSOFT_WINDOWS_DOTNETRUNTIME_PROVIDER_DOTNET_Context,
TRACE_LEVEL_INFORMATION,
CLR_JIT_KEYWORD))
{
DWORD dwHelperSize = 1;
ETW::MethodLog::SendHelperEvent(ullHelperStartAddress, dwHelperSize, pHelperName);
}
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs:107
StubKindwas extended withWrapperStubandShuffleThunk, butGetStubKind(StubKind stubKind)(later in this file) does not map these new values to anyCodeKind, so these stubs will be reported asCodeKind.UnknownviaGetCodeKindwhen walking range-list stubs. Either map them to an existingCodeKind(e.g.,MethodCallThunk) or extendCodeKindif tools need to distinguish them.
| { | ||
| DWORD dwHelperSize=0; | ||
| Stub::RecoverStubAndSize((TADDR)ullHelperStartAddress, &dwHelperSize); | ||
| DWORD dwHelperSize = 1; |
There was a problem hiding this comment.
Pass in actual write barrier size
There was a problem hiding this comment.
Implemented in 6b1d87d: StubInitialized now takes a helper size parameter, and the write barrier callers pass the computed write barrier/check write barrier code size.
Note
This comment was generated by GitHub Copilot.
| VSDLookupStub = 7, | ||
| VSDVTableStub = 8, | ||
| CallCountingStub = 9, | ||
| WrapperStub = 10, |
There was a problem hiding this comment.
Extend GetStubKind with these new kinds
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
StubLinker-generated stubs used a dedicated
LoaderAllocator::m_pStubHeap, separate from the executable memory infrastructure shared by JITed code and other stub kinds. This PR unifies the two paths.LoaderAllocator: removedm_pStubHeapand its init/terminate/DAC/size-reporting plumbing.StubLinker/Stub:StubLinker::LinkandStub::NewStubnow take aLoaderAllocator*instead of aLoaderHeap*, allocating stub memory throughExecutionManager::GetEEJitManager()->AllocCodeFragmentBlock.StubCodeBlockKind: addedSTUB_CODE_BLOCK_STUBLINKandSTUB_CODE_BLOCK_SHUFFLE_THUNKto distinguish these stubs in the code-fragment allocator.StubLinkStubManager: removed its own range list;RangeSectionStubManagernow recognizes the new stub kinds and forwards stub identification/tracing toStubLinkStubManager(mirroring the existing pattern for jump stubs and call-counting stubs).datadescriptor.incanddebug/daccess/request.cppto drop the removed stub heap field.stubcache.cpp,comdelegate.cpp,prestub.cpp,assembly.cpp,appdomain.hpp/.inl, andclass.hto passLoaderAllocator*instead of a stubLoaderHeap*.