Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Revert "Resume after catch for interpreter via native EH" by jkotas · Pull Request #129362 · dotnet/runtime · GitHub
Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Revert "Resume after catch for interpreter via native EH" by jkotas · Pull Request #129362 · dotnet/runtime · GitHub
Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Revert "Resume after catch for interpreter via native EH" by jkotas · Pull Request #129362 · dotnet/runtime · GitHub
Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Revert "Resume after catch for interpreter via native EH" by jkotas · Pull Request #129362 · dotnet/runtime · GitHub
Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Revert "Resume after catch for interpreter via native EH" by jkotas · Pull Request #129362 · dotnet/runtime · GitHub
Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Revert "Resume after catch for interpreter via native EH" by jkotas · Pull Request #129362 · dotnet/runtime · GitHub
Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Revert "Resume after catch for interpreter via native EH" by jkotas · Pull Request #129362 · dotnet/runtime · GitHub
Skip to content

Revert "Resume after catch for interpreter via native EH" - #129362

Merged
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh
Jun 13, 2026
Merged

Revert "Resume after catch for interpreter via native EH"#129362
jkotas merged 2 commits into
mainfrom
revert-128728-resume-after-catch-via-native-eh

Conversation

@jkotas

@jkotasjkotas commented Jun 13, 2026

Copy link
Copy Markdown
Member

Reverts #128728

Fixes#129270

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Reverting to stabilize CI

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 reverts the prior change set that enabled “resume after catch” for the interpreter via native EH, restoring the previous exception-boundary behavior and adjusting interpreter resume mechanics accordingly.

Changes:

  • Removes INSTALL_/UNINSTALL_RESUME_AFTER_CATCH_* handling from various VM exception-boundary sites (QCall, prestub, jit helpers, P/Invoke import, VSD resolve, GC helpers, etc.).
  • Reworks interpreter resume-after-catch plumbing by tracking an interpreter SP marker and updating resume/unwind helpers (notably in InterpreterCodeManager::ResumeAfterCatch and interpreter execution).
  • Updates UM entry prestub unwind metadata on multiple architectures to use UnhandledExceptionHandlerUnix.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/coreclr/vm/wasm/helpers.cppAdjusts interpreter-related InlinedCallFrame reg display update on WASM.
src/coreclr/vm/virtualcallstub.cppRemoves resume-after-catch handler usage from VSD resolve workers.
src/coreclr/vm/riscv64/asmhelpers.SChanges UM entry prestub handler metadata on RISC-V.
src/coreclr/vm/qcall.hRemoves resume-after-catch handler from QCall begin/end macros.
src/coreclr/vm/prestub.cppRemoves resume-after-catch handler usage and modifies prestub exception catch behavior.
src/coreclr/vm/loongarch64/asmhelpers.SChanges UM entry prestub handler metadata on LoongArch64.
src/coreclr/vm/jithelpers.cppRemoves resume-after-catch handler usage from IL throw/rethrow helpers.
src/coreclr/vm/interpexec.hClarifies interpreter calling/stack-pointer rationale in comments.
src/coreclr/vm/interpexec.cppAdds interpreter SP tracking macro and updates ResumeAfterCatchException flow.
src/coreclr/vm/i386/umthunkstub.SChanges UM entry prestub handler metadata on x86 Unix.
src/coreclr/vm/gchelpers.cppRemoves resume-after-catch handler usage from allocation failure helper.
src/coreclr/vm/frames.hAdds non-WASM interpreter SP storage to InterpreterFrame.
src/coreclr/vm/frames.cppUses interpreter SP marker for FP register unwind; adjusts interpreter-frame detection.
src/coreclr/vm/exceptmacros.hRemoves resume-after-catch handler macros; introduces new helper declaration.
src/coreclr/vm/exceptionhandling.hRemoves skip-managed-frames resume helper declaration.
src/coreclr/vm/exceptionhandling.cppRemoves resume-after-catch handling wrapper usage and related helpers.
src/coreclr/vm/excep.cppAdds UnwindAndContinueResumeAfterCatch implementation and related support.
src/coreclr/vm/eetwain.cppReworks interpreter resume-after-catch to unwind to interpreter SP and resume below context.
src/coreclr/vm/dllimport.cppRemoves resume-after-catch handler from PInvokeImportWorker.
src/coreclr/vm/arm64/stubs.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.
src/coreclr/vm/arm64/asmhelpers.SChanges UM entry prestub handler metadata on ARM64.
src/coreclr/vm/amd64/umthunkstub.SChanges UM entry prestub handler metadata on AMD64 Unix.
src/coreclr/vm/amd64/cgenamd64.cppAdjusts interpreter-frame detection in InlinedCallFrame reg display update.

