Skip to content

Conversation

janvorli
Copy link
Member

This change enables using InlinedCallFrame around pinvokes called from the interpreter. It ensures that the InlinedCallFrame caller can be set to the interpreted code and that stack walk from the InlinedCallFrame through the interpreted code and then through the InterpreterFrame works properly.

As an example and to test it, I have added an InlinedCallFrame to the temporary implementation of the INTOP_GC_COLLECT and changed it to call a QCALL.

This change enables using InlinedCallFrame around pinvokes called from
the interpreter. It ensures that the InlinedCallFrame caller can be set
to the interpreted code and that stack walk from the InlinedCallFrame
through the interpreted code and then through the InterpreterFrame works
properly.

As an example and to test it, I have added an InlinedCallFrame to the temporary
implementation of the INTOP_GC_COLLECT and changed it to call a QCALL.
@Copilot Copilot AI review requested due to automatic review settings June 26, 2025 11:51
@janvorli janvorli requested review from BrzVlad and kg as code owners June 26, 2025 11:51
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 enables support for InlinedCallFrame when handling pinvokes from the interpreter, ensuring proper stack walk behavior from InlinedCallFrame through the interpreted code. Key changes include the removal of the m_walkingInterpreterFrames member from the stackwalk header and its usage in stackwalk.cpp, the introduction of a conditional check using InterpreterFrame::DummyCallerIP instead, and the insertion of an InlinedCallFrame usage in the interpreter execution to wrap a QCALL.

Reviewed Changes

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

File Description
src/coreclr/vm/stackwalk.h Removed unused member m_walkingInterpreterFrames in favor of a runtime IP check.
src/coreclr/vm/stackwalk.cpp Replaced m_walkingInterpreterFrames checks with a direct IP comparison against DummyCallerIP.
src/coreclr/vm/interpexec.cpp Added an InlinedCallFrame instance to wrap a GC collection, replacing the previous GCX_COOP block.
src/coreclr/vm/frames.h Introduced a constructor for InlinedCallFrame with proper member initialization under FEATURE_INTERPRETER.
Comments suppressed due to low confidence (3)

src/coreclr/vm/stackwalk.h:771

  • The member variable m_walkingInterpreterFrames was removed in this diff. Please ensure that all previous usages relying on this flag are correctly replaced by the new IP-based check, and that the change does not affect readability or future maintenance.
#ifdef FEATURE_INTERPRETER

src/coreclr/vm/interpexec.cpp:1723

  • [nitpick] The new InlinedCallFrame usage replaces the GCX_COOP block for managing GC interactions. Please confirm that the push/pop mechanism adequately preserves thread state transitions and that there are no unintended side effects compared to the original approach.
                        InlinedCallFrame inlinedCallFrame;

src/coreclr/vm/stackwalk.cpp:2848

  • [nitpick] Replacing the explicit m_walkingInterpreterFrames check with a comparison using InterpreterFrame::DummyCallerIP is a significant logic change. Please verify that this condition reliably distinguishes frames as intended, without missing edge cases that were previously covered by the flag.
                if (GetIP(pRD->pCurrentContext) != (TADDR)InterpreterFrame::DummyCallerIP)

Copy link
Contributor

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

@janvorli
Copy link
Member Author

Unfortunately I have tested it with a different build of runtime by accident, it seems it doesn't work as expected. I need to investigate it.

Sign up for free to 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.

1 participant