gh-127271: Remove the PyCell_Get usage for framelocalsproxy #130383
Conversation
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.
Co-authored-by: Sam Gross <[email protected]>
|
@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?
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.