-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add debug information for runtime async methods #120303
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?
Conversation
- Add new JIT-EE API to report back debug information about the generated state machine and continuations - Refactor debug info storage on VM side to be more easily extensible. The new format has either a thin or fat header. The fat header is used when we have either uninstrumented bounds, patchpoint info, rich debug info or async debug info, and stores the blob sizes of all of those components in addition to the bounds and vars. - Add new async debug information to the storage on the VM side - Set get target method desc for async resumption stubs, to be used for mapping from continuations back to the async IL function that it will resume.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
// Offset in continuation's data where this variable is stored | ||
uint32_t Offset; |
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.
We probably need to include GCData
offset here too, but I expect that we will end up with customized continuation layouts during .NET 11 that gets rid of the Data
/GCData
split, so I designed some of these types around that expectation. It also should be possible to derive the GCData offset by iterating the vars linearly, so we can probably make do with that for now.
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.
Storing variables in arrays may work well for some scenarios, like AOT, which is sensitive to generation of types or GC layouts.
I think uint32_t Offset;
may still work if it means "if Offset is greater than GCData length, subtract the length of GCData and continue in non-GC data"
Just in case we may end up using more than one shape, perhaps add some versioning/flavor marker in the AsyncSuspensionPoint
?
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 think uint32_t Offset; may still work if it means "if Offset is greater than GCData length, subtract the length of GCData and continue in non-GC data"
For some values we have data stored in both Data
and GCData
arrays, and to reconstruct the value we need both offsets. Perhaps we can just do 2x 16-bit offsets in this field until we get tailored continuation layouts. I think it's something to investigate further once we look at actually implementing the diagnostics that access the continuation values.
Just in case we may end up using more than one shape, perhaps add some versioning/flavor marker in the AsyncSuspensionPoint?
I expect we stabilize on something during .NET 11, so if we change it after I think we can take an R2R version update at the same time as well. If we do it like that we can version based on R2R module version (it's already what we do in DebugInfo.cs
).
CONTRACTL | ||
{ | ||
NOTHROW; | ||
THROWS; |
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.
RestorePatchpointInfo
is called from JitPatchpointWorker
, which has STANDARD_VM_CONTRACT
. I think throwing should be ok, since we only throw here on an internal inconsistency in the compressed data when encountered by NibbleReader
. That's consistent with the other Restore
routines and saves us having to write separate decoding routines for the patchpoint info.
}; | ||
|
||
#define DebugInfoBoundsHasInstrumentedBounds 0xFFFFFFFF | ||
#define DebugInfoFat 0 |
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.
Switched this to 0 since encoding 0xFFFFFFFF
takes up a full 6 bytes, and the value 0 is not a common value for cbBounds
(JIT never reports empty bounds over all 1+ million methods in our SPMI collections, I suspect we always have at minimum a prolog bound)
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.
Although I am somewhat worried it is possible to see this from the R2R path. I wonder if we should update R2R major version and teach crossgen2 to produce the fat header if necessary. We'll probably need to encode the debug info in R2R anyway during .NET 11, so I guess we might as well take that update now.
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.
@davidwrighton Any thoughts on this?
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.
Updating the R2R major version, and such is the thing to do.
CompressDebugInfo::RestoreRichDebugInfo( | ||
fpNew, pNewData, | ||
pDebugInfo, | ||
ppInlineTree, pNumInlineTree, | ||
ppRichMappings, pNumRichMappings); | ||
|
||
return TRUE; | ||
} |
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 figured I might as well hook this one up in ReadyToRunJitManager
too since the format now technically allows for R2R images that contain the rich debug info, eeven if crossgen2 doesn't produce it.
--- | ||
mode: 'agent' | ||
tools: ['githubRepo', 'codebase', 'terminalLastCommand'] | ||
tools: ['fetch', 'codebase', 'runCommands', 'usages', 'search', 'think'] |
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.
Updating these tools based on @EgorBo's instructions (I used the prompt to add the JIT-EE boilerplate, but the agent was not making the changes automatically without allowing it access to these tools)
// Index of inline tree node containing the IL offset (0 for root) | ||
uint32_t Inlinee; | ||
// IL offset that resulted in the creation of the suspension point. | ||
uint32_t ILOffset; |
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.
if we care about size, can IL offsets inside a method be uint16_t
?
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 think there is a 2^16 size limit on IL, but in any case we do not care about size in these data types. The actual storage of these is compressed in the VM, so when these values are small, they consume little space.
struct AsyncInfo | ||
{ | ||
// Number of suspension points in the method. | ||
uint32_t NumSuspensionPoints; |
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.
can this be uint16_t
?
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.
Same reason as above I don't think we need to limit this.
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 adds a new JIT-EE API reportAsyncDebugInfo
to report debug information for runtime async methods. The implementation enables the JIT to provide suspension point and continuation variable information back to the VM for improved debugging of async methods.
Key changes:
- Adds new
reportAsyncDebugInfo
API to the JIT-EE interface with structures for async debug information - Refactors debug info storage format to support extensible "fat headers" for additional debug data
- Bumps ReadyToRun major version to 17 to accommodate the new debug info format
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/tests/async/Directory.Build.props | Adds SYSLIB5007 warning suppression for async tests |
src/coreclr/vm/jitinterface.h | Adds reportAsyncDebugInfo method declaration and member variables for async debug data |
src/coreclr/vm/jitinterface.cpp | Implements reportAsyncDebugInfo method and integrates async debug info into compression |
src/coreclr/vm/debuginfostore.h | Adds structures and methods for handling async debug info in the new fat header format |
src/coreclr/vm/debuginfostore.cpp | Implements compression/decompression of async debug info and fat header logic |
src/coreclr/vm/codeman.h | Adds GetAsyncDebugInfo virtual method to IJitManager and implementations |
src/coreclr/vm/codeman.cpp | Implements async debug info retrieval across different JIT managers |
src/coreclr/tools/superpmi/* | Updates SuperPMI shims to handle the new reportAsyncDebugInfo API |
src/coreclr/tools/aot/* | Adds ReadyToRun support for fat header format and async debug info |
src/coreclr/jit/* | Implements async transformation debug info collection and reporting |
src/coreclr/inc/* | Updates interface definitions, version numbers, and adds async debug structures |
Uh oh!
There was an error while loading. Please reload this page.