Fix GCFrame allocated by interpreted code handling #120384
Conversation
There are few cases where GCFrame is created by managed code as a managed local. When that code is interpreted, this local is on the interpreter stack which is NOT the processor stack. So various debug checks and also removing these GCFrames from the per-thread chain doesn't work. This change adds an "os stack location" member to the GCFrame that is set to "this" for GCFrames allocated in native code. For the ones allocated by the interpreted managed code, it is set to the InterpMethodContextFrame of the frame that created it. That value is then used instead of the raw GCFrame pointer to compare locations of these frames.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes GCFrame handling for interpreted managed code by adding an OS stack location tracking mechanism. The issue occurs when GCFrames are allocated on the interpreter stack rather than the processor stack, causing debug checks and frame removal operations to fail.
Key changes:
- Added
GetOSStackLocation()method to GCFrame that returns the appropriate stack location for comparison operations - Updated GCFrame constructor and registration logic to properly set OS stack location for interpreted code scenarios
- Modified debug assertions and frame popping logic to use OS stack location instead of raw GCFrame pointers
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/threads.h | Moved GetGCFrame() method declaration from inline to separate implementation |
| src/coreclr/vm/threads.cpp | Implemented GetGCFrame() method with updated debug assertion using OS stack location |
| src/coreclr/vm/frames.h | Added GetOSStackLocation() method and m_osStackLocation member for interpreter support |
| src/coreclr/vm/frames.cpp | Updated GCFrame constructor and Push() method to use OS stack location for ordering checks |
| src/coreclr/vm/exceptionhandling.cpp | Modified frame popping logic to use OS stack location for comparison |
| src/coreclr/vm/eetwain.cpp | Added interpreter-specific logic to set OS stack location during GCFrame registration |
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
|
I like the approach, but there are too many build failures for me to sign off here. |
|
OSStack is a little misleading because on WASM it's not an OS stack or a native stack, but instead is just in linear memory managed by us. But I think that's fine, it's right on every other target. |
I've picked the naming that David used for similar thing few days ago. |
I've forgotten to add this file
src/libraries/System.Private.CoreLib/src/System/Runtime/GCFrameRegistration.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/GCFrameRegistration.cs
Show resolved
Hide resolved
There are few cases where GCFrame is created by managed code as a managed local. When that code is interpreted, this local is on the interpreter stack which is NOT the processor stack. So various debug checks and also removing these GCFrames from the per-thread chain doesn't work.
This change adds an "os stack location" member to the GCFrame that is set to "this" for GCFrames allocated in native code. For the ones allocated by the interpreted managed code, it is set to the InterpMethodContextFrame of the frame that created it. That value is then used instead of the raw GCFrame pointer to compare locations of these frames.