python
[subinterpreters] Global C variables are a problem
| BPO | 36876 |
|---|---|
| Nosy | @vsajip, @vstinner, @tiran, @phsilva, @ericsnowcurrently, @soltysh, @pablogsal, @sweeneyde |
| PRs |
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']
- PR: gh-99217
- PR: gh-99355
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:
- update check-c-globals.py to identify static locals (and thread-locals)
- deal with any identified globals
- move them to _PyRuntimeState (or thread-locals to PyThreadState, etc.)
- ignore them by adding them to Tools/c-globals/ignored-globals.txt
- add check-c-globals.py to "make check"
- (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.
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.
New changeset ee536b2020b1f0baad1286dbd4345e13870324af by Eric Snow in branch 'master': bpo-36876: Add a tool that identifies unsupported global C variables. (bpo-15877) https://github.com/python/cpython/commit/ee536b2020b1f0baad1286dbd4345e13870324af
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.
New changeset 64535fc6c0712caef0bc46be30e661f7ccf8280e by Eric Snow in branch 'master': bpo-36876: Skip test_check_c_globals for now. (gh-16017) https://github.com/python/cpython/commit/64535fc6c0712caef0bc46be30e661f7ccf8280e
New changeset 088b63ea7a8331a3e34bc93c3b873c60354b4fad by Eric Snow in branch 'master': bpo-36876: Fix the globals checker tool. (gh-16058) https://github.com/python/cpython/commit/088b63ea7a8331a3e34bc93c3b873c60354b4fad
New changeset e4c431ecf50def40eb93c3969c1e4eeaf7bf32f1 by Eric Snow in branch 'master': bpo-36876: Re-organize the c-analyzer tool code. (gh-16841) https://github.com/python/cpython/commit/e4c431ecf50def40eb93c3969c1e4eeaf7bf32f1
New changeset 9def81aa52adc3cc89554156e40742cf17312825 by Vinay Sajip in branch 'master': bpo-36876: Moved Parser/listnode.c statics to interpreter state. (GH-16328) https://github.com/python/cpython/commit/9def81aa52adc3cc89554156e40742cf17312825
FYI, others have been tackling this in separate issues (e.g. Victor, anyone relative to PEP-384).
And I *am* still working on the c-analyzer + a test to that runs it, so we can keep from adding more globals. :)
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.
New changeset 345cd37abe324ad4f60f80e2c3133b8849e54e9b by Eric Snow in branch 'master': bpo-36876: Fix the C analyzer tool. (GH-22841) https://github.com/python/cpython/commit/345cd37abe324ad4f60f80e2c3133b8849e54e9b
New changeset 4fe72090deb7fb7bc09bfa56c92f6b3b0967d395 by Eric Snow in branch 'master': bpo-36876: Small adjustments to the C-analyzer tool. (GH-23045) https://github.com/python/cpython/commit/4fe72090deb7fb7bc09bfa56c92f6b3b0967d395
New changeset 9f02b479e6b6b48d0c2aad621978cff82e530b15 by Eric Snow in branch 'master': bpo-36876: [c-analyzer tool] Tighten up the results and output. (GH-23431) https://github.com/python/cpython/commit/9f02b479e6b6b48d0c2aad621978cff82e530b15
New changeset 7ec59d8861ef1104c3028678b2cacde4c5693e19 by Eric Snow in branch 'master': bpo-36876: [c-analyzer tool] Add a "capi" subcommand to the c-analyzer tool. (gh-23918) https://github.com/python/cpython/commit/7ec59d8861ef1104c3028678b2cacde4c5693e19
New changeset 5ae9be68d9f1a628fdc920b647257f94afb77887 by Eric Snow in branch 'master': bpo-36876: [c-analyzer tool] Additional CLI updates for "capi" command. (gh-23929) https://github.com/python/cpython/commit/5ae9be68d9f1a628fdc920b647257f94afb77887
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'''
New changeset 77bab59c8a1f04922bb975cc4f11e5323d1d379d by Eric Snow in branch 'main': bpo-36876: Update the c-analyzer whitelist. (gh-31225) https://github.com/python/cpython/commit/77bab59c8a1f04922bb975cc4f11e5323d1d379d
New changeset cb68788dcadf43b47292bab7816a5ed9efa69730 by Eric Snow in branch 'main': bpo-36876: Minor cleanup to c-analyzer "ignored" data.' (gh-31239) https://github.com/python/cpython/commit/cb68788dcadf43b47292bab7816a5ed9efa69730
New changeset 80e4f262aa27a39abf3fadc19a6323fea4607a8f by Eric Snow in branch 'main': bpo-36876: Make sure the c-analyzer is checking all the source files.' (gh-31264) https://github.com/python/cpython/commit/80e4f262aa27a39abf3fadc19a6323fea4607a8f
I still don't understand how moving global variables into _PyRuntime does solve any problem. What are the advantages of moving a variable declared as "static" in a C file into _PyRuntime? I don't get the rationale.
For me, it just sounds more complicated to get _PyRuntime.threads._condattr_monotonic.ptr instead of condattr_monotonic. It's annoying to have to repeat all conditions to decide if a variable should be declared or not in _PyRuntimeState, since they are tons of variables, and many are platform specific. A side effect is that any C file using _PyRuntime should now include headers files required to decide if platform specific variables should be declared or not.
I already noticed this problem when I made global variables per-interpreter: move them into PyInterpreterState. See the long list of direct includes in pycore_interp.h:
#include "pycore_atomic.h" // _Py_atomic_address
#include "pycore_ast_state.h" // struct ast_state
#include "pycore_code.h" // struct callable_cache
#include "pycore_context.h" // struct _Py_context_state
#include "pycore_dict.h" // struct _Py_dict_state
#include "pycore_exceptions.h" // struct _Py_exc_state
#include "pycore_floatobject.h" // struct _Py_float_state
#include "pycore_genobject.h" // struct _Py_async_gen_state
#include "pycore_gil.h" // struct _gil_runtime_state
#include "pycore_gc.h" // struct _gc_runtime_state
#include "pycore_list.h" // struct _Py_list_state
#include "pycore_tuple.h" // struct _Py_tuple_state
#include "pycore_typeobject.h" // struct type_cache
#include "pycore_unicodeobject.h" // struct _Py_unicode_state
#include "pycore_warnings.h" // struct _warnings_runtime_state
But they are also many indirect includes, like <windows.h> which is quite big and has many side effects :-(