-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix mach hardware exception forwarding #105003
Conversation
A recent change to enable AVX512 register state processing in the mach exception handling on macOS x64 has broken a PAL test and also a case when a hardware exception occurs in 3rd party native code. In both cases runtime hangs, another exception occurs while forwarding the exception message and that ends up happenning infinitely. The problem is caused by the fact that the MachMessageInfo has grown, since we've changed a field containing float state to contain avx512 state instead. But the buffer that stores the message is of fixed size of 1500 bytes and it is not sufficient. I have grown the buffer size to the necessary size, but there was another issue lurking behind the first one. The MachExceptionInfo::RestoreState was trying to restore the float state always as x86_FLOAT_STATE instead of choosing x86_FLOAT_STATE, x86_AVX_STATE or x86_AVX512_STATE depending of which of them was stored. So the thread_set_state was failing. This change fixes both of the problems.
@jkotas I've simplified the state restoring. I've found that the count is stored in the state as well, so I don't need to have the switch. |
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -189,7 +189,7 @@ class MachMessage | |||
// The maximum size in bytes of any Mach message we can send or receive. Calculating an exact size for | |||
// this is non trivial (basically because of the security trailers that Mach appends) but the current | |||
// value has proven to be more than enough so far. | |||
static const size_t kcbMaxMessageSize = 1500; | |||
static const size_t kcbMaxMessageSize = 0x1500; |
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.
Would it be better to define this as sizeof(mach_message_t)
plus some extra space?
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 actually found a constant in the headers representing the maximum trailer size, so we can define it as sizeof(mach_message_t) + MAX_TRAILER_SIZE and be safe.
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.
Added a commit with that change.
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.
Is the MAX_TRAILER_SIZE in a mach source file header? And I am guessing it refers to the security trailers that are mentioned in the comment above?
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.
Yes, it is in the Apple headers and it refers to those trailers.
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.
Thanks for catching this!
A recent change to enable AVX512 register state processing in the mach exception handling on macOS x64 has broken a PAL test and also a case when a hardware exception occurs in 3rd party native code. In both cases runtime hangs, another exception occurs while forwarding the exception message and that ends up happenning infinitely.
The problem is caused by the fact that the MachMessageInfo has grown, since we've changed a field containing float state to contain avx512 state instead. But the buffer that stores the message is of fixed size of 1500 bytes and it is not sufficient.
I have grown the buffer size to the necessary size, but there was another issue lurking behind the first one. The MachExceptionInfo::RestoreState was trying to restore the float state always as x86_FLOAT_STATE instead of choosing x86_FLOAT_STATE, x86_AVX_STATE or x86_AVX512_STATE depending of which of them was stored. So the thread_set_state was failing.
This change fixes both of the problems.
Close #104968