Skip to content

Commit 038b67c

Browse files
ericsnowcurrentlyFidget-Spinner
authored andcommitted
pythongh-81057: Add a CI Check for New Unsupported C Global Variables (pythongh-102506)
This will keep us from adding new unsupported (i.e. non-const) C global variables, which would break interpreter isolation. FYI, historically it is very uncommon for new global variables to get added. Furthermore, it is rare for new code to break the c-analyzer. So the check should almost always pass unnoticed. Note that I've removed test_check_c_globals. A test wasn't a great fit conceptually and was super slow on debug builds. A CI check is a better fit. This also resolves pythongh-100237. python#81057
1 parent c92b445 commit 038b67c

File tree

8 files changed

+90
-54
lines changed

8 files changed

+90
-54
lines changed

.github/workflows/build.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,9 @@ jobs:
111111
run: make smelly
112112
- name: Check limited ABI symbols
113113
run: make check-limited-abi
114+
- name: Check for unsupported C global variables
115+
if: github.event_name == 'pull_request' # $GITHUB_EVENT_NAME
116+
run: make check-c-globals
114117

115118
build_win32:
116119
name: 'Windows (x86)'

Lib/test/test_check_c_globals.py

Lines changed: 0 additions & 34 deletions
This file was deleted.

Makefile.pre.in

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2560,6 +2560,12 @@ distclean: clobber docclean
25602560
smelly: all
25612561
$(RUNSHARED) ./$(BUILDPYTHON) $(srcdir)/Tools/build/smelly.py
25622562

2563+
# Check if any unsupported C global variables have been added.
2564+
check-c-globals:
2565+
$(PYTHON_FOR_REGEN) $(srcdir)/Tools/c-analyzer/check-c-globals.py \
2566+
--format summary \
2567+
--traceback
2568+
25632569
# Find files with funny names
25642570
funny:
25652571
find $(SUBDIRS) $(SUBDIRSTOO) \

Tools/c-analyzer/c_parser/preprocessor/common.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,15 @@ def converted_error(tool, argv, filename):
115115
def convert_error(tool, argv, filename, stderr, rc):
116116
error = (stderr.splitlines()[0], rc)
117117
if (_expected := is_os_mismatch(filename, stderr)):
118-
logger.debug(stderr.strip())
118+
logger.info(stderr.strip())
119119
raise OSMismatchError(filename, _expected, argv, error, tool)
120120
elif (_missing := is_missing_dep(stderr)):
121-
logger.debug(stderr.strip())
121+
logger.info(stderr.strip())
122122
raise MissingDependenciesError(filename, (_missing,), argv, error, tool)
123123
elif '#error' in stderr:
124124
# XXX Ignore incompatible files.
125125
error = (stderr.splitlines()[1], rc)
126-
logger.debug(stderr.strip())
126+
logger.info(stderr.strip())
127127
raise ErrorDirectiveError(filename, argv, error, tool)
128128
else:
129129
# Try one more time, with stderr written to the terminal.

Tools/c-analyzer/c_parser/preprocessor/gcc.py

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@
66

77
TOOL = 'gcc'
88

9+
META_FILES = {
10+
'<built-in>',
11+
'<command-line>',
12+
}
13+
914
# https://gcc.gnu.org/onlinedocs/cpp/Preprocessor-Output.html
1015
# flags:
1116
# 1 start of a new file
@@ -75,11 +80,15 @@ def _iter_lines(text, reqfile, samefiles, cwd, raw=False):
7580

