-
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
src/coreclr/vm/jithelpers.cpp
Outdated
| { | ||
| // Create a NullReferenceException and throw it with the correct context | ||
| EEException ex(kNullReferenceException); | ||
| oref = ex.CreateThrowable(); |
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 CreateThrowable throws OutOfMemoryException here, is it going to be handled gracefully?
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 original DispatchManagedException(kNullReferenceException) overload does the same thing internally; it calls CreateThrowable() which can throw OOM. So the behavior is unchanged. The exception frame (SoftwareExceptionFrame) is already set up and we're inside FC_CAN_TRIGGER_GC(), so if OOM is thrown, it will be dispatched normally through the existing exception handling machinery.
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.
As a test, I have added new int[(size_t)1000000000 * (size_t)1000000000]; here and run the following test (on Windows):
using System;
try {
throw null;
}
catch (OutOfMemoryException) {
}
Expected behavior: The exception should be handled by the catch,
Actual behavior: The test failed with Unhandled exception. System.OutOfMemoryException: Exception of type 'System.OutOfMemoryException' was thrown.)
I think we are missing one of those macros that flips between C++ and managed exceptions here. @janvorli What's the right macro to use here?
(It is possible that this is broken in main currently - I have not tested that.)
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. I was primarily testing in CI and observed failures on both linux-arm64 and x64. With arm64 now fixed by updating the offset, I’ll look into what adjustments are needed for x64. In the meantime, I’ve reverted this change.
Fixes #116375.