-
Notifications
You must be signed in to change notification settings - Fork 5k
JIT: introduce durable EH region ID #113497
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
Give each EH clause a permanent ID unique to that clause. Use this to revise how we model GT_END_LFIN's nesting depth -- the node now refers to a region ID, and we use that plus a lookup table built during `FinalizeEH` to figure out the nesting depth. Contributes to dotnet#108900.
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.
Pull Request Overview
This PR updates the documentation to reflect the new approach of using a durable EH region ID instead of a stack level in the JIT’s handling of EH clauses.
- Replaces references to the "stack level" with "EH region ID" for GT_END_LFIN nodes.
- Updates descriptions for BBJ_CALLFINALLY/BBJ_ALWAYS pairs to clarify that the final EH nesting depth is now derived using the EH region ID.
@@ -452,7 +452,7 @@ The code this finally returns to looks like this: | |||
|
|||
In this case, it zeros out the ShadowSP slot that it previously set to 0xFC, then jumps to the address that is the actual target of the leave from the finally. | |||
|
|||
The JIT does this "end finally restore" by creating a GT_END_LFIN tree node, with the appropriate stack level as an operand, that generates this code. | |||
The JIT does this "end finally restore" by creating a GT_END_LFIN tree node, with the appropriate EH region ID as an operand, that generates this 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.
Consider adding a brief note or reference indicating where the definition and derivation of the EH region ID is provided, to help guide readers unfamiliar with this change.
Copilot uses AI. Check for mistakes.
@@ -476,7 +476,7 @@ The VM walks the ShadowSP slots in the function `GetHandlerFrameInfo()`, and set | |||
|
|||
An aside on the JIT implementation for x86. | |||
|
|||
The JIT creates BBJ_CALLFINALLY/BBJ_ALWAYS pairs for calling the 'finally' clause. The BBJ_CALLFINALLY block will have a series of CORINFO_JIT_ENDCATCH calls appended at the end, if we need to "leave" a series of nested catches before calling the finally handler (due to a single 'leave' opcode attempting to leave multiple levels of different types of handlers). Then, a GT_END_LFIN statement with the finally clause handler nesting level as an argument is added to the step block where the finally returns to. This is used to generate code to zero out the appropriate level of the ShadowSP slot array after the finally has been executed. The BBJ_CALLFINALLY block itself generates the code to insert the 0xFC value into the ShadowSP slot array. If the 'finally' is invoked by the VM, in exceptional cases, then the VM itself updates the ShadowSP slot array before invoking the 'finally'. | |||
The JIT creates BBJ_CALLFINALLY/BBJ_ALWAYS pairs for calling the 'finally' clause. The BBJ_CALLFINALLY block will have a series of CORINFO_JIT_ENDCATCH calls appended at the end, if we need to "leave" a series of nested catches before calling the finally handler (due to a single 'leave' opcode attempting to leave multiple levels of different types of handlers). Then, a GT_END_LFIN statement with EH region ID as an argument is added to the step block where the finally returns to. This is used to generate code to zero out the appropriate level of the ShadowSP slot array after the finally has been executed and the final EH nesting depth is known. The BBJ_CALLFINALLY block itself generates the code to insert the 0xFC value into the ShadowSP slot array. If the 'finally' is invoked by the VM, in exceptional cases, then the VM itself updates the ShadowSP slot array before invoking the 'finally'. |
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 may be beneficial to briefly explain how using the EH region ID contributes to determining the final EH nesting depth for improved clarity.
Copilot uses AI. Check for mistakes.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@BruceForstall (or anyone) PTAL Only impacts x86. Should be no diff. |
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
{ | ||
// Find the eh table entry via the eh ID | ||
// | ||
unsigned const ehID = (unsigned)treeNode->AsVal()->gtVal1; |
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.
Could we just store EHBlkDsc*
directly in gtVal1
and avoid the lookup table/ID?
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.
Or perhaps these pointers won't be stable after #112998?
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.
The EH table can be reallocated when it grows so these addresses aren't durable.
assert(compHndBBtabCount <= compHndBBtabAllocCount); | ||
|
||
unsigned XTnum; | ||
EHblkDsc* HBtab; | ||
|
||
for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++) | ||
{ | ||
// EH IDs should be unique and in range |
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.
Maybe there should be some validation of m_EHIDtoEHblkDsc
, e.g., that the IDs are legal (within range), and the block pointers are within the EH block table?
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.
Sure, though maybe in a follow-up?
I actually envision bigger changes in the EH representation, but have been holding off. For instance keeping this map live throughout compilation and using it for all targets, as it can simplify some of the other EH maintenance.
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.
Sure, though maybe in a follow-up?
I actually envision bigger changes in the EH representation, but have been holding off. For instance keeping this map live throughout compilation and using it for all targets, as it can simplify some of the other EH maintenance.
Sounds good.
There was an x86 test that timed out, passed on retry. No diffs. The x86 TP bump (mostly in coreclr tests) is possibly from be searching the EH table by ID, typically these are small, but coreclr tests probably has some bigger cases. However why this seemingly impacts arm as well is a mystery. |
Give each EH clause a permanent ID unique to that clause. Use this to revise how we model GT_END_LFIN's nesting depth -- the node now refers to a region ID, and we use that plus a lookup table built during
FinalizeEH
to figure out the nesting depth.Contributes to #108900.