Skip to content

Fix struct ReturnKind on SysV AMD64. #116194

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jun 9, 2025
Merged

Fix struct ReturnKind on SysV AMD64. #116194

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3784150
Fix struct ReturnKind on SysV AMD64.
zengandrew Jun 1, 2025
5730cd0
Delete unreachable code
jkotas Jun 4, 2025
9a718cb
Update src/coreclr/jit/gcencode.cpp
jkotas Jun 4, 2025
1760969
Cleanup MethodDesc::GetReturnKind
jkotas Jun 4, 2025
ae4fdfc
Update src/coreclr/vm/method.hpp
jkotas Jun 4, 2025
c1c609e
Update src/coreclr/vm/method.hpp
jkotas Jun 4, 2025
a5c9613
Delete dead code
jkotas Jun 8, 2025
dbd7587
More cleanup
jkotas Jun 9, 2025
44fa203
Fix contract violation
jkotas Jun 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/coreclr/jit/gcencode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,21 @@ ReturnKind GCInfo::getReturnKind()
case 1:
return VarTypeToReturnKind(retTypeDesc.GetReturnRegType(0));
case 2:
return GetStructReturnKind(VarTypeToReturnKind(retTypeDesc.GetReturnRegType(0)),
VarTypeToReturnKind(retTypeDesc.GetReturnRegType(1)));
{
var_types first = retTypeDesc.GetReturnRegType(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a commit with the cleanup

var_types second = retTypeDesc.GetReturnRegType(1);
#ifdef UNIX_AMD64_ABI
if (varTypeUsesFloatReg(first))
{
// 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;
second = TYP_UNDEF;
}
#endif // UNIX_AMD64_ABI
return GetStructReturnKind(VarTypeToReturnKind(first),
VarTypeToReturnKind(second));
}
default:
#ifdef DEBUG
for (unsigned i = 0; i < regCount; i++)
Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/vm/method.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1299,6 +1299,15 @@ ReturnKind MethodDesc::ParseReturnKindFromSig(INDEBUG(bool supportStringConstruc
regKinds[i] = RT_Scalar;
}
}

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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ReturnKind structReturnKind = GetStructReturnKind(regKinds[0], regKinds[1]);
return structReturnKind;
}
Expand Down