-
Notifications
You must be signed in to change notification settings - Fork 215
[RuntimeAsync] Feedback from the merging PR in the runtime repo. #3095
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
Conversation
I think it is time to merge this as we need to update to the latest API shape (i.e. |
|
||
SoftwareExceptionFrame exceptionFrame; | ||
#ifdef TARGET_X86 | ||
_ASSERTE(!"Introduce `IL_ThrowExact_x86` following the pattern of other Throw helpers."); |
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.
Does it mean the EH test fails on x86 now? Can we disable it somehow?
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.
x86 does not do FEATURE_EH_FUNCLETS
right now so it will work. Thus we can postpone introducing IL_ThrowExact_x86
.
When x86 is moved onto FEATURE_EH_FUNCLETS
plan, which I understand will happen at some point, we can remove HELPER_METHOD_FRAME_BEGIN_ATTRIB_NOPOLL
path, which is desirable, but will also need to implement IL_ThrowExact_x86
. It takes an extra parameter, I have only vague idea why.
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.
All runtime tests should be passing right now, including async and including EH - that is on win-x64 and win-x86, where it is easy to test once in a while.
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.
When x86 is moved onto FEATURE_EH_FUNCLETS plan, which I understand will happen at some point,
It is expected to happen very soon. The changes are in dotnet/runtime#113576. The last remaining item before this gets enabled is private diagnostic tests run. If you would like to get ahead of this, you can apply the 10 line change locally and do the required fixes to make things work.
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.
Context about last testing item: dotnet/runtime#113985 (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.
It takes an extra parameter, I have only vague idea why.
win-x86 doesn't have unwinding information for native code. Thus we need to capture the precise register context in assembly code before giving control to the C code since we cannot run VirtualUnwind to get it.
This should be fairly mechanical change so I can make a PR for the 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.
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.
LGTM
Thanks! |
281ac4b
into
dotnet:feature/async2-experiment
This is to keep track of the changes done to address PR feedback in dotnet/runtime#114675