Fix struct ReturnKind on SysV AMD64. #116194
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
github-actions
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
|
@dotnet-policy-service agree |
jkotas
added
area-CodeGen-coreclr
src/coreclr/jit/gcencode.cpp
Outdated
| return GetStructReturnKind(VarTypeToReturnKind(retTypeDesc.GetReturnRegType(0)), | ||
| VarTypeToReturnKind(retTypeDesc.GetReturnRegType(1))); | ||
| { | ||
| var_types first = retTypeDesc.GetReturnRegType(0); |
There was a problem hiding this comment.
This is the right fix for backport. In main, this path is unreachable in main - the whole method can be under TARGET_X86 ifdef and simplified to only handle a single register.
@jakobbotsch How would you like to handle this?
There was a problem hiding this comment.
@zengandrew Thanks so much for filing the fix here. I have cherry-picked the commit from this PR directly into the two backports (#116206, #116208).
Given the testing it appears to be unnecessary for the 9.0 backport. However 9.0 still has the code around return kinds enabled for x64. That code was removed in #110799 which is not part of 9.0. The actual PRs that made it unnecessary are #95565 and #104336 that are part of 9.0. But, just to be safe, since the code is actually running during hijack, I think we should include it as well.
For main I think we should skip the change (mainly to avoid the misleading commit history, since this code is unused). And indeed we should clean this up (cc @VSadov if you want to do it).
There was a problem hiding this comment.
I have pushed a commit with the cleanup
src/coreclr/vm/method.cpp
Outdated
| if (eeClass->GetEightByteClassification(0) == SystemVClassificationTypeSSE) | ||
| { | ||
| // Skip over SSE types since they do not consume integer registers. | ||
| // An obj/byref in the 2nd eight bytes will be in the first integer register. | ||
| regKinds[0] = regKinds[1]; | ||
| regKinds[1] = RT_Scalar; | ||
| } |
There was a problem hiding this comment.
Can ParseReturnKindFromSig be simplified too? It is only used by MethodDesc::GetReturnKind and that one has only uses from GCStress (which is under TARGET_X86) and CLRToCOMMethodFrame::GcScanRoots_Impl (which does not try to handle the multireg cases, but with a NYI assert.).
The ReturnKind enum could probably be cleaned up too after that.
There was a problem hiding this comment.
The ReturnKind enum could probably be cleaned up too after that.
GCInfo decoder source is shared with diagnostic repo. It needs to support older versions of the format. The no longer used ReturnKind values need to stay for that.
Can ParseReturnKindFromSig be simplified too?
Done
src/coreclr/vm/method.cpp
Outdated
| } | ||
| #endif // UNIX_AMD64_ABI | ||
|
|
||
| if (pReturnTypeMT->ContainsGCPointers() || pReturnTypeMT->IsByRefLike()) |
There was a problem hiding this comment.
If we have pReturnTypeMT->IsByRefLike(), should we return RT_ByRef ?
Or, if we cannot distinguish RT_Object from RT_ByRef, would RT_ByRef be safer to return?
There was a problem hiding this comment.
Does pReturnTypeMT->IsByRefLike() actually imply that there must be a GC ref?
I think at C# level you can make any struct to be stack-only, it does not need to contain pointers or refs. If IsByRefLike() is true for all ref structs, there may not be a pointer.
Perhaps
pReturnTypeMT->ContainsGCPointers() should return RT_Object and
pReturnTypeMT->ContainsGCPointers() && pReturnTypeMT->IsByRefLike() should return RT_ByRef
Or maybe for simplicity just return RT_ByRef for pReturnTypeMT->ContainsGCPointers()?
There was a problem hiding this comment.
It is not the part that is being fixed, but since we are touching this, and it looks a bit suspicious,...
There was a problem hiding this comment.
Ok, I have done more cleanup. This method is only used for GC stress on x86 now. GC stress can tolerate RT_Illegal return (it did that before this change as well, so it is not a coverage regression).
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
jkotas
force-pushed
the
zengandrew-fix-115815
branch
from
c496662 to
dbd7587
Compare
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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 #115815