Skip to content

[C API] Get "self" args or non-null co_varnames from frame object with C-API #90324

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.

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

Closed
Skylion007 mannequin opened this issue Dec 23, 2021 · 17 comments
Closed

[C API] Get "self" args or non-null co_varnames from frame object with C-API #90324

Skylion007 mannequin opened this issue Dec 23, 2021 · 17 comments
Labels
3.11 only security fixes pending The issue will be closed if no feedback is provided topic-C-API type-feature A feature request or enhancement

Comments

@Skylion007
Copy link
Mannequin

Skylion007 mannequin commented Dec 23, 2021

BPO 46166
Nosy @terryjreedy, @gpshead, @vstinner, @markshannon, @ericsnowcurrently, @henryiii, @pablogsal, @Skylion007
PRs
  • bpo-46166: Fix compiler warnings in What's New in Python 3.11 #31198
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2021-12-23.19:33:30.242>
    labels = ['expert-C-API', 'type-feature', '3.11']
    title = '[C API] Get "self" args or non-null co_varnames from frame object with C-API'
    updated_at = <Date 2022-02-07.16:46:31.649>
    user = 'https://github.com/Skylion007'

    bugs.python.org fields:

    activity = <Date 2022-02-07.16:46:31.649>
    actor = 'vstinner'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['C API']
    creation = <Date 2021-12-23.19:33:30.242>
    creator = 'Skylion007'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46166
    keywords = ['patch', '3.11regression']
    message_count = 15.0
    messages = ['409100', '409162', '411902', '411906', '411908', '412206', '412208', '412210', '412528', '412547', '412761', '412762', '412763', '412764', '412768']
    nosy_count = 8.0
    nosy_names = ['terry.reedy', 'gregory.p.smith', 'vstinner', 'Mark.Shannon', 'eric.snow', 'Henry Schreiner', 'pablogsal', 'Skylion007']
    pr_nums = ['31198']
    priority = 'high'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue46166'
    versions = ['Python 3.11']

    @Skylion007
    Copy link
    Mannequin Author

    Skylion007 mannequin commented Dec 23, 2021

    Hello, I am a maintainer with the PyBind11 project. We have been following the 3.11 development branch and have noticed an issue we are encountering with changes to the C-API.

    Particularly, we have an edge case in our overloading dispatch mechanism that we used to solve by inspecting the "self" argument in the co_varnames member of the python frame object: (https://github.com/pybind/pybind11/blob/a224d0cca5f1752acfcdad8e37369e4cda42259e/include/pybind11/pybind11.h#L2380). However, in the new struct, the co_varnames object can now be null. There also doesn't appear to be any public API to populate it on the C-API side. Accessing it via the "inspect" module still works, but that requires us to run a Python code snippit in a potentially very hot code path: (https://github.com/pybind/pybind11/blob/a224d0cca5f1752acfcdad8e37369e4cda42259e/include/pybind11/pybind11.h#L2408).

    As such, we were hoping that either there is some new API change we have missed, or if there is some way other modern (and hopefully somewhat stable way to access the API) so we can emulate the old behavior with the C-API.

    @Skylion007 Skylion007 mannequin added 3.11 only security fixes topic-C-API type-feature A feature request or enhancement labels Dec 23, 2021
    @terryjreedy
    Copy link
    Member

    Pablo, Mark: I am guessing that at least one of you know about this.

    @henryiii
    Copy link
    Mannequin

    henryiii mannequin commented Jan 27, 2022

    It would be great to get this looked at before things start getting too locked in for 3.11!

    @markshannon
    Copy link
    Member

    Yes, we should expose the tuple of variable names, both in Python and in the C-API.

    Would something like
    PyCodeObject_GetVariableName() and PyCodeObject_GetVariableKind() work?

    In the meantime, since you were reading co_varnames directly, why not read co_localsplusnames directly?

    OOI, how do you cope with non-local self?

    @ericsnowcurrently
    Copy link
    Member

    In addition to what Mark said, note that co_varnames get's populated lazily by the Python-level getter for code.co_varnames. So could you call the Python function before entering the hot path?

    Regardless, a dedicated C-API for this like Mark suggested would be the better solution.

    @Skylion007
    Copy link
    Mannequin Author

    Skylion007 mannequin commented Jan 31, 2022

    We didn't want to read colocalsplus directly because we were worried about the stability of that approach and the code complexity / readability. Also, I wasn't aware that colocalsplus would work or if that was lazily populated as well.

    The functions used in CPython to extract the args from colocalsplus do not seem to be public and would need to be reimplemented by PyBind11, right? That seems very brittle as try to support future Python versions and may break in the future.

    Having a somewhat stable C-API to query this information seems like it would be the best solution, but I am open to suggestions on how to best proceed. How would you all recommend PyBind11 proceed with supporting 3.11 if not a C-API addition? The PyBind11 authors want to resolve this before the API becomes too locked down for 3.11.

    @Skylion007
    Copy link
    Mannequin Author

    Skylion007 mannequin commented Jan 31, 2022

    PyCodeObject_GetVariableName() and PyCodeObject_GetVariableKind() work?

    • Some public-gettters such as these functions would be ideal.

    OOI, how do you cope with non-local self?

    • We only care about checking self to prevent an infinite recursion in our method dispatch code so I am not sure a non-local self would be applicable in this case? Correct me if I am wrong.

    @vstinner
    Copy link
    Member

    It would be nice to have a PyFrame_GetVariable(frame, "self") function: get the value of the "frame" variable of the specified frame object.

    @Skylion007
    Copy link
    Mannequin Author

    Skylion007 mannequin commented Feb 4, 2022

    I saw the latest Python 3.11 5A release notes on the frame API changes. Do the notes mean the only officially supported way of accessing co_varnames is now through the Python interface and the inspect module? By using PyObject_GetAttrString?

    Also, the documentation in the WhatsNew is a bit unclear as PyObject_GetAttrString(frame, "f_locals") doesn't work for PyFrameObject*, only PyObject* and it doesn't describe how to get the PyObject* version of FrameObject. The same problem also happens when trying to access the co_varnames field of the PyCodeObject*.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 4, 2022

    PyObject_GetAttrString(frame, "f_locals") doesn't work for PyFrameObject*

    Oh, would you mind to elaborate?

    Example in Python:

    $ ./python
    Python 3.11.0a5+ 
    >>> import sys
    >>> f=sys._getframe()
    >>> f.f_locals
    {'__name__': '__main__', '__doc__': None, ...}

    @Skylion007
    Copy link
    Mannequin Author

    Skylion007 mannequin commented Feb 7, 2022

    The frame object I am referring to was:

    PyFrameObject *frame = PyThreadState_GetFrame(PyThreadState_Get());

    This frame can not be used with PyObject_GetAttrString. Is there anyway to get the PyObject* associated with a PyFrameObject*? It seems weird that some functionality is just not accessible using the Stable ABI of PyThreadState_GetFrame .

    To elabroate: I was referring to the migration guide in the changelog btw:

    f_code: removed, use PyFrame_GetCode() instead. Warning: the function returns a strong reference, need to call Py_DECREF().
    
    f_back: changed (see below), use PyFrame_GetBack().
    
    f_builtins: removed, use PyObject_GetAttrString(frame, "f_builtins").
    
    // this frame object actually has to be a PyObject*, the old one was a  PyFrameObject* . Dropping this in does not work. 
    f_globals: removed, use PyObject_GetAttrString(frame, "f_globals").
    
    f_locals: removed, use PyObject_GetAttrString(frame, "f_locals").
    
    f_lasti: removed, use PyObject_GetAttrString(frame, "f_lasti").
    

    I tried importing sys._getframe(), but that gave an attribute error interestingly enough. Run a full code snippit here works: https://github.com/pybind/pybind11/blob/96b943be1d39958661047eadac506745ba92b2bc/include/pybind11/pybind11.h#L2429, but is really slow and we would like avoid having to rely on it. Not to mention relying on a function that is an starts with an underscore seems like it really should be avoided.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 7, 2022

    Example of C code that I added to _testcapi:
    ---

    static PyObject *
    get_caller_locals(PyObject *self, PyObject *Py_UNUSED(args))
    {
        PyFrameObject *frame = PyThreadState_GetFrame(PyThreadState_Get());
        if (frame == NULL) {
            Py_RETURN_NONE;
        }
        return PyObject_GetAttrString(frame, "f_locals");
    }

    Python example:
    ---

    import _testcapi
    
    def f():
        x = 1
        y = 2
        print(_testcapi.get_caller_locals())
    
    f()

    Output on Python 3.11:
    ---
    {'x': 1, 'y': 2}
    ---

    => it just works.

    A PyFrameObject is a regular Python object, you can use functions like PyObject_GetAttrString().

    Maybe I missed something, correct me if I'm wrong.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 7, 2022

    Is there anyway to get the PyObject* associated with a PyFrameObject*?

    Ah. I see. If you pass a PyFrameObject* frame to PyObject_GetAttrString(), you get a compiler warning.

    You should cast it explicitly: PyObject_GetAttrString((PyObject*)frame, "f_locals").

    @vstinner
    Copy link
    Member

    vstinner commented Feb 7, 2022

    I proposed #75381 to fix the compiler warnings in the doc.

    @vstinner vstinner changed the title Get "self" args or non-null co_varnames from frame object with C-API [C API] Get "self" args or non-null co_varnames from frame object with C-API Feb 7, 2022
    @vstinner vstinner changed the title Get "self" args or non-null co_varnames from frame object with C-API [C API] Get "self" args or non-null co_varnames from frame object with C-API Feb 7, 2022
    @vstinner
    Copy link
    Member

    vstinner commented Feb 7, 2022

    New changeset a89772c by Victor Stinner in branch 'main':
    bpo-46166: Fix compiler warnings in What's New in Python 3.11 (GH-31198)
    a89772c

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel
    Copy link
    Member

    It seems that with @vstinner 's suggestions the immediate problem was resolved. Is there a longer term concern here? Is this similar to #89410?

    @iritkatriel iritkatriel added the pending The issue will be closed if no feedback is provided label May 13, 2023
    @vstinner
    Copy link
    Member

    vstinner commented Dec 2, 2023

    Python 3.12 added PyFrame_GetVar() and PyFrame_GetVarString(). You can use https://pythoncapi-compat.readthedocs.io/ to get these functions on older Python versions.

    I gave examples on how to get the f_locals attribute of a frame object.

    Python now has a wider C API to access PyFrameObject members: https://docs.python.org/dev/c-api/frame.html

    Finally, https://docs.python.org/dev/whatsnew/3.11.html#whatsnew311-c-api-porting documents how to replace removed PyFrameObject members with function calls.

    I close the issue.

    @vstinner vstinner closed this as completed Dec 2, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes pending The issue will be closed if no feedback is provided topic-C-API type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants