Skip to content

Fix Thread::IsAddressInStack for interpreter #116818

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.

Sign up for GitHub

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 3 commits into from
Jun 20, 2025
Merged

Fix Thread::IsAddressInStack for interpreter #116818

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e9805c8
Fix Thread::IsAddressInStack for interpreter
janvorli Jun 19, 2025
46e5aa6
Update src/coreclr/vm/threads.cpp
janvorli Jun 19, 2025
e2ae2ea
Fix interpreter stack comparison
janvorli Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/coreclr/vm/interpexec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ struct InterpMethodContextFrame

struct InterpThreadContext
{
int8_t *pStackStart;
int8_t *pStackEnd;
PTR_INT8 pStackStart;
PTR_INT8 pStackEnd;

// This stack pointer is the highest stack memory that can be used by the current frame. This does not
// change throughout the execution of a frame and it is essentially the upper limit of the execution
// stack pointer. It is needed when re-entering interp, to know from which address we can start using
// stack, and also needed for the GC to be able to scan the stack.
int8_t *pStackPointer;
PTR_INT8 pStackPointer;

FrameDataAllocator frameDataAllocator;

Expand Down
39 changes: 39 additions & 0 deletions src/coreclr/vm/threads.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7784,8 +7784,47 @@ InterpThreadContext* Thread::GetInterpThreadContext()
}
#endif // FEATURE_INTERPRETER

/* static */
BOOL Thread::IsAddressInCurrentStack(PTR_VOID addr)
{
LIMITED_METHOD_DAC_CONTRACT;
Thread* currentThread = GetThreadNULLOk();
if (currentThread == NULL)
{
return FALSE;
}

#ifdef FEATURE_INTERPRETER
InterpThreadContext* pInterpThreadContext = currentThread->m_pInterpThreadContext;
if ((pInterpThreadContext != NULL) && ((PTR_VOID)pInterpThreadContext->pStackStart < addr) && (addr <= (PTR_VOID)pInterpThreadContext->pStackPointer))
{
return true;
}
#endif // FEATURE_INTERPRETER

PTR_VOID sp = dac_cast<PTR_VOID>(GetCurrentSP());
_ASSERTE(currentThread->m_CacheStackBase != NULL);
_ASSERTE(sp < currentThread->m_CacheStackBase);
return sp < addr && addr <= currentThread->m_CacheStackBase;
}

#endif // #ifndef DACCESS_COMPILE

BOOL Thread::IsAddressInStack (PTR_VOID addr) const
{
LIMITED_METHOD_DAC_CONTRACT;
_ASSERTE(m_CacheStackBase != NULL);
_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))
Copy link
Member

Choose a reason for hiding this comment

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

I think the < and <= need to be reversed since the interp stack grows upwards, unlike the native stack.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@BrzVlad BrzVlad Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Vlad is right, I've misunderstood his point. I'll fix it.

{
return TRUE;
}
#endif // FEATURE_INTERPRETER
return m_CacheStackLimit < addr && addr <= m_CacheStackBase;
}

#ifdef DACCESS_COMPILE

void
Expand Down
25 changes: 2 additions & 23 deletions src/coreclr/vm/threads.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2485,29 +2485,8 @@ class Thread
public:
static BOOL UniqueStack(void* startLoc = 0);

BOOL IsAddressInStack (PTR_VOID addr) const
{
LIMITED_METHOD_DAC_CONTRACT;
_ASSERTE(m_CacheStackBase != NULL);
_ASSERTE(m_CacheStackLimit != NULL);
_ASSERTE(m_CacheStackLimit < m_CacheStackBase);
return m_CacheStackLimit < addr && addr <= m_CacheStackBase;
}

static BOOL IsAddressInCurrentStack (PTR_VOID addr)
{
LIMITED_METHOD_DAC_CONTRACT;
Thread* currentThread = GetThreadNULLOk();
if (currentThread == NULL)
{
return FALSE;
}

PTR_VOID sp = dac_cast<PTR_VOID>(GetCurrentSP());
_ASSERTE(currentThread->m_CacheStackBase != NULL);
_ASSERTE(sp < currentThread->m_CacheStackBase);
return sp < addr && addr <= currentThread->m_CacheStackBase;
}
BOOL IsAddressInStack (PTR_VOID addr) const;
static BOOL IsAddressInCurrentStack (PTR_VOID addr);

// DetermineIfGuardPagePresent returns TRUE if the thread's stack contains a proper guard page. This function
// makes a physical check of the stack, rather than relying on whether or not the CLR is currently processing a
Expand Down
Loading