Skip to content

GH-119866: Don't clear frame->stackpointer on release builds #131750

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
brandtbucher merged 2 commits into from
Mar 26, 2025
Merged

GH-119866: Don't clear frame->stackpointer on release builds #131750

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b432e3
Don't set the stack pointer to NULL on release builds
brandtbucher Mar 25, 2025
a0c76fd
Update comment
brandtbucher Mar 26, 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
8 changes: 5 additions & 3 deletions Include/internal/pycore_interpframe.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,15 +163,17 @@ _PyFrame_GetLocalsArray(_PyInterpreterFrame *frame)
return frame->localsplus;
}

/* Fetches the stack pointer, and sets stackpointer to NULL.
Having stackpointer == NULL ensures that invalid
values are not visible to the cycle GC. */
// Fetches the stack pointer, and (on debug builds) sets stackpointer to NULL.
// Having stackpointer == NULL makes it easier to catch missing stack pointer
// spills/restores (which could expose invalid values to the GC) using asserts.
static inline _PyStackRef*
_PyFrame_GetStackPointer(_PyInterpreterFrame *frame)
{
assert(frame->stackpointer != NULL);
_PyStackRef *sp = frame->stackpointer;
#ifndef NDEBUG
Copy link
Member

@chris-eibl chris-eibl Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe then update the comment

Having stackpointer == NULL ensures that invalid
   values are not visible to the cycle GC.

Seems that this was done for the cycle GC - if this is no longer needed, just remove the comment?

Out of curiousity: is it safe in release builds to let the cycle GC see invalid values?

Copy link
Member Author

@brandtbucher brandtbucher Mar 26, 2025

Choose a reason for hiding this comment

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

We want to have "deferred" references on the stack, which means that the GC must be able to see all stack values, and only valid stack values.

Setting the stack pointer to NULL is only useful for our asserts, which are sprinkled throughout to make sure that we're spilling and restoring in the correct places. Basically, the GC should never see an invalid stack pointer. On debug builds, we make this easier to detect by setting invalid stack pointers to NULL and checking for this.

Either way, I'll update the comment.

frame->stackpointer = NULL;
#endif
return sp;
}

Expand Down
Loading