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

Jump to bottom

[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
@ericsnowcurrently
Labels
3.8 expert-subinterpreters type-bug

Comments

@ericsnowcurrently
Copy link

@ericsnowcurrently ericsnowcurrently commented May 10, 2019

BPO 36876
Nosy @vsajip, @vstinner, @tiran, @phsilva, @ericsnowcurrently, @soltysh, @pablogsal, @sweeneyde
PRs
  • #13372
  • #13531
  • #15760
  • #15877
  • #16017
  • #16058
  • #16328
  • #16841
  • #22841
  • #23045
  • #23431
  • #23918
  • #23929
  • #31225
  • #31239
  • #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
    Author

    @ericsnowcurrently 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 3.8 labels May 10, 2019
    @ericsnowcurrently ericsnowcurrently self-assigned this May 10, 2019
    @ericsnowcurrently ericsnowcurrently added the type-bug label May 10, 2019
    @ericsnowcurrently
    Copy link
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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

    @db3l 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
    Author

    @ericsnowcurrently ericsnowcurrently commented Sep 12, 2019

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

    @ericsnowcurrently
    Copy link
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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

    @vsajip 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
    Author

    @ericsnowcurrently ericsnowcurrently commented Nov 23, 2019

    Thanks, Vinay!

    @ericsnowcurrently
    Copy link
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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 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

    @vstinner 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
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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

    @sweeneyde 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
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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
    Author

    @ericsnowcurrently 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 May 3, 2022
    @ericsnowcurrently
    gh-81057: Get the c-analyzer tool working again. (gh-92246)
    456cd51
    ericsnowcurrently added a commit that referenced this issue May 3, 2022
    @ericsnowcurrently
    gh-81057: Fix indent. (gh-92267)
    1f631ae
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @ericsnowcurrently ericsnowcurrently

    Labels
    3.8 expert-subinterpreters type-bug
    Projects
    Subinterpreters
    Status: Todo
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @vsajip @vstinner @ericsnowcurrently @db3l @sweeneyde