-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Bring throw helpers to PUSH_COOP_PINVOKE_FRAME plan #123015
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @mangod9 |
1a243a6 to
00c4f6a
Compare
b8782d2 to
0d677dd
Compare
0d677dd to
5359b4c
Compare
This reverts commit af084b8.
Added MXCSR reset to prevent floating point exceptions during JIT compilation on AMD64 Windows.
|
@janvorli, # in powershell
> ./build.cmd clr+libs -rc Checked
> src/tests/build.cmd Checked -Dir JIT/Directed -Dir JIT/CodeGenBringUpTests -Dir Exceptions -Dir JIT/IL_Conformance /p:LibrariesConfiguration=Debug
> $CORE_ROOT=$env:CORE_ROOT="$pwd/artifacts/tests/coreclr/windows.x64.Debug/Tests/Core_Root"
> artifacts\tests\coreclr\windows.x64.Checked\JIT\IL_Conformance\IL_Conformance\IL_Conformance.cmd
...
END EXECUTION - PASSED
PASSEDCI is showing https://helix.dot.net/api/2019-06-17/jobs/36ba1a26-3370-47a0-a24a-e0b2d2e41006/workitems/IL_Conformance/console: |
|
OK with |
|
|
||
| #ifdef UNIX_AMD64_ABI | ||
| // On Unix AMD64, argument registers are saved in the transition block | ||
| m_Context.Rax = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to store non-volatile registers and control registers only. The EH should not need anything else. So on Unix, we don't need the float state at all. And the general purpose argument registers are also not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going by #116375 (comment), which is why I added FP registers (and took a while to figure out the right restore points). Do you want me to revert FP registers and use PROLOG_WITH_TRANSITION_BLOCK ? I can test if it's sufficient for unwind. Note that we are replacing PAL_VirtualUnwind call in excep.cpp, where libunwind does account for FP registers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was on Unix Amd64 only. Other architectures need to restore some float registers, as they are non-volatile there. Only Unix Amd64 has no non-volatile FP regs.
So for Unix Amd64, let's not capture any FP regs and let's not copy them here - and thus not set the CONTEXT_FLOATING_POINT in the ContextFlags.
| ; which can corrupt MXCSR (setting it to 0x20 with all FP exception masks cleared). | ||
| ; Without this reset, FP operations after exception handling would trigger exceptions. | ||
| ; 0x1F80 = default MXCSR: all exception masks set, round to nearest, no exceptions pending | ||
| push 1F80h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of any case that would have the CONTEXT_FLOATING_POINT set and not have the MXCSR valid in the CONTEXT. We've never had such a problem and you have initialization of that in your new code to update CONTEXT from the TransitionFrame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without 22571b2 and dd15723, one or more tests in JIT/Directed, JIT/CodeGenBringUpTests and Exceptions were failing (#123015 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is likely caused by something else. I don't see how your change (as a whole) could introduce such a problem.
src/coreclr/vm/prestub.cpp
Outdated
| PCODE pbRetVal = (PCODE)NULL; | ||
|
|
||
| #if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) | ||
| // Reset MXCSR to default value to prevent FP exceptions during JIT compilation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a problem with MXCSR getting messed up, it is not sufficient to reset it in prestub. We need to reset it earlier before resuming execution of managed code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved it to CallEHFilterFunclet.
Fixes #116375.