Adding EnumMethodDefinitionByName, StartEnumMethodDefinitionsByName, and EndEnumMethodDefinitionsByName cDAC APIs#125539
Adding EnumMethodDefinitionByName, StartEnumMethodDefinitionsByName, and EndEnumMethodDefinitionsByName cDAC APIs#125539rcj1 wants to merge 5 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR adds cDAC-backed support for enumerating method definitions by name through the legacy IXCLRDataModule COM surface, enabling name-based method lookup via ECMA-335 metadata.
Changes:
- Introduces
CLRDataByNameFlagto represent by-name lookup options (case sensitivity). - Implements
StartEnumMethodDefinitionsByName/EnumMethodDefinitionByName/EndEnumMethodDefinitionsByNameinClrDataModuleusingMetadataReader. - Adds a new
ClrDataMethodDefinitionCOM class to represent returned method-definition objects.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs | Adds CLRDataByNameFlag enum for by-name enumeration flags. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs | Implements cDAC-based method-definition-by-name enumeration, maintaining optional sync with legacy DAC in DEBUG. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs | Introduces a COM class intended to represent method definitions returned by the new enumeration. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs
Outdated
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs
Outdated
Show resolved
Hide resolved
...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs
Show resolved
Hide resolved
...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs
Show resolved
Hide resolved
noahfalk
left a comment
There was a problem hiding this comment.
This looks fine to me modulo adding a bunch of test cases to verify/document that the name syntax supported here is exactly the same as what the DAC implementation supports.
| _flags = flags; | ||
| } | ||
|
|
||
| public void Start(string fullName) |
There was a problem hiding this comment.
Can you add test cases that pass in a wide variety of different names - it serves both as documentation of the syntax and verification that it matches what the DAC did. These type name parsers can have irritating levels of detail in what they accept and how they expect it to be formatted. As much as possible I try to keep name parsing/formatting out of the API surface but I know for compat we don't have a choice here. They can be bug farms if not thoroughly specified and tested. For instance:
- are namespaces required?
- are type aliases allowed? ('int' vs 'Int32')
- are generic type parameters supported?
- if so do they use backtick {arity} notation, square bracket notation, or angle bracket notation?
- are open and closed definitions supported? If closed definitions are supported are generic parameter types parsed with the exact same rules as the outer type?
- are closed definitions that refer to enclosing scope generic variables (!0, !!0) supported?
- are type parameter names supported?
- if both a nested class and its enclosing class have generic parameters, which parameters are attributed to each
- are pointer, array, and by ref types supported? (and how encoded if so)
- are method parameters, calling convention, or return value types considered? If so do they follow the same type formatting rules as the declaring type for the method?
No need to answer them as text, but hopefully the test cases would verify all of this
There was a problem hiding this comment.
- Namespaces are optional
- Type aliases are not allowed
- Generic type parameters are supported only with backtick notation. Closed definitions and parameter names are not supported
- The breakpoint can be set, but Windbg crashes after hitting a breakpoint on say, System.Array.Clone. windbg-stacktrace.txt
- Parameters are ignored, and calling convention and return value types must not be typed. Only the name of the method is allowed and
processed.
There was a problem hiding this comment.
The docs specify that you need the "method name". I'm wondering if we should be more specific about this. Perhaps adding something like "Method parameters will be ignored (bpmd binds to all overloaded methods)."
There was a problem hiding this comment.
Pull request overview
Adds cDAC support for enumerating method definitions by name in the legacy IXCLRData COM surface, enabling scenarios like bpmd to bind methods via metadata-based lookup.
Changes:
- Introduces
CLRDataByNameFlagfor case-sensitivity options used by by-name enumeration. - Implements
StartEnumMethodDefinitionsByName,EnumMethodDefinitionByName, andEndEnumMethodDefinitionsByNameinClrDataModuleusing ECMA-335 metadata, while keeping legacy enumeration in sync for debug validation. - Adds a new COM class
ClrDataMethodDefinitionas the managed representation returned by the enumeration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs |
Adds CLRDataByNameFlag used by the new by-name enumeration APIs. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs |
Implements metadata-backed method-definition-by-name enumeration and handle management. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs |
Adds a COM class to represent enumerated method definitions (currently legacy-delegating). |
...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs
Show resolved
Hide resolved
...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds cDAC support for enumerating method definitions by name in the Legacy IXCLRData layer, along with targeted unit tests that validate name parsing and matching behaviors against synthetic metadata.
Changes:
- Introduces
CLRDataByNameFlagand implementsStartEnumMethodDefinitionsByName/EnumMethodDefinitionByName/EndEnumMethodDefinitionsByNameinClrDataModule. - Adds a new
ClrDataMethodDefinitionCOM class wrapper for returned method definitions. - Adds
EnumMethodDefinitionsTestscovering type/method name parsing (namespaces optional, nested types, generics, constructors, case sensitivity, overload counts).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/native/managed/cdac/tests/EnumMethodDefinitionsTests.cs | Adds synthetic-metadata unit tests for method-by-name parsing/matching and expected enumeration counts. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs | Adds CLRDataByNameFlag to model native CLRDATA_BYNAME_* flags. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs | Implements the module-level method-definition-by-name enumeration using ECMA metadata, with legacy cross-validation/fallback plumbing. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs | Adds a new COM class wrapper for IXCLRDataMethodDefinition results (currently legacy-forwarding). |
...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds cDAC support for enumerating method definitions by name in the legacy IXCLRDataModule COM surface, including flag definitions and a focused unit test validating name parsing and matching behavior.
Changes:
- Implement
StartEnumMethodDefinitionsByName,EnumMethodDefinitionByName, andEndEnumMethodDefinitionsByNameinClrDataModule. - Introduce
CLRDataByNameFlagfor case-sensitivity control. - Add unit tests with synthetic ECMA-335 metadata to validate name parsing (namespace optionality, nesting separators, constructors, overloads, case-insensitive matching).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/native/managed/cdac/tests/EnumMethodDefinitionsTests.cs | Adds unit tests using synthetic metadata to validate method-name parsing/matching behavior. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs | Introduces CLRDataByNameFlag used by the new enumeration APIs. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs | Implements the new “enum method definitions by name” APIs and associated helper enumerator. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs | Adds a COM wrapper type returned by enumeration (IXCLRDataMethodDefinition). |
| GCHandle gcHandle = GCHandle.FromIntPtr((IntPtr)handle); | ||
| if (gcHandle.Target is not EnumMethodDefinitions emdLocal) | ||
| throw new ArgumentException(); | ||
| emd = emdLocal; | ||
| emd.MethodEnumerator.Dispose(); | ||
| gcHandle.Free(); |
| using System; | ||
| using System.Runtime.InteropServices.Marshalling; | ||
| using Microsoft.Diagnostics.DataContractReader.Contracts; |
| private readonly Target _target; | ||
| private readonly TargetPointer _module; | ||
| private readonly uint _token; | ||
| private readonly IXCLRDataMethodDefinition? _legacyImpl; | ||
| public ClrDataMethodDefinition( | ||
| Target target, | ||
| TargetPointer module, | ||
| uint token, | ||
| IXCLRDataMethodDefinition? legacyImpl) | ||
| { | ||
| _target = target; | ||
| _module = module; | ||
| _token = token; | ||
| _legacyImpl = legacyImpl; |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs
Outdated
Show resolved
Hide resolved
| string typePortion = fullName[..dotPos]; | ||
| string methodName = fullName[Math.Min(dotPos + 1, fullName.Length - 1)..]; | ||
|
|
||
| _typeHandle = ResolveType(_reader, typePortion); | ||
| if (_typeHandle != null) | ||
| { | ||
| _methodName = methodName; | ||
| break; | ||
| } | ||
|
|
||
| searchFrom = dotPos - 1; | ||
| } | ||
|
|
||
| if (_typeHandle == null) | ||
| throw new ArgumentException(); | ||
| MethodEnumerator = IterateMethodInstances().GetEnumerator(); |
There was a problem hiding this comment.
Exactly. The start method is supposed to validate the type, not the method name.
| try | ||
| { | ||
| if (method.IsNullRef) | ||
| throw new NullReferenceException(); | ||
| GCHandle gcHandle = GCHandle.FromIntPtr((IntPtr)(*handle)); | ||
| if (gcHandle.Target is not EnumMethodDefinitions emdLocal) | ||
| throw new ArgumentException("Type not found"); | ||
|
|
||
| emd = emdLocal; | ||
| } | ||
| catch (System.Exception ex) | ||
| { | ||
| return ex.HResult; | ||
| } | ||
|
|
| if (emd.MethodEnumerator.MoveNext()) | ||
| { | ||
| uint token = emd.MethodEnumerator.Current; | ||
| method.Interface = new ClrDataMethodDefinition(_target, _address, token, legacyMethod); |
a98cfa7 to
7cb5ac8
Compare
7cb5ac8 to
55ab5de
Compare
Tested locally by using bpmd on various methods (including ctors, and with and without specified namespace) and ensuring that they bind and pass debug validation.