Fix async continuation hijacking for x86 NativeAOT#128706
Conversation
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
There was a problem hiding this comment.
Pull request overview
This PR updates x86 GC/hijack metadata so async continuations can be reported as GC roots when NativeAOT hijacks async methods.
Changes:
- Adds an async bit to x86 GC info encoding/decoding.
- Plumbs async-return metadata through CoreCLR and NativeAOT hijack helpers.
- Reports the hijacked async continuation from the saved ECX location on x86.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/threadsuspend.cpp | Uses code-manager-provided async hijack info. |
| src/coreclr/vm/gc_unwind_x86.inl | Copies decoded async flag into x86 unwind header info. |
| src/coreclr/vm/eetwain.cpp | Returns async metadata from decoded GC info. |
| src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.h | Extends x86 return-value metadata API. |
| src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp | Decodes async flag with return kind. |
| src/coreclr/nativeaot/Runtime/thread.cpp | Reports hijacked async continuation during root scanning. |
| src/coreclr/nativeaot/Runtime/StackFrameIterator.h | Tracks hijacked async continuation location. |
| src/coreclr/nativeaot/Runtime/StackFrameIterator.cpp | Restores async continuation location from transition frame flags. |
| src/coreclr/nativeaot/Runtime/inc/rhbinder.h | Adds x86 ECX-is-GC-ref transition-frame flag. |
| src/coreclr/nativeaot/Runtime/ICodeManager.h | Encodes/decodes async flag alongside return kind. |
| src/coreclr/nativeaot/Runtime/i386/AsmOffsetsCpu.h | Updates x86 structure offsets after iterator layout change. |
| src/coreclr/jit/gcencode.cpp | Emits async metadata in x86 GC info. |
| src/coreclr/inc/gcinfotypes.h | Expands x86 GC header metadata for async state. |
| src/coreclr/inc/gcdecoder.cpp | Decodes async metadata from x86 GC info. |
| src/coreclr/inc/gc_unwind_x86.h | Adds async flag to decoded x86 header info. |
| src/coreclr/inc/eetwain.h | Extends CoreCLR hijack-info API signature. |
| SET_EPILOGCNT_MAX = 4, | ||
| SET_UNTRACKED_MAX = 3, | ||
| SET_RET_KIND_MAX = 3, // 2 bits for ReturnKind | ||
| SET_RET_KIND_MAX = 7, // 3 bits for ReturnKind + isAsync |
There was a problem hiding this comment.
SOS/diagnostics decoders
Also, R2RDump in src\coreclr\tools\aot\ILCompiler.Reflection.ReadyToRun\x86.
We try to handle both old and new version in these decoders. They should switch based on the GC info version.
There was a problem hiding this comment.
Opened dotnet/diagnostics#5862 for the diagnostics equivalent and also updated various places in this PR to be backwards compatible in the parsing.
|
@VSadov How hard would it be to switch windows x86 to the same plan for GC reporting around hijacked address instead of doing this? I know you intentionally left x86 on the old plan, but I do not remember the reasons. |
I think the reasons for not switching x86 to the same GC encoding plan as in x64/arm64 were:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/coreclr/inc/gcdecoder.cpp:215
decodeHeadernow treats second-opcode values 0x04-0x07 asreturnKind/isAsyncregardless of theversionparameter. For GCInfo v3 ReadyToRun images those same opcodes are the existingSET_NOGCREGIONS_CNTencodings, so old x86 GC info will be decoded with the wrong header state. Please branch on the GC info version here (and keep the old opcode layout for v3) before interpreting bit 2 as the async flag.
if (encoding <= SET_RET_KIND_MAX)
{
header->returnKind = (ReturnKind)encoding & 3;
header->isAsync = (encoding & 4) != 0;
}
else if (encoding < FFFF_NOGCREGION_CNT)
{
header->noGCRegionCnt = encoding - SET_NOGCREGIONS_CNT;
| #define READYTORUN_MAJOR_VERSION 21 | ||
| #define READYTORUN_MINOR_VERSION 0x0000 |
There was a problem hiding this comment.
Just trying to predict some of the version bumps happening in other PRs...
NativeAOT hijacking did not properly report the async continuation when hijacking an async function. While we did save and restore the register (implemented in #123084), the part that actually determined whether the register contained a GC ref was missing.
The GC reporting for return values is handled differently on x86 compared to other platforms. On other platforms the JIT reports GC information on the caller's return address instruction which means that the continuation gets reported naturally. This is not the case for x86 where this must be handled specially.
Fix #128624
Fix #127900