-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Adding GetMethodTableFieldData cDAC API #117684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the GetMethodTableFieldData
cDAC API, adds the corresponding data structures and mappings, and updates contracts, metadata layouts, and documentation to support querying instance, static, and thread-static field information from a MethodTable
.
- Implemented
GetMethodTableFieldData
inSOSDacImpl
with debug-only consistency checks against legacy DAC. - Added
DacpMethodTableFieldData
struct to the interface and propagated new field descriptors through the data contracts, runtime type system, native layout, and documentation. - Updated contracts (
EEClass
,IRuntimeTypeSystem
,RuntimeTypeSystem_1
), coreclr headers (class.h
,methodtable.h
,datadescriptor.h
), and design docs.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs | Added implementation of GetMethodTableFieldData |
src/native/managed/cdac/mscordaccore_universal/Legacy/ISOSDacInterface.cs | Defined DacpMethodTableFieldData struct |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/EEClass.cs | Extended EEClass data contract with new field counts and list |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs | Added field-related query methods (GetNum*Fields , GetFieldDescList , etc.) |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs | Declared new field-related methods in the runtime type system interface |
src/coreclr/vm/methodtable.h | Minor cleanup (removed extra blank line) |
src/coreclr/vm/class.h | Added offset constants for new EEClass fields |
src/coreclr/debug/runtimeinfo/datadescriptor.h | Added CDAC_TYPE_FIELD entries for new EEClass fields |
docs/design/datacontracts/RuntimeTypeSystem.md | Updated documentation and data descriptor tables for new fields |
Comments suppressed due to low confidence (3)
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs:952
- This newly added API lacks any unit or integration tests. Please add tests covering types with instance, static, and thread-static fields to verify the behavior.
int ISOSDacInterface.GetMethodTableFieldData(ClrDataAddress mt, void* data)
docs/design/datacontracts/RuntimeTypeSystem.md:380
- The parameter name 'TypeHandle' shadows the type and uses incorrect casing. Rename it to 'typeHandle' and update references in the method body to avoid confusion.
public TargetPointer GetModule(TypeHandle TypeHandle)
src/native/managed/cdac/mscordaccore_universal/Legacy/ISOSDacInterface.cs:139
- [nitpick] Remove the trailing semicolon after the struct declaration to match C# style and maintain consistency with other struct definitions in this file.
};
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actual implementation logic looks good. Good work on adding the datadescriptors 👍
Comments mostly on moving around of fields in RTS contract.
...d/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/EEClass.cs
Show resolved
Hide resolved
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs
Outdated
Show resolved
Hide resolved
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs
Outdated
Show resolved
Hide resolved
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs
Outdated
Show resolved
Hide resolved
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be good to merge after fixing the one comment
No description provided.