Skip to content
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

[C API] Prepare the C API to make PyThreadState opaque: add getter functions #84128

Closed
vstinner opened this issue Mar 12, 2020 · 26 comments
Closed
Labels
3.11 bug and security fixes topic-C-API

Comments

@vstinner
Copy link
Member

BPO 39947
Nosy @vstinner, @fabioz, @markshannon, @ericsnowcurrently, @iritkatriel
PRs
  • bpo-39947: Hide implementation detail of trashcan macros #18971
  • bpo-39947: Move Py_EnterRecursiveCall() to internal C API #18972
  • bpo-39947: Move get_recursion_depth() to _testinternalcapi #18974
  • bpo-39947: Use _PyInterpreterState_GET_UNSAFE() #18978
  • bpo-39947: Add PyInterpreterState_Get() function #18979
  • bpo-39947: Add PyThreadState_GetInterpreter() #18981
  • bpo-39947: Add PyThreadState_GetFrame() function #19092
  • bpo-39947: Use PyThreadState_GetFrame() #19159
  • bpo-39947: Add _PyThreadState_GetDict() #19160
  • bpo-39947: Add PyThreadState_GetID() function #19163
  • Add symbols of the stable ABI to python3dll.c #23598
  • [3.9] bpo-42415: Add symbols of the stable ABI to python3dll.c (GH-23598) #23801
  • bpo-39947: revert incorrect change to a comment #26788
  • bpo-39947: Remove old private trashcan C API functions #26869
  • bpo-39947: Add PyThreadState_SetTrace() function #29121
  • 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 2020-03-12.17:46:54.454>
    labels = ['expert-C-API', '3.11']
    title = '[C API] Make the PyThreadState structure opaque (move it to the internal C API)'
    updated_at = <Date 2022-04-06.13:20:11.746>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2022-04-06.13:20:11.746>
    actor = 'vstinner'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['C API']
    creation = <Date 2020-03-12.17:46:54.454>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39947
    keywords = ['patch']
    message_count = 25.0
    messages = ['364034', '364078', '364080', '364083', '364084', '364103', '364108', '364111', '364113', '364124', '364591', '364674', '365011', '365021', '365022', '370589', '372319', '382551', '383169', '383210', '390054', '396421', '402368', '403816', '416868']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'fabioz', 'Mark.Shannon', 'eric.snow', 'iritkatriel']
    pr_nums = ['18971', '18972', '18974', '18978', '18979', '18981', '19092', '19159', '19160', '19163', '23598', '23801', '26788', '26869', '29121']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue39947'
    versions = ['Python 3.11']

    @vstinner
    Copy link
    Member Author

    Python 3.8 moved PyInterpreterState to the internal C API (commit be3b295 of bpo-35886)... which caused bpo-38500 issue.

    In Python 3.9, I provided Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() as regular functions for the limited API: commit f4b1e3d of bpo-38644. Previously, there were defined as macros, but these macros didn’t compile with the limited C API which cannot access PyThreadState.recursion_depth field (the structure is opaque in the limited C API).

    That's an enhancement for the limited C API, but PyThreadState is still exposed to the "cpython" C API (Include/cpython/).

    We should prepare the C API to make the PyThreadState structure opaque. It cannot be done at once, there are different consumers of the PyThreadState structure. In CPython code base, I found:

    • Py_TRASHCAN_BEGIN_CONDITION and Py_TRASHCAN_END macros access tstate->trash_delete_nesting field. Maybe we can hide these implementation details into new private function calls.

    • faulthandler.c: faulthandler_py_enable() reads tstate->interp. We should maybe provide a getter function.

    • _tracemalloc.c: traceback_get_frames() reads tstate->frame. We should maybe provide a getter function.

    • Private _Py_EnterRecursiveCall() and _Py_LeaveRecursiveCall() access tstate->recursion_depth. One solution is to move these functions to the internal C API.

    faulthandler and _tracemalloc are low-level debugging C extensions. Maybe it's ok for them to use the internal C API. But they are examples of C extensions accessing directly the PyThreadState structure.

    See also bpo-39946 "Is it time to remove _PyThreadState_GetFrame() hook?" about PyThreadState.frame.

    @vstinner vstinner added 3.9 only security fixes topic-C-API labels Mar 12, 2020
    @vstinner vstinner changed the title Move PyThreadState structure to the internal C API Make the PyThreadState structure opaque (move it to the internal C API) Mar 12, 2020
    @vstinner vstinner changed the title Move PyThreadState structure to the internal C API Make the PyThreadState structure opaque (move it to the internal C API) Mar 12, 2020
    @vstinner
    Copy link
    Member Author

    New changeset 224481a by Victor Stinner in branch 'master':
    bpo-39947: Move Py_EnterRecursiveCall() to internal C API (GH-18972)
    224481a

    @fabioz
    Copy link
    Mannequin

    fabioz mannequin commented Mar 13, 2020

    As a note, externally I have to use it in pydevd to set the tracing for different threads -- i.e.: https://bugs.python.org/issue35370

    Will that still be possible?

    @vstinner
    Copy link
    Member Author

    As a note, externally I have to use it in pydevd to set the tracing for different threads -- i.e.: https://bugs.python.org/issue35370 Will that still be possible?

    My intent is not to prevent third-party C extension modules to modify PyThreadState, but to make the structure opaque. I mean that we should add getter and setter function for the most commonly used PyThreadState fields.

    @vstinner
    Copy link
    Member Author

    New changeset 3f2f4fe by Victor Stinner in branch 'master':
    bpo-39947: Move get_recursion_depth() to _testinternalcapi (GH-18974)
    3f2f4fe

    @vstinner
    Copy link
    Member Author

    New changeset 38965ec by Victor Stinner in branch 'master':
    bpo-39947: Hide implementation detail of trashcan macros (GH-18971)
    38965ec

    @vstinner
    Copy link
    Member Author

    I tested to build numpy with an opaque PyThreadState. First issue, Plex gets the current interpreter using PyThreadState.interp:

    /tmp/pip-install-aq60p8w2/Cython/Cython/Plex/Scanners.c:7447:73: erreur: déréférencement d'un pointeur du type incomplet « PyThreadState » {alias « struct _ts »}
     7447 |     PY_INT64_T current_id = PyInterpreterState_GetID(PyThreadState_Get()->interp);
    

    We should add a PyThreadState_GetInterpreter(tstate) getter. faulthandler_py_enable() would use it for example.

    Maybe _PyInterpreterState_Get() can be used, but it's a private function. There are also _PyThreadState_UncheckedGet() and _PyGILState_GetInterpreterStateUnsafe() which are worse: don't check for NULL pointers.

    @vstinner
    Copy link
    Member Author

    New changeset ff4584c by Victor Stinner in branch 'master':
    bpo-39947: Use _PyInterpreterState_GET_UNSAFE() (GH-18978)
    ff4584c

    @vstinner
    Copy link
    Member Author

    New changeset be79373 by Victor Stinner in branch 'master':
    bpo-39947: Add PyInterpreterState_Get() function (GH-18979)
    be79373

    @vstinner
    Copy link
    Member Author

    New changeset 8fb02b6 by Victor Stinner in branch 'master':
    bpo-39947: Add PyThreadState_GetInterpreter() (GH-18981)
    8fb02b6

    @vstinner
    Copy link
    Member Author

    I added PyInterpreterState_Get() and PyThreadState_GetInterpreter() functions to get the interpreter.

    @vstinner
    Copy link
    Member Author

    New changeset fd1e1a1 by Victor Stinner in branch 'master':
    bpo-39947: Add PyThreadState_GetFrame() function (GH-19092)
    fd1e1a1

    @vstinner
    Copy link
    Member Author

    New changeset 3072338 by Victor Stinner in branch 'master':
    bpo-39947: Use PyThreadState_GetFrame() (GH-19159)
    3072338

    @vstinner
    Copy link
    Member Author

    New changeset 0e427c6 by Victor Stinner in branch 'master':
    bpo-39947: Add _PyThreadState_GetDict() function (GH-19160)
    0e427c6

    @vstinner
    Copy link
    Member Author

    New changeset 5c3cda0 by Victor Stinner in branch 'master':
    bpo-39947: Add PyThreadState_GetID() function (GH-19163)
    5c3cda0

    @vstinner vstinner changed the title Make the PyThreadState structure opaque (move it to the internal C API) [C API] Make the PyThreadState structure opaque (move it to the internal C API) Apr 14, 2020
    @vstinner vstinner changed the title Make the PyThreadState structure opaque (move it to the internal C API) [C API] Make the PyThreadState structure opaque (move it to the internal C API) Apr 14, 2020
    @vstinner
    Copy link
    Member Author

    vstinner commented Jun 2, 2020

    Cython still access multiple PyThreadState members which have no getter or setter yet.

    __Pyx_PyErr_ExceptionMatchesInState():

        PyObject *exc_type = tstate->curexc_type;
        ...

    => internal _PyErr_Occurred(tstate) could solve this issue: move it the public/private API? Or expose internal _PyErr_ExceptionMatches(tstate, exc)?

    __Pyx_ErrRestoreInState() is a reimplementation of internal _PyErr_Restore(): get/set curexc_type, curexc_value and curexc_traceback members.

    __Pyx_PyFunction_FastCallNoKw:

    static PyObject* __Pyx_PyFunction_FastCallNoKw(...) {
        ...
        ++tstate->recursion_depth;
        Py_DECREF(f);
        --tstate->recursion_depth;
        return result;
    }

    Why not calling Py_EnterRecursiveCall/Py_LeaveRecursiveCall?

    There are likely others.

    @vstinner
    Copy link
    Member Author

    I marked bpo-35949 "Move PyThreadState into Include/internal/pycore_pystate.h" as a duplicate of this issue.

    bpo-35949 lists Py_ALLOW_RECURSION and Py_END_ALLOW_RECURSION which access PyThreadState.recursion_critical member directly.

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 5, 2020

    bpo-35949 lists Py_ALLOW_RECURSION and Py_END_ALLOW_RECURSION which access PyThreadState.recursion_critical member directly.

    Oh, problem solved by:

    commit dcc5421
    Author: Serhiy Storchaka <storchaka@gmail.com>
    Date: Mon Oct 5 12:32:00 2020 +0300

    bpo-41936. Remove macros Py_ALLOW_RECURSION/Py_END_ALLOW_RECURSION (GH-22552)
    

    @vstinner
    Copy link
    Member Author

    New changeset fcc6935 by Victor Stinner in branch 'master':
    Add symbols of the stable ABI to python3dll.c (GH-23598)
    fcc6935

    @vstinner
    Copy link
    Member Author

    New changeset 1662868 by Victor Stinner in branch '3.9':
    Add symbols of the stable ABI to python3dll.c (GH-23598) (GH-23801)
    1662868

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 2, 2021

    About setting frame local variables, see:

    @vstinner
    Copy link
    Member Author

    New changeset db532a0 by Victor Stinner in branch 'main':
    bpo-39947: Remove old private trashcan C API functions (GH-26869)
    db532a0

    @vstinner
    Copy link
    Member Author

    See bpo-43760 and PR 28498 for a discussion about the PyThreadState.use_tracing member.

    @vstinner
    Copy link
    Member Author

    See also bpo-45247: [C API] Add explicit support for Cython to the C API.

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 6, 2022

    I would be nice to make the PyThreadState opaque in Python 3.12. IMO it's too late for Python 3.11. Hopefully, Cython should be prepared for such change. At the beginning, maybe Cython can just use the internal C API, as it does to access the internal PyFrameObject structure.

    @vstinner vstinner added 3.11 bug and security fixes and removed 3.9 only security fixes labels Apr 6, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @vstinner
    Copy link
    Member Author

    Many PyThreadState structure changes were discussed recently in the issue #87926.

    I created PR #29121 to add PyThreadState_SetProfile() and PyThreadState_SetTrace() functions, but I wasn't sure my implementation would fit the use cases of debuggers and profilers which need these. I abandoned this PR. @pablogsal added PyEval_SetProfileAllThreads() and PyEval_SetTraceAllThreads() functions to Python 3.12 (commit e34c82a) which should fit the use case, with a different design.

    @vstinner vstinner changed the title [C API] Make the PyThreadState structure opaque (move it to the internal C API) [C API] Prepare the C API to make PyThreadState opaque: add getter functions Jun 21, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 bug and security fixes topic-C-API
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant