[release/8.0-staging] Fix SysV first/second return register GC info mismatch #116208
Conversation
On SysV AMD64, structs returned in a float and int reg pair were being classified as RT_Scalar_XX. This causes downstream consumers (e.g., HijackFrame::GcScanRoots) to look for obj/byref's in the second int reg. Per the ABI, however, the first float is passed through a float reg and the obj/byref is passed through the _first_ int reg. We now detect and fix this case by skipping the first float type in the ReturnKind encoding and moving the second type into the first. Fix dotnet#115815
There was a problem hiding this comment.
Pull Request Overview
This PR backports a fix to address a SysV GC info mismatch when generating calls for methods returning two registers. The changes adjust register assignments in the VM and JIT components and include a regression test to validate the fix.
- Adjusted register reordering in vm/method.cpp for precise GC info.
- Updated GC encoding and code generation logic in jit/gcencode.cpp and jit/codegenxarch.cpp.
- Added a regression test in tests/JIT/Regression/JitBlue to capture the issue.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_115815/Runtime_115815.csproj | Added test project file for regression testing |
| src/tests/JIT/Regression/JitBlue/Runtime_115815/Runtime_115815.cs | Added regression test to trigger the GC info mismatch issue |
| src/coreclr/vm/method.cpp | Adjusted register handling for SIMD returning methods |
| src/coreclr/jit/gcencode.cpp | Updated GC info computation for two-register returns under UNIX_AMD64_ABI |
| src/coreclr/jit/codegenxarch.cpp | Fixed retSize assignment when the second register is REG_INTRET |
| } | ||
| } | ||
|
|
||
| if (eeClass->GetEightByteClassification(0) == SystemVClassificationTypeSSE) |
There was a problem hiding this comment.
Consider expanding the comment to clearly explain why swapping regKinds[0] and regKinds[1] corrects the GC info mismatch when the first eight-byte is classified as SSE.
Copilot uses AI. Check for mistakes.
| { | ||
| // first does not consume an int register in this case so an obj/ref | ||
| // in the second ReturnKind would actually be found in the first int reg. | ||
| first = second; |
There was a problem hiding this comment.
It might be helpful to include a brief comment clarifying why setting 'first' to 'second' and then assigning TYP_UNDEF to 'second' is necessary under UNIX_AMD64_ABI.
Copilot uses AI. Check for mistakes.
| retSize = emitTypeSize(retTypeDesc->GetReturnRegType(0)); | ||
| secondRetSize = emitTypeSize(retTypeDesc->GetReturnRegType(1)); | ||
|
|
||
| if (retTypeDesc->GetABIReturnReg(1) == REG_INTRET) |
There was a problem hiding this comment.
Consider adding a comment to explain the rationale for swapping retSize and secondRetSize in situations where the second return register is REG_INTRET, to clarify the underlying assumption about register allocation.
Copilot uses AI. Check for mistakes.
jeffschwMSFT
added
Servicing-consider
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
rbhanda
added
Servicing-approved
jakobbotsch
changed the title
jakobbotsch
changed the title
|
/ba-g Infra issue in irrelevant test job |
jakobbotsch
merged commit de66703
into
dotnet:release/8.0-staging
Backport of #115827 to release/8.0-staging. Includes @zengandrew's change in #116194.
Customer Impact
The JIT may generate incorrect GC information on linux-x64 and osx-x64 for methods that return in two registers if the first returned register is a SIMD register and the second returned register is an integer register containing an object reference.
In that case the generated GC information incorrectly reports an object reference in the
rdxregister that is actually present in theraxregister.This requires a struct that is laid out by the VM as a float followed by an object reference. This is an unusual layout as the VM tries to reorder struct layouts to keep object references at the beginning. Yet the customer reported issue #115815 shows a case where it happens. A reduced test case looks something like:
The GC info mismatch leads to unpredictable outcomes (segfaults, heap corruption, etc.).
The VM has a similar problem when it wants to suspend all running threads if it needs to suspend in a method like
GetValueabove. In those cases the VM may not properly preserve the GC reference returned, and may mistakenly treat thedoubleas a GC reference instead.The PR includes fixes for both the JIT and VM issue.
Regression
As far as I can tell this bug is present since the original introduction of SysV support.
Testing
Regression test introduced.
Risk
Low.