Use no-op MD Importer for DIA-backed stacktraces#129866
Open
hoyosjs wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes CoreCLR’s symbol-reader setup in Module::GetISymUnmanagedReader to avoid materializing a module’s real public metadata importer when constructing DIA-backed symbol readers, by instead passing a process-wide inert IMetaDataImport2 implementation.
Changes:
- Introduces
NoopMetadataImport, a singletonIMetaDataImport2implementation intended only to satisfy the binder’s non-null metadata-import parameter. - Updates
Module::GetISymUnmanagedReaderto pass the noop importer toISymUnmanagedBinder::GetReaderFromStream/GetReaderForFileinstead of using the module’s importer. - Wires the new files into the VM build via
src/coreclr/vm/CMakeLists.txt.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/vm/noopmetadataimport.h | Declares NoopMetadataImport singleton implementing IMetaDataImport2 as an inert binder placeholder. |
| src/coreclr/vm/noopmetadataimport.cpp | Implements singleton creation, COM QI/AddRef/Release behavior, and E_NOTIMPL stubs for metadata APIs. |
| src/coreclr/vm/CMakeLists.txt | Adds the new noop importer source/header to VM build lists. |
| src/coreclr/vm/ceeload.cpp | Switches symbol-reader binder calls to use NoopMetadataImport::GetInstance() instead of module metadata importers. |
AaronRobinsonMSFT
approved these changes
Jun 25, 2026
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
AaronRobinsonMSFT
approved these changes
Jun 26, 2026
jkotas
reviewed
Jun 26, 2026
|
|
||
| // Every method asserts in debug so an unexpected call (a future reader consumer | ||
| // that walks constant/local signatures, or a diasymreader change) is caught | ||
| // immediately; release builds return E_NOTIMPL. |
Member
There was a problem hiding this comment.
Do we have any tests that run on checked build and hit this path?
| @@ -21,6 +21,10 @@ STDAPI CreateMetaDataDispenser( | |||
| REFIID riid, | |||
| void ** pMetaDataDispenserOut); | |||
|
|
|||
| // Creation function to get a do-nothing IMetaDataImport2 instance for DIA. | |||
| STDAPI CreateNoopMetaDataImport2( | |||
Member
There was a problem hiding this comment.
Suggested change
| STDAPI CreateNoopMetaDataImport2( | |
| IMetaDataImport2* GetNoopMetaDataImport2(); |
This can never fail so we can just return it.
(Also, "STDAPI" should not be needed on any methods in this file.)
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // | ||
| // NoopMetadataImport implementation. |
Member
There was a problem hiding this comment.
All methods declared in corpriv.h are implemented under src\coreclr\md... .
It would make more sense for this file to be under src\coreclr\md\runtime
| riid == IID_IMetaDataImport2) | ||
| { | ||
| *ppvObject = static_cast<IMetaDataImport2*>(this); | ||
| AddRef(); |
Member
There was a problem hiding this comment.
Suggested change
| AddRef(); |
Unnecessary
jkotas
reviewed
Jun 26, 2026
| STDMETHOD(EnumMethodSpecs)(HCORENUM* phEnum, mdToken tk, mdMethodSpec rMethodSpecs[], ULONG cMax, ULONG* pcMethodSpecs) override { NOOPMD_NYI(EnumMethodSpecs); } | ||
|
|
||
| NoopMetadataImport() = default; | ||
| ~NoopMetadataImport() = default; |
Member
There was a problem hiding this comment.
Suggested change
| ~NoopMetadataImport() = default; |
We should not need destructor - it can only cause problem during shutdown.
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.
When resolving line numbers and sequence points for classic PDB-backed modules,
Module::GetISymUnmanagedReaderonmainpasses a real metadata importer into DIA. Materializing that importer can switch metadata access to the RW-backed variant, and later metadata reads can contend on its lock-heavy path.This change avoids that transition by passing a process-wide no-op importer (
NoopMetadataImport) that only satisfies the binder's non-null COM contract. Since it contains no module-linked state, it is a singleton and has no collectible-lifetime coupling.NoopMetadataImportbehavior:IUnknown,IMetaDataImport,IMetaDataImport2and does not exposeIID_IGetIMDInternalImport.AddRef/Releaseare process-lifetime no-ops.E_NOTIMPLin Release.The binder requires a non-null metadata import interface, but the StackTrace/source-line in-proc path does not need metadata signatures from that importer. A no-op importer keeps the binder contract while avoiding the RW swap.
Performance tests
Same repro from issue: https://gist.github.com/jkotas/617109b64d9b0ad52b491cdf021b2f8a
Machine:
Experiment:
Results (avg exceptions/sec):
This matches the target scenario (
full + no PDB) and keeps non-target cells effectively unchanged.Fixes #90563