7681
# The first line is special.
7782
# The next two lines are consistent.
78-
for expected in [
79-
f'# 1 "{reqfile}"',
80-
'# 1 "<built-in>"',
81-
'# 1 "<command-line>"',
82-
]:
83+
firstlines = [
84+
f'# 0 "{reqfile}"',
85+
'# 0 "<built-in>"',
86+
'# 0 "<command-line>"',
87+
]
88+
if text.startswith('# 1 '):
89+
# Some preprocessors emit a lineno of 1 for line-less entries.
90+
firstlines = [l.replace('# 0 ', '# 1 ') for l in firstlines]
91+
for expected in firstlines:
8392
line = next(lines)
8493
if line != expected:
8594
raise NotImplementedError((line, expected))
@@ -121,7 +130,7 @@ def _iter_top_include_lines(lines, topfile, cwd,
121130
# _parse_marker_line() that the preprocessor reported lno as 1.
122131
lno = 1
123132
for line in lines:
124-
if line == '# 1 "<command-line>" 2':
133+
if line == '# 0 "<command-line>" 2' or line == '# 1 "<command-line>" 2':
125134
# We're done with this top-level include.
126135
return
127136

@@ -174,8 +183,8 @@ def _parse_marker_line(line, reqfile=None):
174183
return None, None, None
175184
lno, origfile, flags = m.groups()
176185
lno = int(lno)
186+
assert origfile not in META_FILES, (line,)
177187
assert lno > 0, (line, lno)
178-
assert origfile not in ('<built-in>', '<command-line>'), (line,)
179188
flags = set(int(f) for f in flags.split()) if flags else ()
180189

181190
if 1 in flags:

Tools/c-analyzer/cpython/__main__.py

Lines changed: 54 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import logging
22
import sys
3+
import textwrap
34

45
from c_common.fsutil import expand_filenames, iter_files_by_suffix
56
from c_common.scriptutil import (
@@ -26,6 +27,39 @@
2627
logger = logging.getLogger(__name__)
2728

2829

30+
CHECK_EXPLANATION = textwrap.dedent('''
31+
-------------------------
32+
33+
Non-constant global variables are generally not supported
34+
in the CPython repo. We use a tool to analyze the C code
35+
and report if any unsupported globals are found. The tool
36+
may be run manually with:
37+
38+
./python Tools/c-analyzer/check-c-globals.py --format summary [FILE]
39+
40+
Occasionally the tool is unable to parse updated code.
41+
If this happens then add the file to the "EXCLUDED" list
42+
in Tools/c-analyzer/cpython/_parser.py and create a new
43+
issue for fixing the tool (and CC ericsnowcurrently
44+
on the issue).
45+
46+
If the tool reports an unsupported global variable and
47+
it is actually const (and thus supported) then first try
48+
fixing the declaration appropriately in the code. If that
49+
doesn't work then add the variable to the "should be const"
50+
section of Tools/c-analyzer/cpython/ignored.tsv.
51+
52+
If the tool otherwise reports an unsupported global variable
53+
then first try to make it non-global, possibly adding to
54+
PyInterpreterState (for core code) or module state (for
55+
extension modules). In an emergency, you can add the
56+
variable to Tools/c-analyzer/cpython/globals-to-fix.tsv
57+
to get CI passing, but doing so should be avoided. If
58+
this course it taken, be sure to create an issue for
59+
eliminating the global (and CC ericsnowcurrently).
60+
''')
61+
62+
2963
def _resolve_filenames(filenames):
3064
if filenames:
3165
resolved = (_files.resolve_filename(f) for f in filenames)
@@ -123,14 +157,26 @@ def _cli_check(parser, **kwargs):
123157
def cmd_check(filenames=None, **kwargs):
124158
filenames = _resolve_filenames(filenames)
125159
kwargs['get_file_preprocessor'] = _parser.get_preprocessor(log_err=print)
126-
c_analyzer.cmd_check(
127-
filenames,
128-
relroot=REPO_ROOT,
129-
_analyze=_analyzer.analyze,
130-
_CHECKS=CHECKS,
131-
file_maxsizes=_parser.MAX_SIZES,
132-
**kwargs
133-
)
160+
try:
161+
c_analyzer.cmd_check(
162+
filenames,
163+
relroot=REPO_ROOT,
164+
_analyze=_analyzer.analyze,
165+
_CHECKS=CHECKS,
166+
file_maxsizes=_parser.MAX_SIZES,
167+
**kwargs
168+
)
169+
except SystemExit as exc:
170+
num_failed = exc.args[0] if getattr(exc, 'args', None) else None
171+
if isinstance(num_failed, int):
172+
if num_failed > 0:
173+
sys.stderr.flush()
174+
print(CHECK_EXPLANATION, flush=True)
175+
raise # re-raise
176+
except Exception:
177+
sys.stderr.flush()
178+
print(CHECK_EXPLANATION, flush=True)
179+
raise # re-raise
134180

135181

136182
def cmd_analyze(filenames=None, **kwargs):

Tools/c-analyzer/cpython/_parser.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,15 +106,20 @@ def clean_lines(text):
106106
* ./Include/internal
107107
108108
Modules/_decimal/**/*.c Modules/_decimal/libmpdec
109+
Modules/_elementtree.c Modules/expat
109110
Modules/_hacl/*.c Modules/_hacl/include
110111
Modules/_hacl/*.h Modules/_hacl/include
111-
Modules/_tkinter.c /usr/include/tcl8.6
112112
Modules/md5module.c Modules/_hacl/include
113113
Modules/sha1module.c Modules/_hacl/include
114114
Modules/sha2module.c Modules/_hacl/include
115-
Modules/tkappinit.c /usr/include/tcl
116115
Objects/stringlib/*.h Objects
117116
117+
# possible system-installed headers, just in case
118+
Modules/_tkinter.c /usr/include/tcl8.6
119+
Modules/_uuidmodule.c /usr/include/uuid
120+
Modules/nismodule.c /usr/include/tirpc
121+
Modules/tkappinit.c /usr/include/tcl
122+
118123
# @end=tsv@
119124
''')[1:]
120125

Tools/c-analyzer/cpython/ignored.tsv

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,7 @@ Modules/_xxinterpchannelsmodule.c - _channelid_end_recv -
228228
Modules/_xxinterpchannelsmodule.c - _channelid_end_send -
229229
Modules/_zoneinfo.c - DAYS_BEFORE_MONTH -
230230
Modules/_zoneinfo.c - DAYS_IN_MONTH -
231+
Modules/_xxsubinterpretersmodule.c - no_exception -
231232
Modules/arraymodule.c - descriptors -
232233
Modules/arraymodule.c - emptybuf -
233234
Modules/cjkcodecs/_codecs_cn.c - _mapping_list -

0 commit comments

Comments
 (0)