[release/9.0-staging] Fix SysV first/second return register GC info mismatch #116206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
Backport fix for SysV first/second return register GC info mismatch on linux-x64 and osx-x64.
- Added a new regression test to reproduce and guard against the GC info misreport.
- Updated
genCallInstructionto swap return-size metadata when the second SysV return register is an integer, ensuring the GC map reflects the true register holding the object reference.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_115815/Runtime_115815.csproj | Added project file to include the new regression test in the test suite. |
| src/tests/JIT/Regression/JitBlue/Runtime_115815/Runtime_115815.cs | Introduced a stress test that repeatedly calls the problematic pattern to catch misreported GC info. |
| src/coreclr/jit/codegenxarch.cpp | Modified genCallInstruction to detect when the second return ABI register is REG_INTRET and swap retSize/secondRetSize accordingly. |
Comments suppressed due to low confidence (1)
src/tests/JIT/Regression/JitBlue/Runtime_115815/Runtime_115815.cs:12
- The test currently only ensures no crash occurs. Consider adding assertions inside the loops to verify that
GetValue(j)returns the expectedContainer.Nameanddoublevalue, which would catch logical regressions beyond GC misreporting.
public static void TestEntryPoint()
| // The emitter has hardcoded belief that retSize corresponds to | ||
| // REG_INTRET and secondRetSize to REG_INTRET_1, so fix up the | ||
| // situation here. | ||
| assert(!EA_IS_GCREF_OR_BYREF(retSize)); |
There was a problem hiding this comment.
[nitpick] Consider adding a descriptive message to this assert (e.g., && "Expected non-GC return size when swapping for INTRET ABI") to aid debugging if it ever fails.
Suggested change
| assert(!EA_IS_GCREF_OR_BYREF(retSize)); | |
| assert(!EA_IS_GCREF_OR_BYREF(retSize) && "Expected non-GC return size when swapping for INTRET ABI"); |
Copilot uses AI. Check for mistakes.
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
jeffschwMSFT
approved these changes
Jun 2, 2025
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
jkotas
approved these changes
Jun 3, 2025
74a39ca
into
dotnet:release/9.0-staging
106 of 110 checks passed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #115827 to release/9.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.