Skip to content

Set of interpreter related fixes for coreclr tests #118617

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

Merged
merged 4 commits into from
Aug 21, 2025

Conversation

janvorli
Copy link
Member

The changes in this PR were made to fix various issues uncovered by the coreclr tests.

  • Handle case when a try block is eliminated because it can never be executed.
  • Ensure call stubs are generated for IL stubs.
  • Fix shadow stack updating during stack walk
  • Fix handling managed exceptions thrown as SEH from managed code called by JIT / Interpreter compiler in interpreted code.
  • Fix resuming after catch in interpreted code for cases when interpreted code calls another interpreted code via a prestub, which currently happens for delegates.
  • Ensure that ip is saved to the pFrame->ip before calling native methods that can throw to make sure that the stack walk reports correct return IP.

The changes in this PR were made to fix various issues uncovered by the
coreclr tests.
* Handle case when a try block is eliminated because it can never be
  executed.
* Ensure call stubs are generated for IL stubs.
* Fix shadow stack updating during stack walk
* Fix handling managed exceptions thrown as SEH from managed code called
  by JIT / Interpreter compiler in interpreted code.
* Fix resuming after catch in interpreted code for cases when
  interpreted code calls another interpreted code via a prestub, which
  currently happens for delegates.
* Ensure that ip is saved to the pFrame->ip before calling native
  methods that can throw to make sure that the stack walk reports
  correct return IP.
@janvorli janvorli added this to the 11.0.0 milestone Aug 11, 2025
@janvorli janvorli self-assigned this Aug 11, 2025
@Copilot Copilot AI review requested due to automatic review settings August 11, 2025 22:50
@janvorli janvorli requested review from BrzVlad and kg as code owners August 11, 2025 22:50
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a 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 fixes various issues discovered by the coreclr tests related to interpreter functionality, focusing on exception handling, stack management, and IL stub generation. The changes primarily address edge cases in exception propagation and ensure proper state management during method calls.

Key changes:

  • Adds proper handling for managed exceptions thrown from PrepareInitialCode calls
  • Fixes shadow stack pointer (SSP) updating during stack walks in interpreter frames
  • Ensures call stubs are generated for IL stubs when using the interpreter

Reviewed Changes

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

File Description
src/coreclr/vm/interpexec.cpp Adds exception handling wrapper, fixes IP saving timing, and improves catch resumption logic
src/coreclr/vm/frames.cpp Updates shadow stack pointer during register display updates
src/coreclr/vm/dllimport.cpp Ensures call stub creation for interpreted IL stubs
src/coreclr/interpreter/compiler.cpp Handles unreachable try blocks and validates leave instruction targets

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm convinced we have bugs around GC reporting here due to moving the ip early, but that is going to require a trickier fix than I had originally thought. My current thought is that for GC reporting purposes we should do a tweak where we report the conservative merge between the stack before/after the current IP when at the top of a series of interpreter stack frames, but if the InterpMethodContextFrame currently being reported isn't the active frame on the InterpreterFrame we can report only the current data.

@davidwrighton davidwrighton merged commit 4b2a7d5 into dotnet:main Aug 21, 2025
90 of 93 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants