From 32165ccb22c98b5d1506021c3a567e65e520cdf5 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 20 Nov 2020 15:07:58 +0100 Subject: [PATCH 1/3] bpo-42212: smelly.py also checks the dynamic library The smelly.py script now also checks the Python dynamic library and extension modules, not only the Python static library. Make also the script more verbose: explain what it does. The GitHub Action job now builds Python with the libpython dynamic library. --- .github/workflows/build.yml | 3 +- .../2020-11-20-15-11-05.bpo-42212.sjzgOf.rst | 3 + Tools/scripts/smelly.py | 150 ++++++++++++++---- 3 files changed, 120 insertions(+), 36 deletions(-) create mode 100644 Misc/NEWS.d/next/Tools-Demos/2020-11-20-15-11-05.bpo-42212.sjzgOf.rst diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c1a017165665f6..f543a94af363b8 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -60,7 +60,8 @@ jobs: run: sudo ./.github/workflows/posix-deps-apt.sh - name: Build CPython run: | - ./configure --with-pydebug + # Build Python with the libpython dynamic library + ./configure --with-pydebug --enable-shared make -j4 regen-all - name: Check for changes run: | diff --git a/Misc/NEWS.d/next/Tools-Demos/2020-11-20-15-11-05.bpo-42212.sjzgOf.rst b/Misc/NEWS.d/next/Tools-Demos/2020-11-20-15-11-05.bpo-42212.sjzgOf.rst new file mode 100644 index 00000000000000..d2cbe3de6fe92e --- /dev/null +++ b/Misc/NEWS.d/next/Tools-Demos/2020-11-20-15-11-05.bpo-42212.sjzgOf.rst @@ -0,0 +1,3 @@ +The smelly.py script now also checks the Python dynamic library and extension +modules, not only the Python static library. Make also the script more verbose: +explain what it does. diff --git a/Tools/scripts/smelly.py b/Tools/scripts/smelly.py index 43d091654d2dc8..014f2fe51a8ff1 100755 --- a/Tools/scripts/smelly.py +++ b/Tools/scripts/smelly.py @@ -1,17 +1,45 @@ #!/usr/bin/env python # Script checking that all symbols exported by libpython start with Py or _Py +import os.path import subprocess import sys import sysconfig -def get_exported_symbols(): - LIBRARY = sysconfig.get_config_var('LIBRARY') - if not LIBRARY: - raise Exception("failed to get LIBRARY") +ALLOWED_PREFIXES = ('Py', '_Py') +if sys.platform == 'darwin': + ALLOWED_PREFIXES += ('__Py',) + +IGNORE_EXTENSION = "_ctypes_test" + + +def is_local_symbol_type(symtype): + # Ignore local symbols. + + # If lowercase, the symbol is usually local; if uppercase, the symbol + # is global (external). There are however a few lowercase symbols that + # are shown for special global symbols ("u", "v" and "w"). + if symtype.islower() and symtype not in "uvw": + return True + + # Ignore the initialized data section (d and D) and the BSS data + # section. For example, ignore "__bss_start (type: B)" + # and "_edata (type: D)". + if symtype in "bBdD": + return True + + return False + - args = ('nm', '-p', LIBRARY) +def get_exported_symbols(library, dynamic=False): + print(f"Check that {library} only exports symbols starting with Py or _Py") + + # Only look at dynamic symbols + args = ['nm', '--no-sort'] + if dynamic: + args.append('--dynamic') + args.append(library) print("+ %s" % ' '.join(args)) proc = subprocess.run(args, stdout=subprocess.PIPE, universal_newlines=True) if proc.returncode: @@ -25,12 +53,9 @@ def get_exported_symbols(): def get_smelly_symbols(stdout): - symbols = [] - ignored_symtypes = set() - - allowed_prefixes = ('Py', '_Py') - if sys.platform == 'darwin': - allowed_prefixes += ('__Py',) + smelly_symbols = [] + python_symbols = [] + local_symbols = [] for line in stdout.splitlines(): # Split line '0000000000001b80 D PyTextIOWrapper_Type' @@ -42,41 +67,96 @@ def get_smelly_symbols(stdout): continue symtype = parts[1].strip() - # Ignore private symbols. - # - # If lowercase, the symbol is usually local; if uppercase, the symbol - # is global (external). There are however a few lowercase symbols that - # are shown for special global symbols ("u", "v" and "w"). - if symtype.islower() and symtype not in "uvw": - ignored_symtypes.add(symtype) + symbol = parts[-1] + symbol = '%s (type: %s)' % (symbol, symtype) + + if symbol.startswith(ALLOWED_PREFIXES): + python_symbols.append(symbol) continue - symbol = parts[-1] - if symbol.startswith(allowed_prefixes): + if is_local_symbol_type(symtype): + local_symbols.append(symbol) + else: + smelly_symbols.append(symbol) + + if local_symbols: + print(f"Ignore {len(local_symbols)} local symbols") + return smelly_symbols, python_symbols + + +def check_library(library, dynamic=False): + nm_output = get_exported_symbols(library, dynamic) + smelly_symbols, python_symbols = get_smelly_symbols(nm_output) + + if not smelly_symbols: + print(f"OK: no smelly symbol found ({len(python_symbols)} Python symbols)") + return 0 + + print() + smelly_symbols.sort() + for symbol in smelly_symbols: + print("Smelly symbol: %s" % symbol) + + print() + print("ERROR: Found %s smelly symbols!" % len(smelly_symbols)) + return len(smelly_symbols) + + +def check_extensions(): + print(__file__) + srcdir = os.path.dirname(os.path.dirname(os.path.dirname(__file__))) + filename = os.path.join(srcdir, "pybuilddir.txt") + try: + with open(filename, encoding="utf-8") as fp: + pybuilddir = fp.readline() + except FileNotFoundError: + print(f"Cannot check extensions because {filename} does not exist") + return True + + print(f"Check extension modules from {pybuilddir} directory") + builddir = os.path.join(srcdir, pybuilddir) + nsymbol = 0 + for name in os.listdir(builddir): + if not name.endswith(".so"): + continue + if IGNORE_EXTENSION in name: + print() + print(f"Ignore extension: {name}") continue - symbol = '%s (type: %s)' % (symbol, symtype) - symbols.append(symbol) - if ignored_symtypes: - print("Ignored symbol types: %s" % ', '.join(sorted(ignored_symtypes))) print() - return symbols + filename = os.path.join(builddir, name) + nsymbol += check_library(filename, dynamic=True) + + return nsymbol def main(): - nm_output = get_exported_symbols() - symbols = get_smelly_symbols(nm_output) + # static library + LIBRARY = sysconfig.get_config_var('LIBRARY') + if not LIBRARY: + raise Exception("failed to get LIBRARY variable from sysconfig") + nsymbol = check_library(LIBRARY) + + # dynamic library + LDLIBRARY = sysconfig.get_config_var('LDLIBRARY') + if not LDLIBRARY: + raise Exception("failed to get LDLIBRARY variable from sysconfig") + if LDLIBRARY != LIBRARY: + print() + nsymbol += check_library(LDLIBRARY, dynamic=True) - if not symbols: - print("OK: no smelly symbol found") - sys.exit(0) + # Check extension modules like _ssl.cpython-310d-x86_64-linux-gnu.so + nsymbol += check_extensions() + + if nsymbol: + print() + print(f"ERROR: Found {nsymbol} smelly symbols in total!") + sys.exit(1) - symbols.sort() - for symbol in symbols: - print("Smelly symbol: %s" % symbol) print() - print("ERROR: Found %s smelly symbols!" % len(symbols)) - sys.exit(1) + print(f"OK: all exported symbols of all libraries " + f"are prefixed with {' or '.join(map(repr, ALLOWED_PREFIXES))}") if __name__ == "__main__": From 6c8b0eedc42bdf7273311fd5e71287cee78f3ade Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 24 Nov 2020 12:16:46 +0100 Subject: [PATCH 2/3] Ignore _init and _fini symbols --- Tools/scripts/smelly.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Tools/scripts/smelly.py b/Tools/scripts/smelly.py index 014f2fe51a8ff1..12c34d6fab917f 100755 --- a/Tools/scripts/smelly.py +++ b/Tools/scripts/smelly.py @@ -11,7 +11,9 @@ if sys.platform == 'darwin': ALLOWED_PREFIXES += ('__Py',) -IGNORE_EXTENSION = "_ctypes_test" +IGNORED_EXTENSION = "_ctypes_test" +# Ignore constructor and destructor functions +IGNORED_SYMBOLS = {'_init', '_fini'} def is_local_symbol_type(symtype): @@ -76,6 +78,8 @@ def get_smelly_symbols(stdout): if is_local_symbol_type(symtype): local_symbols.append(symbol) + elif symbol in IGNORED_SYMBOLS: + local_symbols.append(symbol) else: smelly_symbols.append(symbol) @@ -119,7 +123,7 @@ def check_extensions(): for name in os.listdir(builddir): if not name.endswith(".so"): continue - if IGNORE_EXTENSION in name: + if IGNORED_EXTENSION in name: print() print(f"Ignore extension: {name}") continue From 8bae949c79df8c349dcb078e1e7b3f4e9b3f4458 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 24 Nov 2020 13:02:33 +0100 Subject: [PATCH 3/3] Fix test on IGNORED_SYMBOLS --- Tools/scripts/smelly.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Tools/scripts/smelly.py b/Tools/scripts/smelly.py index 12c34d6fab917f..e8a375c808cdaa 100755 --- a/Tools/scripts/smelly.py +++ b/Tools/scripts/smelly.py @@ -70,18 +70,18 @@ def get_smelly_symbols(stdout): symtype = parts[1].strip() symbol = parts[-1] - symbol = '%s (type: %s)' % (symbol, symtype) + result = '%s (type: %s)' % (symbol, symtype) if symbol.startswith(ALLOWED_PREFIXES): - python_symbols.append(symbol) + python_symbols.append(result) continue if is_local_symbol_type(symtype): - local_symbols.append(symbol) + local_symbols.append(result) elif symbol in IGNORED_SYMBOLS: - local_symbols.append(symbol) + local_symbols.append(result) else: - smelly_symbols.append(symbol) + smelly_symbols.append(result) if local_symbols: print(f"Ignore {len(local_symbols)} local symbols")