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
merged 8 commits into from
Feb 27, 2025
Merged

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

Changes from all 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
79 changes: 59 additions & 20 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,10 @@ framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
}

if (cell != NULL) {
value = PyCell_GET(cell);
value = PyCell_GetRef((PyCellObject *)cell);
}
else {
Py_XINCREF(value);
}

if (value == NULL) {
Expand All @@ -67,24 +70,42 @@ 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)
framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject *key, bool read, PyObject **value_ptr)
{
/*
* Returns -2 (!) if an error occurred; exception will be set.
* Returns the fast locals index of the key on success:
* - if read == true, returns the index if the value is not NULL
* - if read == false, returns the index if the value is not hidden
* Otherwise returns -1.
*
* If read == true and value_ptr is not NULL, *value_ptr is set to
* the value of the key if it is found (with a new reference).
*/

// value_ptr should only be given if we are reading the value
assert(read || value_ptr == NULL);

PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);

// Ensure that the key is hashable.
Py_hash_t key_hash = PyObject_Hash(key);
if (key_hash == -1) {
return -2;
}

bool found = false;

// We do 2 loops here because it's highly possible the key is interned
Expand All @@ -93,7 +114,14 @@ 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) {
PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i);
if (value != NULL) {
if (value_ptr != NULL) {
*value_ptr = value;
}
else {
Py_DECREF(value);
}
return i;
}
} else {
Expand Down Expand Up @@ -124,7 +152,14 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read)
}
if (same) {
if (read) {
if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) {
PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i);
if (value != NULL) {
if (value_ptr != NULL) {
*value_ptr = value;
}
else {
Py_DECREF(value);
}
return i;
}
} else {
Expand All @@ -142,25 +177,27 @@ static PyObject *
framelocalsproxy_getitem(PyObject *self, PyObject *key)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *value = NULL;

int i = framelocalsproxy_getkeyindex(frame, key, true);
int i = framelocalsproxy_getkeyindex(frame, key, true, &value);
if (i == -2) {
return NULL;
}
if (i >= 0) {
PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i);
assert(value != NULL);
return Py_NewRef(value);
return value;
}
assert(value == NULL);

// Okay not in the fast locals, try extra locals

PyObject *extra = frame->f_extra_locals;
if (extra != NULL) {
PyObject *value = PyDict_GetItem(extra, key);
if (PyDict_GetItemRef(extra, key, &value) < 0) {
return NULL;
}
if (value != NULL) {
return Py_NewRef(value);
return value;
}
}

Expand All @@ -176,7 +213,7 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
_PyStackRef *fast = _PyFrame_GetLocalsArray(frame->f_frame);
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);

int i = framelocalsproxy_getkeyindex(frame, key, false);
int i = framelocalsproxy_getkeyindex(frame, key, false, NULL);
if (i == -2) {
return -1;
}
Expand Down Expand Up @@ -297,8 +334,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 +547,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 +588,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 +642,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 All @@ -613,7 +654,7 @@ framelocalsproxy_contains(PyObject *self, PyObject *key)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;

int i = framelocalsproxy_getkeyindex(frame, key, true);
int i = framelocalsproxy_getkeyindex(frame, key, true, NULL);
if (i == -2) {
return -1;
}
Expand Down Expand Up @@ -724,7 +765,7 @@ framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs)

PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;

int i = framelocalsproxy_getkeyindex(frame, key, false);
int i = framelocalsproxy_getkeyindex(frame, key, false, NULL);
if (i == -2) {
return NULL;
}
Expand Down Expand Up @@ -2066,9 +2107,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
Loading