Open
Conversation
… classes - Remove NonVMComHolder<T> (identical alias to ReleaseHolder<T>), update call sites - Remove unused types: DoNothingHolder, FileHandleHolder, RegKeyHolder, BoolFlagStateHolder, FieldNuller, VoidCloseFileHandle, VoidUnmapViewOfFile, TypeUnmapViewOfFile, DoLocalFree - Replace MapViewHolder Wrapper typedef with explicit RAII class - Replace LocalAllocHolder SpecializedWrapper alias with explicit RAII class - Refactor HKEYHolder: add SuppressRelease(), proper Release() in operator=, assertion in operator&(), remove implicit operator HKEY*() - Remove unnecessary #ifndef SOS_INCLUDE guard around ReleaseHolder - Add STATIC_CONTRACT_WRAPPER to DoTheRelease Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…gement in holder.h; update LocalAllocHolder usage in sstring.cpp and util.cpp
…lder types and remove unused ArrayHolder class
…tion to streamline memory management
…ngerous default value
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors native resource management in CoreCLR debugging / createdump components by removing legacy holder types (ArrayHolder, NonVMComHolder, standalone releaseholder.h) and standardizing usage on the more general holders in holder.h (e.g., NewArrayHolder, AStringHolder, WStringHolder, ReleaseHolder, and new trait-based lifetime holders).
Changes:
- Removed
arrayholder.h/releaseholder.hand updated call sites to use holders fromholder.h. - Added new trait-based holders in
holder.h(e.g.,LifetimeHolder,MapViewHolder,LocalAllocHolder,HKEYHolder,BSTRHolder) and updated usage patterns accordingly. - Simplified COM registry lookup helper logic and updated createdump/debugger code paths to use the unified holders.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/utilcode/util.cpp | Refactors registry string reading helper and COM CLSID lookup to use HKEYHolder and updated ReadStringValue signature. |
| src/coreclr/utilcode/sstring.cpp | Updates FormatMessage dynamic allocation to use the updated LocalAllocHolder pattern. |
| src/coreclr/inc/releaseholder.h | Removes the legacy COM ReleaseHolder implementation (now consolidated into holder.h). |
| src/coreclr/inc/holder.h | Introduces trait-based LifetimeHolder and related holders; removes legacy aliases/holders and consolidates release logic. |
| src/coreclr/inc/arrayholder.h | Removes the legacy ArrayHolder implementation (replaced by NewArrayHolder / AStringHolder / WStringHolder). |
| src/coreclr/debug/di/module.cpp | Updates file mapping code to use the new MapViewHolder initialization style. |
| src/coreclr/debug/dbgutil/machoreader.cpp | Replaces ArrayHolder usage with NewArrayHolder for dyld image info allocation. |
| src/coreclr/debug/daccess/daccess.cpp | Replaces NonVMComHolder with ReleaseHolder for COM interface ownership. |
| src/coreclr/debug/daccess/cdac.h | Updates stored COM target field to use ReleaseHolder. |
| src/coreclr/debug/createdump/threadinfo.cpp | Replaces ArrayHolder<WCHAR> with WStringHolder for exception type name buffers. |
| src/coreclr/debug/createdump/dumpname.cpp | Replaces ArrayHolder<char> with AStringHolder for hostname buffer. |
| src/coreclr/debug/createdump/createdumpwindows.cpp | Replaces ArrayHolder<char> with AStringHolder for temporary path/name buffer. |
| src/coreclr/debug/createdump/createdumpmain.cpp | Replaces ArrayHolder<char> with AStringHolder for temp path buffer. |
| src/coreclr/debug/createdump/createdump.h | Replaces removed holder headers with holder.h and defines _ASSERTE for compatibility. |
| src/coreclr/debug/createdump/crashreportwriter.cpp | Replaces array holders with AStringHolder/WStringHolder for sysctl/method name buffers. |
| src/coreclr/debug/createdump/crashinfo.cpp | Replaces array holders with AStringHolder/WStringHolder for module name and formatting buffers. |
…r.h and updating related includes; adjust method calls for consistency
Member
Author
|
@hoyosjs for |
Contributor
|
Tagging subscribers to this area: @agocke |
jkoritzinsky
approved these changes
Mar 31, 2026
Member
|
Can we move more of the holder types over from SpecializedWrapper? It would be nice to start unraveling the Wrapper/Holder hierarchy if we can. |
This was referenced Apr 1, 2026
Open
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
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 refactors memory management in the createdump and related debugging code by removing the legacy
ArrayHolder(and some other holders) and replacing them with new, more robust holder types defined inholder.h. The changes modernize resource management, improve safety, and simplify code by consolidating smart pointer logic. Additionally, some minor improvements and cleanups are made to COM interface holders and other utility types.The most important changes are:
Memory Management Refactoring:
ArrayHolderimplementation (arrayholder.h) and replaced all usages with new holder types likeAStringHolder,WStringHolder, andNewArrayHolder, as defined inholder.h.holder.h, includingLifetimeHolder,MapViewHolder, andLocalAllocHolder, and updated their usage throughout the codebase.Holder Type Consolidation and Cleanup:
NonVMComHolderwithReleaseHolderfor COM interface management, and removed redundant holder aliases.Minor Fixes and Improvements:
Relesae→Release)..Extract()instead of.Detach()for stack frame management.These changes collectively improve code safety, maintainability, and clarity by consolidating resource management logic and removing legacy code.This pull request refactors resource management throughout the CoreCLR debugging components by replacing the custom
ArrayHolderandNonVMComHoldersmart pointer classes with new, more general-purpose holders defined inholder.h. It also removes the now-unusedarrayholder.handreleaseholder.hfiles and introduces more robust, trait-based holder implementations, improving code clarity and maintainability. The changes touch many files, especially those dealing with memory and handle management, and update usage sites accordingly.