JIT: Preserve async continuation arg across EnC remap#128664
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR adjusts CoreCLR JIT frame layout for async methods under Edit-and-Continue (EnC) so the async continuation argument’s stack home is placed in the EnC-preserved “frame header” region, and updates GC info preserved-area sizing on x64 and ARM64 accordingly.
Changes:
lclvars.cpp: Skips normal arg/local offset assignment forlvaAsyncContinuationArgunder EnC and allocates its stack slot vialvaAllocAsyncContexts.codegenxarch.cpp: Extends EnC preserved-area size to include the continuation slot on x64.codegenarmarch.cpp: Extends EnC preserved-area size to include the continuation slot on ARM64.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/lclvars.cpp | Changes EnC frame-layout allocation/skip logic so the async continuation arg is intended to live in the EnC-preserved header region. |
| src/coreclr/jit/codegenxarch.cpp | Updates x64 EnC preserved-area size computation to account for the continuation slot. |
| src/coreclr/jit/codegenarmarch.cpp | Updates ARM64 EnC preserved-area size computation to account for the continuation slot. |
|
How does the generic context get handled? We should align these. |
Generic context is stored as an explicit parameter so is automatically stored into |
|
I wonder if we should give this a number instead. I am not totally sure if things will work out correctly in all cases if we try to allocate an argument together with locals. |
I think it makes sense. The variable already has a normal parameter home, so we would simply be assigning it a proper IL value, like |
I believe this approach will require a R2R major version bump, because NativeVarInfo isn't versioned today. |
|
I think that should be ok, we can combine it with the change for #128397. |
@jakobbotsch you are correct - the R2R major version was bumped on #122704 which is during .NET 11. @jkotas do you see any issues with breaking the R2R format across daily .NET 11 builds and previews? |
I do not see any issues, as you have said we started a new compat tier for .NET 11 already. |
|
I thought that you are asking about whether it is ok to bump the version. We like to bump the version whenever we break the format to save us debugging weird crashes caused by mismatched versions. |
Ahh - I thought there was agreement on no version bump. Bringing it back! |
| private const uint MAX_ILNUM = unchecked((uint)-5); | ||
|
|
There was a problem hiding this comment.
@rcj1 @max-charlamb just checking before dismissing this comment - do you see any concerns with adding ASYNC_CONTINUATION_ILNUM (-4) toNativeVarInfo without adjusting for older builds?
|
FYI - it looks like we just took another R2R breaking change in main |
Change this one to 20 to resolve the conflict. |
When async V2 is enabled and EnC is active, the JIT now assigns ASYNC_CONTINUATION_ILNUM (-4) to the async continuation arg variable instead of UNKNOWN_ILNUM (-4, previously). This allows the debugger to identify and preserve this variable across EnC remaps. Also adds compMapILvarNum reverse mapping case and fixes a typo in cordebuginfo.h. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The ASYNC_CONTINUATION_ILNUM addition changes MAX_ILNUM from -4 to -5, which alters the NativeVarInfo encoding bias. Bump the R2R major version so old images are rejected rather than silently misinterpreting variable info. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2e5c78f to
683aa07
Compare
| #define READYTORUN_MAJOR_VERSION 20 | ||
| #define READYTORUN_MINOR_VERSION 0x0000 | ||
|
|
||
| #define MINIMUM_READYTORUN_MAJOR_VERSION 19 | ||
| #define MINIMUM_READYTORUN_MAJOR_VERSION 20 | ||
|
|
The async continuation argument was homed in the local frame area which gets zeroed by FixContextForEnC during EnC remap. On platforms with shadow space, the continuation accidentally survived because it was homed above callerSP, outside the zeroed region. On platforms without shadow space, the continuation was destroyed.
Fix: Assign a dedicated IL number (ASYNC_CONTINUATION_ILNUM = -4) so the continuation arg appears in NativeVarInfo and survives EnC remap on all platforms.
This shifts MAX_ILNUM from -4 to -5, changing the NativeVarInfo encoding bias. R2R major version bumped 18→19 accordingly.