AddLoadedRuntime#1497
Open
leculver wants to merge 10 commits into
Open
Conversation
Move the ThinLockLayout enum out of DacHeap into its own file and carry it on TargetProperties, computed once per runtime in DacServiceProvider. DacHeap now reads _target.ThinLockLayout instead of a separate ctor parameter. Pointer-size-only TargetProperties instances (DacLibrary, DacDataTarget) use the conservative Legacy layout since their layout is never consumed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9f6a7174-a30a-45e3-8475-430831714734
SkipRuntimeEnumeration (default false) makes ClrMD skip all runtime detection so ClrVersions is empty unless a host registers runtimes via AddLoadedRuntime, avoiding paying for enumeration a host does itself. AddLoadedRuntime now replaces an existing runtime that has the same ModuleInfo instead of appending a duplicate, so a module maps to a single runtime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9f6a7174-a30a-45e3-8475-430831714734
- Stable/shared dacLock: AddLoadedRuntime now stores a single non-null lock per ClrInfo (not a fresh one per CreateRuntime), so host-registered runtimes serialize consistently. - Synchronize _clrs: AddLoadedRuntime mutation and ClrVersions lazy init now share a lock (double-checked), preventing lost-update races. - Foreign IXCLRDataProcess: release the pre-wrapper Marshal.AddRef if ClrDataProcess construction fails, and dispose the process (releasing the ref) if the DacServiceProvider QI chain throws. - DacTypeHelpers.GetILMap: dispose each ClrDataMethod in a finally so a DAC throw no longer leaks native COM (release builds have no finalizer net). - DacServiceProvider.Dispose is now idempotent (returns instead of throwing on repeat). - Nit: thinLockLayout casing; drop the duplicate GetThinLockLayout computation in CreateForeignProcess (pointer-only target uses Legacy). - Test: ClrObjectTests.Scope.Dispose now disposes the ClrRuntime (not just the DataTarget); it was leaking a runtime whose cached ClrDataModule tripped the DEBUG finalizer at host shutdown. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9f6a7174-a30a-45e3-8475-430831714734
hoyosjs
reviewed
Jul 16, 2026
|
|
||
| if (_clrs.IsDefault) | ||
| if (!_clrs.IsDefault) | ||
| return _clrs; |
Member
There was a problem hiding this comment.
I don't know if this should be both the list and sentinel - it means AddRuntime immediately causes ClrVersions to never iterate (once registration is used, no other runtime is discovered), But if iteration is done and then registration happens, you get the 2 of them.
Contributor
Author
There was a problem hiding this comment.
Good point, fixed.
AddLoadedRuntime populated _clrs, and GetOrCreateClrVersions gated enumeration on _clrs.IsDefault, so registering a runtime before the first ClrVersions query silently suppressed ClrMD's own enumeration. Add an explicit _enumerated flag so enumeration is gated solely by SkipRuntimeEnumeration. ClrMD's enumerated runtimes are now merged with any already-registered ones, with the host registration winning on a matching ModuleInfo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9f6a7174-a30a-45e3-8475-430831714734
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request introduces several improvements and refactorings to the ClrMD runtime detection and hosting APIs, with a focus on supporting host-supplied runtimes (such as those provided by SOS), improving test coverage, and simplifying some internal code paths. The most important changes are outlined below.
Host-supplied runtime support and API improvements
ClrInfoclass to support construction for host-supplied runtimes, allowing external hosts to provide their ownIXCLRDataProcessand DAC lock, rather than relying solely on ClrMD's runtime detection and DAC loading. This includes adding theClrDataProcessFactoryandDacLockproperties and updating runtime creation logic to use them when present. [1] [2] [3]DacServiceProviderto support host-ownedIXCLRDataProcesspointers, ensuring proper reference counting and lock sharing for thread safety.Test coverage enhancements
DataTargetTests.csto verify that host-supplied runtimes are correctly registered and replace existing entries, and that heap enumeration matches between normal and host-supplied runtimes.Internal refactoring and simplification
DacHeapconstructor arguments toTargetProperties, simplifying the interface and centralizing layout logic. [1] [2] [3] [4]GetDacLibraryFromPathandCreateDacFromPath) inClrInfofor improved readability and maintainability.Minor fixes and code cleanup
ClrSymbolProviderWrapperandClrModuleInfo. [1] [2]Scope.Disposeto avoid resource leaks.These changes collectively improve ClrMD's flexibility for advanced hosting scenarios, increase test reliability, and simplify internal APIs.