gh-81057: Add a CI Check for New Unsupported C Global Variables (gh-1…

…02506)

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

.github/workflows/build.yml

@@ -111,6 +111,9 @@ jobs:
         run: make smelly
       - name: Check limited ABI symbols
         run: make check-limited-abi
+      - name: Check for unsupported C global variables
+        if: github.event_name == 'pull_request'  # $GITHUB_EVENT_NAME
+        run: make check-c-globals
 
   build_win32:
     name: 'Windows (x86)'

Makefile.pre.in

@@ -2560,6 +2560,12 @@ distclean: clobber docclean
 smelly: all
 	$(RUNSHARED) ./$(BUILDPYTHON) $(srcdir)/Tools/build/smelly.py
 
+# Check if any unsupported C global variables have been added.
+check-c-globals:
+	$(PYTHON_FOR_REGEN) $(srcdir)/Tools/c-analyzer/check-c-globals.py \
+		--format summary \
+		--traceback
+
 # Find files with funny names
 funny:
 	find $(SUBDIRS) $(SUBDIRSTOO) \

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

@@ -115,15 +115,15 @@ def converted_error(tool, argv, filename):
 def convert_error(tool, argv, filename, stderr, rc):
     error = (stderr.splitlines()[0], rc)
     if (_expected := is_os_mismatch(filename, stderr)):
-        logger.debug(stderr.strip())
+        logger.info(stderr.strip())
         raise OSMismatchError(filename, _expected, argv, error, tool)
     elif (_missing := is_missing_dep(stderr)):
-        logger.debug(stderr.strip())
+        logger.info(stderr.strip())
         raise MissingDependenciesError(filename, (_missing,), argv, error, tool)
     elif '#error' in stderr:
         # XXX Ignore incompatible files.
         error = (stderr.splitlines()[1], rc)
-        logger.debug(stderr.strip())
+        logger.info(stderr.strip())
         raise ErrorDirectiveError(filename, argv, error, tool)
     else:
         # Try one more time, with stderr written to the terminal.

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

@@ -6,6 +6,11 @@
 
 TOOL = 'gcc'
 
+META_FILES = {
+    '<built-in>',
+    '<command-line>',
+}
+
 # https://gcc.gnu.org/onlinedocs/cpp/Preprocessor-Output.html
 # flags:
 #  1  start of a new file
@@ -75,11 +80,15 @@ def _iter_lines(text, reqfile, samefiles, cwd, raw=False):
 
     # The first line is special.
     # The next two lines are consistent.
-    for expected in [
-        f'# 1 "{reqfile}"',
-        '# 1 "<built-in>"',
-        '# 1 "<command-line>"',
-    ]:
+    firstlines = [
+        f'# 0 "{reqfile}"',
+        '# 0 "<built-in>"',
+        '# 0 "<command-line>"',
+    ]
+    if text.startswith('# 1 '):
+        # Some preprocessors emit a lineno of 1 for line-less entries.
+        firstlines = [l.replace('# 0 ', '# 1 ') for l in firstlines]
+    for expected in firstlines:
         line = next(lines)
         if line != expected:
             raise NotImplementedError((line, expected))
