Fix Thread::IsAddressInStack for interpreter #116818
Conversation
The check is used at several places to verify that e.g. `StringHandleOnStack` is really on stack. It needs to be updated to take the interpreter stack into account tooo.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Thread methods to correctly account for the interpreter stack when checking if an address is within the stack range.
- Removed inline implementations from the header and added function declarations.
- Updated both IsAddressInCurrentStack and IsAddressInStack in the cpp file to handle interpreter-specific checks.
- Changed pointer types in InterpThreadContext (in interpexec.h) from int8_t* to PTR_INT8 to enhance type safety.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/vm/threads.h | Removed inline definitions; now only declares the functions. |
| src/coreclr/vm/threads.cpp | Updated stack range checks to include interpreter stack support. |
| src/coreclr/vm/interpexec.h | Changed pointer types to PTR_INT8 for interpreter contexts. |
src/coreclr/vm/threads.cpp
Outdated
| _ASSERTE(m_CacheStackLimit != NULL); | ||
| _ASSERTE(m_CacheStackLimit < m_CacheStackBase); | ||
| #ifdef FEATURE_INTERPRETER | ||
| if ((m_pInterpThreadContext != NULL) && ((PTR_VOID)m_pInterpThreadContext->pStackStart < addr) && (addr <= (PTR_VOID)m_pInterpThreadContext->pStackPointer)) |
There was a problem hiding this comment.
I think the < and <= need to be reversed since the interp stack grows upwards, unlike the native stack.
There was a problem hiding this comment.
No, the pStackStart is at smaller address than the pStackPointer (the pStackStart is the address at which we've allocated the memory for the stack), so the comparison is correct.
There was a problem hiding this comment.
On second thought, I'm not sure why the comparison is like that for the native stack pointer either. pStackStart is a valid address inside the interpreter stack. Why would it return false for this value ?
There was a problem hiding this comment.
I'm not sure I understand. The point of this method is to check whether the address addr is on the stack or not. So we are comparing it to the lowest and highest address of the valid part of the stack.
This function is used in various asserts.
There was a problem hiding this comment.
Vlad is right, I've misunderstood his point. I'll fix it.
Co-authored-by: Copilot <[email protected]>
The check is used at several places to verify that e.g.
StringHandleOnStackis really on stack. It needs to be updated to take the interpreter stack into account too.