gh-127271: Remove the PyCell_Get usage for framelocalsproxy#130383
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Objects/frameobject.c Outdated
| PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); | ||
| assert(value != NULL); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, I'm just thinking about the cell case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah that makes sense. I can pass in an pointer to hold the key. I'll work on it.
There was a problem hiding this comment.
Okay I made framelocalsproxy_getkeyindex do check & retrieve in a single operation. Tweaked a few other places to make it work.
gaogaotiantian commented Feb 27, 2025
@colesbury do you have any other suggestions for the current implementation? |
colesbury left a comment
colesbury left a comment There was a problem hiding this comment.
One suggestion below, but otherwise the changes to use new references instead of borrowed references LGTM.
Objects/frameobject.c Outdated
| PyObject *extra = frame->f_extra_locals; | ||
| if (extra != NULL){ | ||
| PyObject *value = PyDict_GetItem(extra, key); | ||
| value = PyDict_GetItem(extra, key); |
There was a problem hiding this comment.
Use PyDict_GetItemRef if possible?
gaogaotiantian merged commit 6140b08 into python:mainUh oh!
There was an error while loading. Please reload this page.
3 participants
In #127272 a
PyCell_Getusage inframeobject.cwas missed from being replaced for free-threaded safety. I guess the reason was probably the function was labelled to return a borrowed reference and a deeper understanding of the piece of code is needed to make that change. (or maybe I'm just thinking too much and it just slipped away :))I made the change to use
PyCell_GetRefand changed the function to return a new reference (because otherwise it won't be safe in free-threaded build). However, there are a few places in the code where it does not care about the actual value stored, it only cares whether such value exists - so I added a util function to avoid havingPy_XDECREFs all around the code.