You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
add AppDomain-managed external memory handles for GC-scanning managed references stored outside the GC heap and managed stacks
enumerate external memory handles through DAC and a dedicated cDAC ExternalMemoryHandles contract, including byref-like field walking and interior-pointer resolution
protect unboxed byref-like func-eval results for the lifetime of the returned ICorDebugValue
Testing
build.cmd tools+tools.cdactests -test
build.cmd clr
Note
This pull request description was generated with GitHub Copilot.
Transfer external memory handle ownership to the returned debugger value so byref-like result storage remains GC-protected for its lifetime.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Have you been able to observe debugger crashes or misbehaviors that are fixed by this change?
I have not observed any debugger crashes, but I did find in the func-eval code that we create a boxed byreflike on the heap to represent the result of the funceval:
I haven't heard about external memory handles before. What do we plan to use them for?
I plan to use them basically for supporting func-eval (this PR allocates a handle for the return of a funceval). In a future PR, I want to use them to replace the func-eval use cases for ProtectValueClassFrame as well (byreflike args that can't be boxed), enabling me to close #133730 in favor of this PR. The other usages of ProtectValueClassFrame today can all accept boxing (RuntimeMethodInfo.Invoke) so those cases will just move to the boxing path.
CCR flagged that the strong-handle path treated any non-null RetValueType
as a boxed object, including byref-like results whose slot holds value
data copied into the external memory handle rather than an OBJECTREF.
Exclude byref-like RetValueType from that condition; the external
memory handle remains the sole GC root for byref-like func-eval results.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The new lock has no dependencies with other Crst types, so it can
participate in ranking instead of being explicitly unordered.
Regenerated crsttypes_generated.h via CrstTypeTool.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Cleanup and partial-dump handling issues remain, end-to-end func-eval lifetime coverage is missing, and public API approval is unresolved.
Review tier: Lite Findings: None
Previously missed (1)
In code that hasn't changed since last review
Add an end-to-end debugger lifetime regression test
src/coreclr/debug/ee/funceval.cpp:2821
These changes allocate/register a real GC root and transfer ownership across the debugger IPC boundary, but the added tests only use mocked cDAC targets; none exercises actual FuncEvalComplete/GC/ICorDebugValue lifetime or cleanup after GetResult. Add an end-to-end debugger regression that forces a collection while the returned byref-like value is held and verifies disposal after release; this is the behavior the native changes are intended to protect.
[!NOTE] This review comment was generated by GitHub Copilot.
Span/ref-struct/byref-like off-heap storage, so maybe some ECS in a game engine
I would expect ECS in a game engine is not small, this sounds like a factory for GC pause problem.
It was a very intentional design decision not to support storing Spans on the GC heap. We do not want to pay the extra cost for scanning byrefs on the GC heap, nor do we want to make byrefs more expensive in order to reduce the cost of scanning them during GC.
ExternalMemoryHandleRootData and IExternalMemoryHandles add public surface in the Abstractions assembly, but the PR description contains no linked issue with the required api-approved label. Link the approved proposal and verify this shape, or keep the new surface internal until approval is available.
Approve the new public interior-pointer helper API
GCInteriorPointerResolver is another newly public type with a public constructor and Resolve method, but the PR description has no linked issue carrying the required api-approved label. Either link and match the approved API shape, or make this implementation helper internal until the API is approved.
DB_IPCE_DISPOSE_EXTERNAL_MEMORY_HANDLE is not handled by DbgTransportSession::GetEventSize in src/coreclr/debug/shared/dbgtransportsession.cpp; it falls into the unknown-event default. When this event uses the debugger transport, the payload is sent with only the base size (and asserts in debug), so vmExternalMemoryHandle is not delivered and the target handle leaks. Add a sizeof(pEvent->DisposeExternalMemoryHandle) case before relying on this event.
Preserve raw interior pointers when resolution is disabled
The contract documents that resolveInteriorPointers: false returns the raw pointer, but this early filter drops null and the pointer-size invalid sentinel regardless of that flag. This prevents callers from inspecting raw interior values and differs from the non-resolving stack-root path. Apply the null/invalid filter only when resolution is requested.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ExternalMemoryHandlescontract, including byref-like field walking and interior-pointer resolutionICorDebugValueTesting
build.cmd tools+tools.cdactests -testbuild.cmd clrNote
This pull request description was generated with GitHub Copilot.