Skip to content

gh-127271: Remove the PyCell_Get usage for framelocalsproxy #130383

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
gaogaotiantian merged 8 commits into from
Feb 27, 2025
+59 −20
Merged

gh-127271: Remove the PyCell_Get usage for framelocalsproxy #130383

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2269af5
Remove the PyCell_Get usage for framelocalsproxy
gaogaotiantian Feb 20, 2025
25491da
Fix warning
gaogaotiantian Feb 21, 2025
2a98b81
Fix style
gaogaotiantian Feb 21, 2025
2828ee2
Do check & read in a single operation
gaogaotiantian Feb 25, 2025
c22ba6b
C style
gaogaotiantian Feb 25, 2025
0373601
Fix lint
gaogaotiantian Feb 25, 2025
e396e1b
Remove unused var
gaogaotiantian Feb 25, 2025
72776f0
Use PyDict_GetItemRef
gaogaotiantian Feb 27, 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
37 changes: 26 additions & 11 deletions Objects/frameobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
#define OFF(x) offsetof(PyFrameObject, x)


// Returns borrowed reference or NULL
// Returns new reference or NULL
static PyObject *
framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
{
Expand Down Expand Up @@ -57,7 +57,9 @@ framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
}

if (cell != NULL) {
value = PyCell_GET(cell);
value = PyCell_GetRef((PyCellObject *)cell);
} else {
Comment thread
gaogaotiantian marked this conversation as resolved.
Outdated
Py_XINCREF(value);
}

if (value == NULL) {
Expand All @@ -67,6 +69,17 @@ framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
return value;
}

static bool
framelocalsproxy_hasval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
{
PyObject *value = framelocalsproxy_getval(frame, co, i);
if (value == NULL) {
return false;
}
Py_DECREF(value);
return true;
}

static int
framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read)
{
Expand All @@ -93,7 +106,7 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read)
PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i);
if (name == key) {
if (read) {
if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) {
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {
return i;
}
} else {
Expand Down Expand Up @@ -124,7 +137,7 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read)
}
if (same) {
if (read) {
if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) {
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {
return i;
}
} else {
Expand All @@ -151,7 +164,7 @@ framelocalsproxy_getitem(PyObject *self, PyObject *key)
if (i >= 0) {
PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i);
assert(value != NULL);
Copy link
Copy Markdown
Contributor

@colesbury colesbury Feb 21, 2025

Choose a reason for hiding this comment

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

This doesn't look safe to me. I don't think we can rely on consistency between the earlier framelocalsproxy_getkeyindex call and the framelocalsproxy_getval here.

Copy link
Copy Markdown
Member Author

@gaogaotiantian gaogaotiantian Feb 21, 2025

Choose a reason for hiding this comment

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

Do you mean a very specific case where the content of the cell on that index is set to NULL by PyCell_Set from another thread? I think that's probably an outlier. If it's a normal fast variable, can it happen? Multi-threading can't affect the local fast variables on stack right?

Copy link
Copy Markdown
Contributor

@colesbury colesbury Feb 21, 2025

Choose a reason for hiding this comment

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

Yeah, I'm just thinking about the cell case.

Copy link
Copy Markdown
Member Author

@gaogaotiantian gaogaotiantian Feb 21, 2025

Choose a reason for hiding this comment

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

The normal Python operation won't set NULL to a cell right? So we should maybe raise some exception here if the value is NULL? That's the only case I can think of.

Copy link
Copy Markdown
Contributor

@colesbury colesbury Feb 25, 2025

Choose a reason for hiding this comment

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

I'm not sure about the details here, but in general I think we should avoid this sort of pattern where we perform a "check" separately from a "retrieval" and instead should combine the two. For example, framelocalsproxy_getkeyindex could return the local at the index through an optional output argument.

Copy link
Copy Markdown
Member Author

@gaogaotiantian gaogaotiantian Feb 25, 2025

Choose a reason for hiding this comment

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

Yeah that makes sense. I can pass in an pointer to hold the key. I'll work on it.

Copy link
Copy Markdown
Member Author

@gaogaotiantian gaogaotiantian Feb 25, 2025

Choose a reason for hiding this comment

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

Okay I made framelocalsproxy_getkeyindex do check & retrieve in a single operation. Tweaked a few other places to make it work.

return Py_NewRef(value);
return value;
}

// Okay not in the fast locals, try extra locals
Expand Down Expand Up @@ -297,8 +310,7 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored))
}

for (int i = 0; i < co->co_nlocalsplus; i++) {
PyObject *val = framelocalsproxy_getval(frame->f_frame, co, i);
if (val) {
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {
PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i);
if (PyList_Append(names, name) < 0) {
Py_DECREF(names);
Expand Down Expand Up @@ -511,8 +523,10 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored))
if (value) {
if (PyList_Append(values, value) < 0) {
Py_DECREF(values);
Py_DECREF(value);
return NULL;
}
Py_DECREF(value);
}
}

Expand Down Expand Up @@ -550,16 +564,19 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored))
PyObject *pair = PyTuple_Pack(2, name, value);
if (pair == NULL) {
Py_DECREF(items);
Py_DECREF(value);
return NULL;
}

if (PyList_Append(items, pair) < 0) {
Py_DECREF(items);
Py_DECREF(pair);
Py_DECREF(value);
return NULL;
}

Py_DECREF(pair);
Py_DECREF(value);
}
}

Expand Down Expand Up @@ -601,7 +618,7 @@ framelocalsproxy_length(PyObject *self)
}

for (int i = 0; i < co->co_nlocalsplus; i++) {
if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) {
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {
size++;
}
}
Expand Down Expand Up @@ -2066,9 +2083,7 @@ _PyFrame_HasHiddenLocals(_PyInterpreterFrame *frame)
_PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i);

if (kind & CO_FAST_HIDDEN) {
PyObject* value = framelocalsproxy_getval(frame, co, i);

if (value != NULL) {
if (framelocalsproxy_hasval(frame, co, i)) {
return true;
}
}
Expand Down