Comment threadsrc/coreclr/vm/wasm/helpers.cpp
Comment threadsrc/coreclr/vm/arm64/stubs.cpp
Comment threadsrc/coreclr/vm/amd64/cgenamd64.cpp
Comment threadsrc/coreclr/vm/frames.cpp
Comment threadsrc/coreclr/vm/prestub.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp
@github-actions

This comment has been minimized.

@BrzVlad

Copy link
Copy Markdown
Member

This would need to revert 380b84f as well, which I'm assuming is not related to the crash in question.

@jkotas

Copy link
Copy Markdown
MemberAuthor

This would need to revert 380b84f as well

Amended the PR

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #129362

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Justified. PR #128728 ("Resume after catch for interpreter via native EH") introduced a regression causing System.AccessViolationException crashes in System.Runtime.Tests.ControlledExecutionTests, with 15 CI hits in 7 days (tracked in #129270). Reverting to stabilize CI is the correct response.

Approach: This is a revert of PR #128728 plus its follow-up fix commit 380b84f ("Add handling of RethrowAfterCatch exception in a few places"). The revert restores the original manual stack-unwinding approach for resume-after-catch in the interpreter, removing the macro-based C++ exception propagation mechanism that was causing the AV.

Summary: ✅ LGTM. Clean revert of a regression-causing change, restoring the previous known-working interpreter resume-after-catch implementation. The code being restored was the working baseline before #128728. No new untested code paths are introduced.


Detailed Findings

✅ Revert Completeness — Both original PR and follow-up are covered

The PR correctly reverts both:

  1. PR Resume after catch for interpreter via native EH #128728 — the core change (macro-based INSTALL/UNINSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME/CONTEXT system)
  2. Commit 380b84f — the follow-up fix that added ResumeAfterCatchException handling at managed-to-native boundaries

This was flagged by @BrzVlad and addressed by @jkotas. All 25 changed files are consistent with a full revert.

✅ Null Check Removal (m_Next != NULL) — Safe

The existing automated review comments flagging the removal of m_Next != NULL guards are false positives. FRAME_TOP is defined as PTR_Frame(~0) (i.e., 0xFFFF...FFFF), not NULL. In the frame chain, m_Next is either a valid frame pointer or FRAME_TOP. The m_Next != NULL checks were defensively added by #128728 and did not exist in the pre-#128728 working code. This is confirmed by the fact that these code paths functioned correctly for the entire lifetime of the interpreter feature before #128728 was merged.

✅ Assert Change in prestub.cpp / threads.cpp — Correct

The replacement of the null-guarded if (ohThrowable != NULL) with _ASSERTE(ohThrowable) + unconditional use is correct. The comment "ohThrowable can be NULL when we've caught the ResumeAfterCatchException" only applied when the INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_FRAME macro was active (which caught ResumeAfterCatchException before the EX_CATCH block). With the macro removed, ResumeAfterCatchException (a C++ exception) will not reach the EX_CATCH block for managed exceptions, so ohThrowable will always be non-null here.

✅ Assert Removal in ExecuteFunctionBelowContext — Expected

The removed _ASSERTE(*(ULONG64*)targetSSP == pContext->Rip) was specific to the SSP computation path used by #128728. The restored code computes targetSSP via GetSSPForFrameOnCurrentStack and GetInterpExecMethodSSP(), which have different invariants. The removal is consistent with the restored implementation.

UnhandledExceptionHandlerUnix Restoration for TheUMEntryPrestub

Restoring the exception handler personality on all Unix architectures (amd64, arm64, i386, loongarch64, riscv64) is correct — the original implementation needed this handler because ResumeAfterCatchException propagates through these stubs via C++ exception handling.

💡 Minor: Preprocessor Indentation in prestub.cpp

The #if/#elif/#endif directives were re-indented (4-space indent) to match their surrounding context. This is cosmetic and matches the pre-#128728 state. No action needed.

Generated by Code Review for issue #129362 · ● 8.6M ·

@jkotas

Copy link
Copy Markdown
MemberAuthor

/ba-g The job running on agent NetCore-Public 124 ran longer than the maximum time of 180 minutes.

@jkotas
jkotas merged commit 7304c88 into mainJun 13, 2026
117 of 130 checks passed
@jkotas
jkotas deleted the revert-128728-resume-after-catch-via-native-eh branch June 13, 2026 13:50
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Runtime.Tests.ControlledExecutionTests failing with fatal error

3 participants

@jkotas@BrzVlad