-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Cleanup EH / stackwalk interpreter frame handling #124296
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?
Changes from all commits
bbc3d51
431baa4
e2acabf
5f4c02b
a67eba9
7da4632
559cead
664d5d7
8d64ace
2571a17
f31b0ab
428d6e0
2452446
fc85948
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ extern IL_Rethrow_Impl:proc | |
| ifdef FEATURE_INTERPRETER | ||
| extern ExecuteInterpretedMethod:proc | ||
| extern GetInterpThreadContextWithPossiblyMissingThreadOrCallStub:proc | ||
| extern CallInterpreterFuncletWorker:proc | ||
| endif | ||
|
|
||
| extern g_pPollGC:QWORD | ||
|
|
@@ -559,7 +560,7 @@ ifdef FEATURE_INTERPRETER | |
|
|
||
| NESTED_ENTRY InterpreterStub, _TEXT | ||
|
|
||
| PROLOG_WITH_TRANSITION_BLOCK | ||
| PROLOG_WITH_TRANSITION_BLOCK 0, <PushCalleeSavedFloatRegs> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that, instead of using libunwind to obtain the set of callee saved registers at this interpreter entry location, we store them explicitly each time into the transition block. Trying to understand the benefits, do we have a strong motivation to do this ? Given we would make interpreter entry even more expensive.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tried to describe the motivations in the PR description. But let me share more details:
The main motivation was the first one, the second one just moving the needle more in the motivation. A minor extra motivation is that it makes the stack frame iterator code cleaner, but that was more of a nice outcome than anything else.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. I was wondering mainly about the main motivation behind getting rid of libunwind. (And whether we want to do this at the expense of minor potential regressions)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From a maintenance perspective, porting Dependency footprint in terms of $ git grep 'PAL_VirtualUnwind' dotnet/release/8.0 -- src/coreclr | wc -l
41
$ git grep 'PAL_VirtualUnwind' dotnet/release/9.0 -- src/coreclr | wc -l
42
$ git grep 'PAL_VirtualUnwind' dotnet/release/10.0 -- src/coreclr | wc -l
25
$ git grep 'PAL_VirtualUnwind' dotnet/main -- src/coreclr | wc -l
25We may not be able to eliminate all usages, but we can continue reducing the dependency and evaluate alternative strategies for the rest, however feasible. Runtimes that control their code generation (go, java..) typically also control their unwinding strategy and do not rely on general OS unwinding for managed frames. At native–managed boundaries, behavior is usually constrained, and cross-boundary unwinding is either disallowed or treated as a fatal condition.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if unwinding via libunwind is slower, it seems we would want to prioritize normal execution path, rather than the exceptional flow. If we have the option of using it on some platforms, to me it seems like we should keep this option open. Maybe have the possibility of using it to populate the full register context only for a subset of frame types, like the interpreter transitions. Of course, these are only some thoughts from a high level perspective, this might not necessarily be feasible for what it's worth.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The other pattern that can be used to get rid of libunwind dependencies is try/catch the C++ exception in C++ and return non-exceptionally to the caller. #123482 is proposing that we do that for QCalls. You pay the cost of C++ try/catch and error return check vs. the cost of arch-specific asm helpers that must save a bunch of registers. (I understand that this alternative may not be suitable in this case.)
Some perf numbers would be nice. |
||
|
|
||
| __InterpreterStubArgumentRegistersOffset = __PWTB_ArgumentRegisters | ||
| ; IR bytecode address | ||
|
|
@@ -1286,6 +1287,56 @@ END_PROLOGUE | |
| ret | ||
| NESTED_END CallJittedMethodRetU2, _TEXT | ||
|
|
||
| ;========================================================================== | ||
| ; Create a real TransitionBlock and call CallInterpreterFuncletWorker | ||
| ; to execute an interpreter funclet (catch/finally/filter handler). | ||
| ; | ||
| ; extern "C" DWORD_PTR CallInterpreterFunclet( | ||
| ; OBJECTREF throwable, // rcx | ||
| ; void* pHandler, // rdx | ||
| ; REGDISPLAY *pRD, // r8 | ||
| ; ExInfo *pExInfo, // r9 | ||
| ; bool isFilter // [rsp+28h] | ||
| ; ); | ||
| ;========================================================================== | ||
| extern CallInterpreterFuncletWorker:proc | ||
|
|
||
| NESTED_ENTRY CallInterpreterFunclet, _TEXT | ||
|
|
||
| PROLOG_WITH_TRANSITION_BLOCK 16, <PushCalleeSavedFloatRegs> | ||
|
|
||
| ; Pass TransitionBlock* as last (6th) argument on stack | ||
| ; Worker signature: CallInterpreterFuncletWorker(throwable, pHandler, pRD, pExInfo, isFilter, TransitionBlock*) | ||
| ; Original args: rcx=throwable, rdx=pHandler, r8=pRD, r9=pExInfo, [rsp+__PWTB_ArgumentRegisters+20h]=isFilter | ||
|
|
||
| ; Move isFilter to 5th param slot | ||
| mov rax, [rsp + __PWTB_ArgumentRegisters + 20h] ; isFilter (5th param from original caller) | ||
| mov [rsp + 20h], rax ; pass isFilter as 5th param on stack | ||
|
|
||
| ; Put TransitionBlock* as 6th param on stack | ||
| lea rax, [rsp + __PWTB_TransitionBlock] | ||
| mov [rsp + 28h], rax ; TransitionBlock* as 6th param | ||
|
|
||
| ; rcx, rdx, r8, r9 remain unchanged (throwable, pHandler, pRD, pExInfo) | ||
|
|
||
| call CallInterpreterFuncletWorker | ||
|
|
||
| EPILOG_WITH_TRANSITION_BLOCK_RETURN | ||
|
|
||
| NESTED_END CallInterpreterFunclet, _TEXT | ||
|
|
||
| extern AsyncHelpers_ResumeInterpreterContinuationWorker:proc | ||
|
|
||
| NESTED_ENTRY AsyncHelpers_ResumeInterpreterContinuation, _TEXT | ||
| PROLOG_WITH_TRANSITION_BLOCK 0, <PushCalleeSavedFloatRegs> | ||
|
|
||
| lea r8, [rsp + __PWTB_TransitionBlock] | ||
| call AsyncHelpers_ResumeInterpreterContinuationWorker | ||
|
|
||
| EPILOG_WITH_TRANSITION_BLOCK_RETURN | ||
|
|
||
| NESTED_END AsyncHelpers_ResumeInterpreterContinuation, _TEXT | ||
|
|
||
| endif ; FEATURE_INTERPRETER | ||
|
|
||
| ;========================================================================== | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.