-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: Fix GC info for fast tailcalls with contained targets #122924
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
For fast tailcalls we consume the operands as part of generating the `GT_CALL` node, yet we do not actually use values in the registers until after generating the epilog. This is incompatible with assumptions made by the code that implements the consumption side of operands. That code assumes that the registers will be used immediately and kills any GC information/local information immediately. Normally this is not a problem. We end up with a wrong GC information view in codegen, but since the emitter uses a lazy approach to register GC information, we don't end up actually reporting the wrong GC information. However, we can see a problem because of various constructs where the emitter ends up synchronizing its GC information view with the view that codegen had -- for example if a label with GC information was created. That started happening recently with dotnet#107283 when we started allowing tailcalls out of methods with GS cookie checks. Fix the situation by remarking the base/index of contained indirections as containing GC pointers if necessary. There is already corresponding logic for arguments that does the same remarking. The fix is only needed on x64/x86 since other platforms do not support contained indirections in calls and hence the target will not usually contain any GC pointers.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
PTAL @dotnet/jit-contrib |
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 fixes a GC information tracking bug for fast tailcalls with contained indirect call targets on x64/x86. When registers containing GC pointers are consumed for a contained indirection but remain live into the epilog, the GC tracking information needs to be explicitly restored after consumption. The fix follows the same pattern used for argument registers in fast tailcalls.
Key Changes:
- Added GC pointer remarking logic for base/index registers of contained indirections in fast tailcall targets
- Updated validation logic to skip all non-variable pointer registers for tailcall blocks (not just argument registers)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/codegenxarch.cpp | Adds logic to remark base/index registers as GC pointers after consuming a contained indirection for fast tailcall targets, ensuring GC tracking remains correct through the epilog |
| src/coreclr/jit/codegenlinear.cpp | Changes validation logic to skip all non-variable pointer registers (not just argument registers) for blocks with tailcalls, since target registers may also be live into the epilog; updates comment to reflect this broader scope |
| if (indir->HasIndex() && indir->Index()->TypeIs(TYP_BYREF, TYP_REF)) | ||
| { | ||
| gcInfo.gcMarkRegPtrVal(indir->Index()->GetRegNum(), indir->Index()->TypeGet()); | ||
| } |
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 Base or Index be contained, hence no GetRegNum here?
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 so. At least the codegen for the target address of the GT_CALL does not have any special handling for contained base/index:
runtime/src/coreclr/jit/codegenxarch.cpp
Lines 6243 to 6257 in dd0661f
| GenTreeIndir* indir = target->AsIndir(); | |
| regNumber iReg = indir->HasBase() ? indir->Base()->GetRegNum() : REG_NA; | |
| regNumber xReg = indir->HasIndex() ? indir->Index()->GetRegNum() : REG_NA; | |
| // These should have been put in volatile registers to ensure they do not | |
| // get overridden by epilog sequence during tailcall. | |
| assert(!params.isJump || (iReg == REG_NA) || ((RBM_CALLEE_TRASH & genRegMask(iReg)) != 0)); | |
| assert(!params.isJump || (xReg == REG_NA) || ((RBM_CALLEE_TRASH & genRegMask(xReg)) != 0)); | |
| params.callType = EC_INDIR_ARD; | |
| params.ireg = iReg; | |
| params.xreg = xReg; | |
| params.xmul = indir->Scale(); | |
| params.disp = indir->Offset(); | |
| genEmitCallWithCurrentGC(params); |
For fast tailcalls we consume the operands as part of generating the
GT_CALLnode, yet we do not actually use values in the registers until after generating the epilog.This is incompatible with assumptions made by the code that implements the consumption side of operands. That code assumes that the registers will be used immediately and kills any GC information/local information immediately.
Normally this is not a problem. We end up with a wrong GC information view in codegen, but since the emitter uses a lazy approach to register GC information, we don't end up actually reporting the wrong GC information. However, we can see a problem because of various constructs where the emitter ends up synchronizing its GC information view with the view that codegen had -- for example if a label with GC information was created. That started happening recently with #107283 when we started allowing tailcalls out of methods with GS cookie checks.
Fix the situation by remarking the base/index of contained indirections as containing GC pointers if necessary. There is already corresponding logic for arguments that does the same remarking.
The fix is only needed on x64/x86 since other platforms do not support contained indirections in calls and hence the target will not usually contain any GC pointers.
Fix #122544