@@ -121,7 +130,7 @@ def _iter_top_include_lines(lines, topfile, cwd,
     # _parse_marker_line() that the preprocessor reported lno as 1.
     lno = 1
     for line in lines:
-        if line == '# 1 "<command-line>" 2':
+        if line == '# 0 "<command-line>" 2' or line == '# 1 "<command-line>" 2':
             # We're done with this top-level include.
             return
 
@@ -174,8 +183,8 @@ def _parse_marker_line(line, reqfile=None):
         return None, None, None
     lno, origfile, flags = m.groups()
     lno = int(lno)
+    assert origfile not in META_FILES, (line,)
     assert lno > 0, (line, lno)
-    assert origfile not in ('<built-in>', '<command-line>'), (line,)
     flags = set(int(f) for f in flags.split()) if flags else ()
 
     if 1 in flags:

Tools/c-analyzer/cpython/__main__.py

@@ -1,5 +1,6 @@
 import logging
 import sys
+import textwrap
 
 from c_common.fsutil import expand_filenames, iter_files_by_suffix
 from c_common.scriptutil import (
@@ -26,6 +27,39 @@
 logger = logging.getLogger(__name__)
 
 
+CHECK_EXPLANATION = textwrap.dedent('''
+    -------------------------
+
+    Non-constant global variables are generally not supported
+    in the CPython repo.  We use a tool to analyze the C code
+    and report if any unsupported globals are found.  The tool
+    may be run manually with:
+
+      ./python Tools/c-analyzer/check-c-globals.py --format summary [FILE]
+
+    Occasionally the tool is unable to parse updated code.
+    If this happens then add the file to the "EXCLUDED" list
+    in Tools/c-analyzer/cpython/_parser.py and create a new
+    issue for fixing the tool (and CC ericsnowcurrently
+    on the issue).
+
+    If the tool reports an unsupported global variable and
+    it is actually const (and thus supported) then first try
+    fixing the declaration appropriately in the code.  If that
+    doesn't work then add the variable to the "should be const"
+    section of Tools/c-analyzer/cpython/ignored.tsv.
+
+    If the tool otherwise reports an unsupported global variable
+    then first try to make it non-global, possibly adding to
+    PyInterpreterState (for core code) or module state (for
+    extension modules).  In an emergency, you can add the
+    variable to Tools/c-analyzer/cpython/globals-to-fix.tsv
+    to get CI passing, but doing so should be avoided.  If
+    this course it taken, be sure to create an issue for
+    eliminating the global (and CC ericsnowcurrently).
+''')
+
+
 def _resolve_filenames(filenames):
     if filenames:
         resolved = (_files.resolve_filename(f) for f in filenames)
@@ -123,14 +157,26 @@ def _cli_check(parser, **kwargs):
 def cmd_check(filenames=None, **kwargs):
     filenames = _resolve_filenames(filenames)
     kwargs['get_file_preprocessor'] = _parser.get_preprocessor(log_err=print)
-    c_analyzer.cmd_check(
-        filenames,
-        relroot=REPO_ROOT,
-        _analyze=_analyzer.analyze,
-        _CHECKS=CHECKS,
-        file_maxsizes=_parser.MAX_SIZES,
-        **kwargs
-    )
+    try:
+        c_analyzer.cmd_check(
+            filenames,
+            relroot=REPO_ROOT,
+            _analyze=_analyzer.analyze,
+            _CHECKS=CHECKS,
+            file_maxsizes=_parser.MAX_SIZES,
+            **kwargs
+        )
+    except SystemExit as exc:
+        num_failed = exc.args[0] if getattr(exc, 'args', None) else None
+        if isinstance(num_failed, int):
+            if num_failed > 0:
+                sys.stderr.flush()
+                print(CHECK_EXPLANATION, flush=True)
+        raise  # re-raise
+    except Exception:
+        sys.stderr.flush()
+        print(CHECK_EXPLANATION, flush=True)
+        raise  # re-raise
 
 
 def cmd_analyze(filenames=None, **kwargs):

Tools/c-analyzer/cpython/_parser.py

@@ -106,15 +106,20 @@ def clean_lines(text):
 *	./Include/internal
 
 Modules/_decimal/**/*.c	Modules/_decimal/libmpdec
+Modules/_elementtree.c	Modules/expat
 Modules/_hacl/*.c	Modules/_hacl/include
 Modules/_hacl/*.h	Modules/_hacl/include
-Modules/_tkinter.c	/usr/include/tcl8.6
 Modules/md5module.c	Modules/_hacl/include
 Modules/sha1module.c	Modules/_hacl/include
 Modules/sha2module.c	Modules/_hacl/include
-Modules/tkappinit.c	/usr/include/tcl
 Objects/stringlib/*.h	Objects
 
+# possible system-installed headers, just in case
+Modules/_tkinter.c	/usr/include/tcl8.6
+Modules/_uuidmodule.c	/usr/include/uuid
+Modules/nismodule.c	/usr/include/tirpc
+Modules/tkappinit.c	/usr/include/tcl
+
 # @end=tsv@
 ''')[1:]
 

Tools/c-analyzer/cpython/ignored.tsv

@@ -228,6 +228,7 @@ Modules/_xxinterpchannelsmodule.c	-	_channelid_end_recv	-
 Modules/_xxinterpchannelsmodule.c	-	_channelid_end_send	-
 Modules/_zoneinfo.c	-	DAYS_BEFORE_MONTH	-
 Modules/_zoneinfo.c	-	DAYS_IN_MONTH	-
+Modules/_xxsubinterpretersmodule.c	-	no_exception	-
 Modules/arraymodule.c	-	descriptors	-
 Modules/arraymodule.c	-	emptybuf	-
 Modules/cjkcodecs/_codecs_cn.c	-	_mapping_list	-