-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[cDAC] GetCodeHeaderData fork #120275
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
base: main
Are you sure you want to change the base?
[cDAC] GetCodeHeaderData fork #120275
Conversation
|
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.
Pull Request Overview
This PR implements the GetCodeHeaderData method and adds GC info decoder functionality to the contract DAC (cDAC) system. It introduces a new GCInfo contract with platform-specific implementations for decoding garbage collection metadata from native code.
Key Changes:
- Implements
GetCodeHeaderDatamethod with proper type safety and error handling - Adds a comprehensive GCInfo contract with decoder capabilities for AMD64, ARM64, and ARM architectures
- Extends ExecutionManager with new methods for method region info, JIT type detection, and method descriptor lookup
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| SOSDacImpl.cs | Implements GetCodeHeaderData with GCInfo decoder integration and debug assertions |
| ISOSDacInterface.cs | Updates interface signature and adds supporting data structures |
| CachingContractRegistry.cs | Registers the new GCInfo contract factory |
| GCInfo contract files | Complete GCInfo decoder implementation with platform-specific traits |
| ExecutionManager files | Extends with method region info, JIT type detection, and entrypoint resolution |
| Data structure files | Adds UnwindInfo, PortableEntryPoint, and related helper classes |
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/IGCInfoTraits.cs:1
- Corrected spelling of 'ENCBACE' to 'ENCBASE'.
// Licensed to the .NET Foundation under one or more agreements.
...ed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/GCInfoDecoder.cs
Outdated
Show resolved
Hide resolved
....Diagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/ARMGCInfoTraits.cs
Outdated
Show resolved
Hide resolved
...iagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/ARM64GCInfoTraits.cs
Outdated
Show resolved
Hide resolved
...iagnostics.DataContractReader.Contracts/Contracts/GCInfo/PlatformTraits/AMD64GCInfoTraits.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.
Pull Request Overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
....Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/FrameIterator.cs
Show resolved
Hide resolved
...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs
Outdated
Show resolved
Hide resolved
|
|
||
| ## Implementation | ||
|
|
||
| The GCInfo contract has platform specific implementations as GCInfo differs per architecture. With the exception of x86, all platforms have a common encoding scheme with different encoding lengths and normalization functions for data. x86 uses an entirely different scheme which is not currently supported by this contract. |
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.
How do we handle the interpreter. It also uses the GC info.
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.
Mirroring how the runtime handles this case, I believe it is acceptable to have a different entrypoint to decode the 'standard platform' code and the interpreter code.
IGCInfoHandle IGCInfo.DecodeGCInfo(TargetPointer gcInfoAddress, uint gcVersion)IGCInfoHandle IGCInfo.DecodeInterpreterGCInfo(TargetPointer gcInfoAddress, uint gcVersion)
I don't believe this can be encapsulated in the current format as the ExecutionManager calls into the GCInfo contract to find the method region info. If the GCInfo contract differentiated between interpreted and jitted code, then it would need to call into ExecutionManager creating a cyclic dependency.
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.
Shall we use similar arch-specific structure as: src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/{arch}/{arch}Unwinder.cs or bring that to this (single directory) plan?
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.
I don't have a strong feeling either way. I didn't make /{arch}/ subdirectories here because each platform only has a single file. In the future we may have versions of traits and multiple files. I think it would make sense to have specific directories.
fork of #119609 with GCInfo decoder