Skip to content

JIT: Stop reporting FP locals as being on the x87 stack for 32-bit platforms #98580

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

Closed
wants to merge 2 commits into from
Closed

JIT: Stop reporting FP locals as being on the x87 stack for 32-bit platforms #98580

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db7d80a
JIT: Stop reporting FP locals as being on the x87 stack for 32-bit pl…
jakobbotsch Feb 16, 2024
c80fc7d
Stop testing in CI
jakobbotsch Feb 16, 2024
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
19 changes: 0 additions & 19 deletions src/coreclr/jit/scopeinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,28 +403,12 @@ void CodeGenInterface::siVarLoc::siFillRegisterVarLoc(
break;
#endif // !TARGET_64BIT

#ifdef TARGET_64BIT
case TYP_FLOAT:
case TYP_DOUBLE:
// TODO-AMD64-Bug: ndp\clr\src\inc\corinfo.h has a definition of RegNum that only goes up to R15,
// so no XMM registers can get debug information.
Comment on lines -409 to -410
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is still true (though RegNum is no longer is corinfo.h, apparently).

See CodeGen::checkICodeDebugInfo(). We set vlrReg and assume that our register number values are the same as the RegNum values. RegNum doesn't define the FP registers, but we still set it. Seems odd. I presume the debugger ignores it?

Copy link
Member Author

@jakobbotsch jakobbotsch Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well the comment might be true, but I think the real problem is that the current RegNumber is actually GeneralPurposeRegNumber, and the value stored in vlReg.vlrReg for VLT_REG_FP is some FloatingPointRegNumber enum that doesn't exist (i.e. xmm0 gets index 0). The debugger handles it here for x64:

runtime/src/coreclr/debug/di/rsthread.cpp

Lines 8405 to 8411 in efe8b87

#if defined(TARGET_64BIT) || defined(TARGET_ARM)
case ICorDebugInfo::VLT_REG_FP:
#if defined(TARGET_ARM) // @ARMTODO
hr = E_NOTIMPL;
#elif defined(TARGET_AMD64)
hr = m_nativeFrame->GetLocalFloatingPointValue(pNativeVarInfo->loc.vlReg.vlrReg + REGISTER_AMD64_XMM0,
type, ppValue);

I think I'll need to add some similar handling for x86 and run the debugger tests as part of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be checked in the managed ObjWriter too since it expects the JIT register number (ie. not zero based).

this->vlType = VLT_REG_FP;
this->vlReg.vlrReg = varDsc->GetRegNum();
break;

#else // !TARGET_64BIT

case TYP_FLOAT:
case TYP_DOUBLE:
if (isFloatRegType(type))
{
this->vlType = VLT_FPSTK;
this->vlFPstk.vlfReg = varDsc->GetRegNum();
}
break;
Comment on lines -417 to -424
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, this isn't necessary for ABI purposes right? Floating-point args still get returned in ST(0) for Windows (and I think Unix as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so, since this handling is around locals.


#endif // !TARGET_64BIT

#ifdef FEATURE_SIMD
case TYP_SIMD8:
case TYP_SIMD12:
Expand All @@ -436,9 +420,6 @@ void CodeGenInterface::siVarLoc::siFillRegisterVarLoc(
{
this->vlType = VLT_REG_FP;

// TODO-AMD64-Bug: ndp\clr\src\inc\corinfo.h has a definition of RegNum that only goes up to R15,
// so no XMM registers can get debug information.
//
// Note: Need to initialize vlrReg field, otherwise during jit dump hitting an assert
// in eeDispVar() --> getRegName() that regNumber is valid.
this->vlReg.vlrReg = varDsc->GetRegNum();
Expand Down