Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 1ff81c0

Browse filesBrowse files
ericsnowcurrentlyericsnowcurrently
authored
gh-81057: Add a CI Check for New Unsupported C Global Variables (gh-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 gh-100237. #81057
1 parent a703f74 commit 1ff81c0
Copy full SHA for 1ff81c0

File tree

Expand file treeCollapse file tree

8 files changed

+90
-54
lines changed
Filter options
Expand file treeCollapse file tree

8 files changed

+90
-54
lines changed

‎.github/workflows/build.yml

Copy file name to clipboardExpand all lines: .github/workflows/build.yml
+3Lines 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

Copy file name to clipboardExpand all lines: Lib/test/test_check_c_globals.py
-34Lines changed: 0 additions & 34 deletions
This file was deleted.

‎Makefile.pre.in

Copy file name to clipboardExpand all lines: Makefile.pre.in
+6Lines 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

Copy file name to clipboardExpand all lines: Tools/c-analyzer/c_parser/preprocessor/common.py
+3-3Lines 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

Copy file name to clipboardExpand all lines: Tools/c-analyzer/c_parser/preprocessor/gcc.py
+16-7Lines 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

Copy file name to clipboardExpand all lines: Tools/c-analyzer/cpython/__main__.py
+54-8Lines 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

Copy file name to clipboardExpand all lines: Tools/c-analyzer/cpython/_parser.py
+7-2Lines 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

Copy file name to clipboardExpand all lines: Tools/c-analyzer/cpython/ignored.tsv
+1Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.