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

[subinterpreters] Global C variables are a problem #81057

Open
ericsnowcurrently opened this issue May 10, 2019 · 22 comments
Open

[subinterpreters] Global C variables are a problem #81057

ericsnowcurrently opened this issue May 10, 2019 · 22 comments
Assignees
Labels
3.8 expert-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented May 10, 2019

BPO 36876
Nosy @vsajip, @vstinner, @tiran, @phsilva, @ericsnowcurrently, @soltysh, @pablogsal, @sweeneyde
PRs
  • bpo-36876: Avoid static locals. #13372
  • bpo-36876: Use a consistent variable name for kwlist. #13531
  • bpo-36876: Make some static string literal arrays constant. #15760
  • bpo-36876: Add a tool that identifies unsupported global C variables. #15877
  • bpo-36876: Skip test_check_c_globals for now. #16017
  • bpo-36876: Fix the globals checker tool. #16058
  • bpo-36876: Moved Parser/listnode.c statics to interpreter state. #16328
  • bpo-36876: Re-organize the c-analyzer tool code. #16841
  • bpo-36876: Fix the C analyzer tool. #22841
  • bpo-36876: Small adjustments to the C-analyzer tool. #23045
  • bpo-36876: [c-analyzer tool] Tighten up the results and output. #23431
  • bpo-36876: [c-analyzer tool] Add a "capi" subcommand to the c-analyzer tool. #23918
  • bpo-36876: [c-analyzer tool] Additional CLI updates for "capi" command. #23929
  • bpo-36876: Update the c-analyzer whitelist. #31225
  • bpo-36876: Minor cleanup to c-analyzer "ignored" data.' #31239
  • bpo-36876: Make sure the c-analyzer is checking all the source files.' #31264
  • 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 = 'https://github.com/ericsnowcurrently'
    closed_at = None
    created_at = <Date 2019-05-10.19:01:42.087>
    labels = ['expert-subinterpreters', 'type-bug', '3.8']
    title = '[subinterpreters] Global C variables are a problem'
    updated_at = <Date 2022-02-10.23:14:23.025>
    user = 'https://github.com/ericsnowcurrently'

    bugs.python.org fields:

    activity = <Date 2022-02-10.23:14:23.025>
    actor = 'eric.snow'
    assignee = 'eric.snow'
    closed = False
    closed_date = None
    closer = None
    components = ['Subinterpreters']
    creation = <Date 2019-05-10.19:01:42.087>
    creator = 'eric.snow'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36876
    keywords = ['patch']
    message_count = 22.0
    messages = ['342119', '342125', '352013', '352032', '352063', '352085', '352208', '354928', '356181', '357351', '357352', '357353', '368910', '379390', '379999', '381511', '383698', '383778', '393785', '412886', '412957', '413029']
    nosy_count = 8.0
    nosy_names = ['vinay.sajip', 'vstinner', 'christian.heimes', 'phsilva', 'eric.snow', 'maciej.szulik', 'pablogsal', 'Dennis Sweeney']
    pr_nums = ['13372', '13531', '15760', '15877', '16017', '16058', '16328', '16841', '22841', '23045', '23431', '23918', '23929', '31225', '31239', '31264']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue36876'
    versions = ['Python 3.8']

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented May 10, 2019

    We still have a bunch of "global" C variables (static globals, static locals, maybe thread-local storage) in our code-base that break the isolation between interpreters. I added Tools/c-globals/check-c-globals.py a while back to help identify such variables, however more have crept in. I also did not take static locals or thread-locals into account.

    To address the above, we should do the following:

    1. update check-c-globals.py to identify static locals (and thread-locals)
    2. deal with any identified globals
    3. add check-c-globals.py to "make check"
    4. (if "make check" isn't already there), ensure check-c-globals.py is run at some point in CI

    Separately, we should move fields out of _PyRuntimeState into PyInterpreterState wherever possible. That can also be done at step 2 if it's not too much work.

    @ericsnowcurrently ericsnowcurrently added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) 3.8 labels May 10, 2019
    @ericsnowcurrently ericsnowcurrently self-assigned this May 10, 2019
    @ericsnowcurrently ericsnowcurrently added the type-bug An unexpected behavior, bug, or error label May 10, 2019
    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented May 10, 2019

    Also, Tools/c-globals/ignored-globals.txt is a bit out of date (some vars have been removed, renamed, or moved to another file). That should get cleaned up. It might also make sense to update check-c-globals.py to verify that all variables in ignored-globals.txt actually exist.

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Sep 11, 2019

    New changeset ee536b2 by Eric Snow in branch 'master':
    bpo-36876: Add a tool that identifies unsupported global C variables. (bpo-15877)
    ee536b2

    @db3l
    Copy link
    Contributor

    db3l commented Sep 11, 2019

    The new test_check_c_globals.ActualChecks test is failing with an "unexpected success" on the bolen-ubuntu buildbot (under Ubuntu 18.04.3). I can reproduce the failure in a manually built tree.

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Sep 12, 2019

    @db3l, I'll take a look right away.

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Sep 12, 2019

    New changeset 64535fc by Eric Snow in branch 'master':
    bpo-36876: Skip test_check_c_globals for now. (gh-16017)
    64535fc

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Sep 12, 2019

    New changeset 088b63e by Eric Snow in branch 'master':
    bpo-36876: Fix the globals checker tool. (gh-16058)
    088b63e

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Oct 19, 2019

    New changeset e4c431e by Eric Snow in branch 'master':
    bpo-36876: Re-organize the c-analyzer tool code. (gh-16841)
    e4c431e

    @vsajip
    Copy link
    Member

    vsajip commented Nov 7, 2019

    New changeset 9def81a by Vinay Sajip in branch 'master':
    bpo-36876: Moved Parser/listnode.c statics to interpreter state. (GH-16328)
    9def81a

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Nov 23, 2019

    Thanks, Vinay!

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Nov 23, 2019

    FYI, others have been tackling this in separate issues (e.g. Victor, anyone relative to PEP-384).

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Nov 23, 2019

    And I *am* still working on the c-analyzer + a test to that runs it, so we can keep from adding more globals. :)

    @vstinner vstinner added expert-subinterpreters and removed interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) labels May 15, 2020
    @vstinner vstinner changed the title Global C variables are a problem. [subinterpreters] Global C variables are a problem May 15, 2020
    @vstinner
    Copy link
    Member

    vstinner commented May 15, 2020

    More and more C extensions are converted to multiphase initialization API (PEP-489) and their global variables are moved into a new module state.

    bpo-39465 will make _Py_IDENTIFIER compatible with subinterpreters.

    See also bpo-40521 for caches like free lists and Unicode interned strings.

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Oct 23, 2020

    New changeset 345cd37 by Eric Snow in branch 'master':
    bpo-36876: Fix the C analyzer tool. (GH-22841)
    345cd37

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Oct 30, 2020

    New changeset 4fe7209 by Eric Snow in branch 'master':
    bpo-36876: Small adjustments to the C-analyzer tool. (GH-23045)
    4fe7209

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Nov 20, 2020

    New changeset 9f02b47 by Eric Snow in branch 'master':
    bpo-36876: [c-analyzer tool] Tighten up the results and output. (GH-23431)
    9f02b47

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Dec 24, 2020

    New changeset 7ec59d8 by Eric Snow in branch 'master':
    bpo-36876: [c-analyzer tool] Add a "capi" subcommand to the c-analyzer tool. (gh-23918)
    7ec59d8

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Dec 25, 2020

    New changeset 5ae9be6 by Eric Snow in branch 'master':
    bpo-36876: [c-analyzer tool] Additional CLI updates for "capi" command. (gh-23929)
    5ae9be6

    @sweeneyde
    Copy link
    Member

    sweeneyde commented May 17, 2021

    I'm getting the following FutureWarning on a certain regular expression. I think it just needs "[]" to become "\[\]".

    0:05:36 load avg: 0.00 [ 53/427] test_check_c_globals
    ...\Tools\c-analyzer\c_common\tables.py:236: FutureWarning: Possible nested set at position 12
    _COLSPEC_RE = re.compile(textwrap.dedent(r'''

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Feb 9, 2022

    New changeset 77bab59 by Eric Snow in branch 'main':
    bpo-36876: Update the c-analyzer whitelist. (gh-31225)
    77bab59

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Feb 10, 2022

    New changeset cb68788 by Eric Snow in branch 'main':
    bpo-36876: Minor cleanup to c-analyzer "ignored" data.' (gh-31239)
    cb68788

    @ericsnowcurrently
    Copy link
    Member Author

    ericsnowcurrently commented Feb 10, 2022

    New changeset 80e4f26 by Eric Snow in branch 'main':
    bpo-36876: Make sure the c-analyzer is checking all the source files.' (gh-31264)
    80e4f26

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    ericsnowcurrently added a commit that referenced this issue Nov 11, 2022
    … Objects Initializer (gh-99389)
    
    Up until now we had a single generated initializer macro for all the statically declared global objects in _PyRuntimeState, including several one-offs (e.g. the empty tuple). The one-offs don't need to be generated, but were because we had one big initializer. Having separate initializers for set of generated global objects allows us to generate only the ones we need to.  This allows us to add initializers for one-off global objects without having to generate them.
    
    #81057
    ericsnowcurrently added a commit that referenced this issue Nov 11, 2022
    We also move the closely related max_module_number and add comments documenting the group of struct members.
    
    #81057
    ericsnowcurrently added a commit that referenced this issue Nov 11, 2022
    As we consolidate global variables, we find some objects that are almost suitable to add to _PyRuntimeState.global_objects, but have some small/sneaky bit of per-interpreter state (e.g. a weakref list). We're adding PyInterpreterState.static_objects so we can move such objects there. (We'll removed the _not_used field once we've added others.)
    
    #81057
    ericsnowcurrently added a commit that referenced this issue Nov 11, 2022
    The global allocators were stored in 3 static global variables: _PyMem_Raw, _PyMem, and _PyObject.  State for the "small block" allocator was stored in another 13.  That makes a total of 16 global variables. We are moving all 16 to the _PyRuntimeState struct as part of the work for gh-81057.  (If PEP 684 is accepted then we will follow up by moving them all to PyInterpreterState.)
    
    #81057
    ericsnowcurrently added a commit that referenced this issue Nov 12, 2022
    We actually don't move PyImport_Inittab.  Instead, we make a copy that we keep on _PyRuntimeState and use only that after Py_Initialize().  We also prevent folks from modifying PyImport_Inittab (the best we can) after that point.
    
    #81057
    ethanfurman pushed a commit to ethanfurman/cpython that referenced this issue Nov 12, 2022
    …Global Objects Initializer (pythongh-99389)
    
    Up until now we had a single generated initializer macro for all the statically declared global objects in _PyRuntimeState, including several one-offs (e.g. the empty tuple). The one-offs don't need to be generated, but were because we had one big initializer. Having separate initializers for set of generated global objects allows us to generate only the ones we need to.  This allows us to add initializers for one-off global objects without having to generate them.
    
    python#81057
    ericsnowcurrently added a commit that referenced this issue Nov 14, 2022
    …h-99487)
    
    This moves nearly all remaining object-holding globals in core code (other than static types).
    
    #81057
    ericsnowcurrently added a commit that referenced this issue Nov 15, 2022
    This is the first of several changes to consolidate non-object globals in core code.
    
    #81057
    CuriousLearner added a commit to CuriousLearner/cpython that referenced this issue Nov 16, 2022
    * main: (8272 commits)
      Update Windows readme.txt to clarify Visual Studio required versions (pythonGH-99522)
      pythongh-99460 Emscripten trampolines on optimized METH_O and METH_NOARGS code paths (python#99461)
      pythongh-92647: [Enum] use final status to determine lookup or create (pythonGH-99500)
      pythongh-81057: Move Globals in Core Code to _PyRuntimeState (pythongh-99496)
      Post 3.12.0a2
      pythongh-99300: Use Py_NewRef() in Python/Python-ast.c (python#99499)
      pythongh-93649: Split pytime and datetime tests from _testcapimodule.c (python#99494)
      pythongh-99370: fix test_zippath_from_non_installed_posix (pythonGH-99483)
      pythonGH-99205: remove `_static` field from `PyThreadState` and `PyInterpreterState` (pythonGH-99385)
      pythongh-81057: Move the Remaining Import State Globals to _PyRuntimeState (pythongh-99488)
      pythongh-87604: Avoid publishing list of active per-interpreter audit hooks via the gc module (pythonGH-99373)
      pythongh-93649: Split getargs tests from _testcapimodule.c (python#99346)
      pythongh-81057: Move Global Variables Holding Objects to _PyRuntimeState. (pythongh-99487)
      pythonGH-98219: reduce sleep time in `asyncio` subprocess test (python#99464)
      pythonGH-99388: add `loop_factory` parameter to `asyncio.run` (python#99462)
      pythongh-99300: Use Py_NewRef() in PC/ directory (python#99479)
      pythongh-99300: Use Py_NewRef() in Doc/ directory  (python#99480)
      pythongh-99300: Use Py_NewRef() in Modules/ directory (python#99473)
      pythongh-99300: Use Py_NewRef() in Modules/ directory (python#99469)
      pythongh-99370: Calculate zip path from prefix when in a venv (pythonGH-99371)
      ...
    ericsnowcurrently added a commit that referenced this issue Nov 16, 2022
    This is part of the effort to consolidate global variables, to make them easier to manage (and make it easier to later move some of them to PyInterpreterState).
    
    #81057
    ericsnowcurrently added a commit that referenced this issue Nov 16, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 expert-subinterpreters type-bug An unexpected behavior, bug, or error
    Projects
    Status: Todo
    Development

    No branches or pull requests

    5 participants