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.
The external memory handle list was previously a member of AppDomain,
even though there is only ever a single AppDomain per process. Move
the list (and its lock) to a static on ExternalMemoryHandle itself so
the type owns its own lifetime and stops hanging off AppDomain's
grab-bag of unrelated state.
- AppDomain::AddExternalMemoryHandle/RemoveExternalMemoryHandle/
GCScanExternalMemoryHandles become ExternalMemoryHandle::Add/Remove/
GCScanRoots (plus a new Init/Cleanup pair called from
AppDomain::Init/~AppDomain).
- DebuggerExternalMemoryHandle and DacRefWalker::WalkExternalMemoryHandles
no longer need an AppDomain to reach the list.
- The cDAC ExternalMemoryHandles contract now reads a new
ExternalMemoryHandles global pointer instead of walking through the
AppDomain data descriptor.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CrstStatic (rather than CrstExplicitInit) is the established pattern for
a genuine process-wide static Crst that is explicitly Init()'d and never
destructed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add/Remove mutate the ExternalMemoryHandle list under s_crst, but
GCScanRoots walked it without taking that lock. During the initial,
concurrent mark phase of a background GC the EE keeps running, so a
debugger func-eval could concurrently insert or delete a node while the
GC thread iterates, risking a use-after-free (delete) or a missed root
(insert).
Follow the existing pattern used for interop ref-counted handle
callbacks (Interop::OnBeforeGCScanRoots/OnAfterGCScanRoots) and for
AppDomain statics (EnumAllStaticGCRefs), which skip work during the
concurrent phase: only invoke ExternalMemoryHandle::GCScanRoots when
sc->concurrent is false. The list can only be mutated while the EE is
suspended for a GC or by a thread in cooperative mode, so it is safe to
walk lock-free during the blocking mark/relocate phases and the final,
EE-suspended mark phase of a background GC; the concurrent mark phase
is simply skipped, and the subsequent final mark phase sees the
up-to-date list.
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.