From 9e6493849ec2389460a8933314fcc8b7be01c221 Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Wed, 24 Sep 2025 02:52:24 +0100 Subject: [PATCH 1/3] GH-139174: Prepare `pathlib.Path.info` for new methods (#139175) Merge `_WindowsPathInfo` and `_PosixPathInfo` classes into a new `_StatResultInfo` class. On Windows, this means relying on `os.stat()` rather than `os.path.isfile()` and friends, which is a little slower. But there's value in making the code easier to maintain, and we're going to need the stat result for implementing `size()`, `mode()` etc. Also move the classes from `pathlib._os` to `pathlib` proper. --- Lib/pathlib/__init__.py | 260 +++++++++++++++++++++++++++++++++++-- Lib/pathlib/_os.py | 279 ---------------------------------------- 2 files changed, 251 insertions(+), 288 deletions(-) diff --git a/Lib/pathlib/__init__.py b/Lib/pathlib/__init__.py index bc39a30c6538ce..8a892102cc00ea 100644 --- a/Lib/pathlib/__init__.py +++ b/Lib/pathlib/__init__.py @@ -14,7 +14,9 @@ from errno import * from glob import _StringGlobber, _no_recurse_symlinks from itertools import chain -from stat import S_ISDIR, S_ISREG, S_ISSOCK, S_ISBLK, S_ISCHR, S_ISFIFO +from stat import ( + S_IMODE, S_ISDIR, S_ISREG, S_ISLNK, S_ISSOCK, S_ISBLK, S_ISCHR, S_ISFIFO, +) from _collections_abc import Sequence try: @@ -27,10 +29,9 @@ grp = None from pathlib._os import ( - PathInfo, DirEntryInfo, vfsopen, vfspath, ensure_different_files, ensure_distinct_paths, - copyfile2, copyfileobj, copy_info, + copyfile2, copyfileobj, ) @@ -612,6 +613,247 @@ class PureWindowsPath(PurePath): __slots__ = () +class _Info: + __slots__ = ('_path',) + + def __init__(self, path): + self._path = path + + def __repr__(self): + path_type = "WindowsPath" if os.name == "nt" else "PosixPath" + return f"<{path_type}.info>" + + def _stat(self, *, follow_symlinks=True): + """Return the status as an os.stat_result.""" + raise NotImplementedError + + def _posix_permissions(self, *, follow_symlinks=True): + """Return the POSIX file permissions.""" + return S_IMODE(self._stat(follow_symlinks=follow_symlinks).st_mode) + + def _file_id(self, *, follow_symlinks=True): + """Returns the identifier of the file.""" + st = self._stat(follow_symlinks=follow_symlinks) + return st.st_dev, st.st_ino + + def _access_time_ns(self, *, follow_symlinks=True): + """Return the access time in nanoseconds.""" + return self._stat(follow_symlinks=follow_symlinks).st_atime_ns + + def _mod_time_ns(self, *, follow_symlinks=True): + """Return the modify time in nanoseconds.""" + return self._stat(follow_symlinks=follow_symlinks).st_mtime_ns + + if hasattr(os.stat_result, 'st_flags'): + def _bsd_flags(self, *, follow_symlinks=True): + """Return the flags.""" + return self._stat(follow_symlinks=follow_symlinks).st_flags + + if hasattr(os, 'listxattr'): + def _xattrs(self, *, follow_symlinks=True): + """Return the xattrs as a list of (attr, value) pairs, or an empty + list if extended attributes aren't supported.""" + try: + return [ + (attr, os.getxattr(self._path, attr, follow_symlinks=follow_symlinks)) + for attr in os.listxattr(self._path, follow_symlinks=follow_symlinks)] + except OSError as err: + if err.errno not in (EPERM, ENOTSUP, ENODATA, EINVAL, EACCES): + raise + return [] + + +_STAT_RESULT_ERROR = [] # falsy sentinel indicating stat() failed. + + +class _StatResultInfo(_Info): + """Implementation of pathlib.types.PathInfo that provides status + information by querying a wrapped os.stat_result object. Don't try to + construct it yourself.""" + __slots__ = ('_stat_result', '_lstat_result') + + def __init__(self, path): + super().__init__(path) + self._stat_result = None + self._lstat_result = None + + def _stat(self, *, follow_symlinks=True): + """Return the status as an os.stat_result.""" + if follow_symlinks: + if not self._stat_result: + try: + self._stat_result = os.stat(self._path) + except (OSError, ValueError): + self._stat_result = _STAT_RESULT_ERROR + raise + return self._stat_result + else: + if not self._lstat_result: + try: + self._lstat_result = os.lstat(self._path) + except (OSError, ValueError): + self._lstat_result = _STAT_RESULT_ERROR + raise + return self._lstat_result + + def exists(self, *, follow_symlinks=True): + """Whether this path exists.""" + if follow_symlinks: + if self._stat_result is _STAT_RESULT_ERROR: + return False + else: + if self._lstat_result is _STAT_RESULT_ERROR: + return False + try: + self._stat(follow_symlinks=follow_symlinks) + except (OSError, ValueError): + return False + return True + + def is_dir(self, *, follow_symlinks=True): + """Whether this path is a directory.""" + if follow_symlinks: + if self._stat_result is _STAT_RESULT_ERROR: + return False + else: + if self._lstat_result is _STAT_RESULT_ERROR: + return False + try: + st = self._stat(follow_symlinks=follow_symlinks) + except (OSError, ValueError): + return False + return S_ISDIR(st.st_mode) + + def is_file(self, *, follow_symlinks=True): + """Whether this path is a regular file.""" + if follow_symlinks: + if self._stat_result is _STAT_RESULT_ERROR: + return False + else: + if self._lstat_result is _STAT_RESULT_ERROR: + return False + try: + st = self._stat(follow_symlinks=follow_symlinks) + except (OSError, ValueError): + return False + return S_ISREG(st.st_mode) + + def is_symlink(self): + """Whether this path is a symbolic link.""" + if self._lstat_result is _STAT_RESULT_ERROR: + return False + try: + st = self._stat(follow_symlinks=False) + except (OSError, ValueError): + return False + return S_ISLNK(st.st_mode) + + +class _DirEntryInfo(_Info): + """Implementation of pathlib.types.PathInfo that provides status + information by querying a wrapped os.DirEntry object. Don't try to + construct it yourself.""" + __slots__ = ('_entry',) + + def __init__(self, entry): + super().__init__(entry.path) + self._entry = entry + + def _stat(self, *, follow_symlinks=True): + """Return the status as an os.stat_result.""" + return self._entry.stat(follow_symlinks=follow_symlinks) + + def exists(self, *, follow_symlinks=True): + """Whether this path exists.""" + if not follow_symlinks: + return True + try: + self._stat(follow_symlinks=follow_symlinks) + except OSError: + return False + return True + + def is_dir(self, *, follow_symlinks=True): + """Whether this path is a directory.""" + try: + return self._entry.is_dir(follow_symlinks=follow_symlinks) + except OSError: + return False + + def is_file(self, *, follow_symlinks=True): + """Whether this path is a regular file.""" + try: + return self._entry.is_file(follow_symlinks=follow_symlinks) + except OSError: + return False + + def is_symlink(self): + """Whether this path is a symbolic link.""" + try: + return self._entry.is_symlink() + except OSError: + return False + + +def _copy_info(info, target, follow_symlinks=True): + """Copy metadata from the given PathInfo to the given local path.""" + copy_times_ns = ( + hasattr(info, '_access_time_ns') and + hasattr(info, '_mod_time_ns') and + (follow_symlinks or os.utime in os.supports_follow_symlinks)) + if copy_times_ns: + t0 = info._access_time_ns(follow_symlinks=follow_symlinks) + t1 = info._mod_time_ns(follow_symlinks=follow_symlinks) + os.utime(target, ns=(t0, t1), follow_symlinks=follow_symlinks) + + # We must copy extended attributes before the file is (potentially) + # chmod()'ed read-only, otherwise setxattr() will error with -EACCES. + copy_xattrs = ( + hasattr(info, '_xattrs') and + hasattr(os, 'setxattr') and + (follow_symlinks or os.setxattr in os.supports_follow_symlinks)) + if copy_xattrs: + xattrs = info._xattrs(follow_symlinks=follow_symlinks) + for attr, value in xattrs: + try: + os.setxattr(target, attr, value, follow_symlinks=follow_symlinks) + except OSError as e: + if e.errno not in (EPERM, ENOTSUP, ENODATA, EINVAL, EACCES): + raise + + copy_posix_permissions = ( + hasattr(info, '_posix_permissions') and + (follow_symlinks or os.chmod in os.supports_follow_symlinks)) + if copy_posix_permissions: + posix_permissions = info._posix_permissions(follow_symlinks=follow_symlinks) + try: + os.chmod(target, posix_permissions, follow_symlinks=follow_symlinks) + except NotImplementedError: + # if we got a NotImplementedError, it's because + # * follow_symlinks=False, + # * lchown() is unavailable, and + # * either + # * fchownat() is unavailable or + # * fchownat() doesn't implement AT_SYMLINK_NOFOLLOW. + # (it returned ENOSUP.) + # therefore we're out of options--we simply cannot chown the + # symlink. give up, suppress the error. + # (which is what shutil always did in this circumstance.) + pass + + copy_bsd_flags = ( + hasattr(info, '_bsd_flags') and + hasattr(os, 'chflags') and + (follow_symlinks or os.chflags in os.supports_follow_symlinks)) + if copy_bsd_flags: + bsd_flags = info._bsd_flags(follow_symlinks=follow_symlinks) + try: + os.chflags(target, bsd_flags, follow_symlinks=follow_symlinks) + except OSError as why: + if why.errno not in (EOPNOTSUPP, ENOTSUP): + raise + + class Path(PurePath): """PurePath subclass that can make system calls. @@ -637,7 +879,7 @@ def info(self): try: return self._info except AttributeError: - self._info = PathInfo(self) + self._info = _StatResultInfo(str(self)) return self._info def stat(self, *, follow_symlinks=True): @@ -817,7 +1059,7 @@ def _filter_trailing_slash(self, paths): def _from_dir_entry(self, dir_entry, path_str): path = self.with_segments(path_str) path._str = path_str - path._info = DirEntryInfo(dir_entry) + path._info = _DirEntryInfo(dir_entry) return path def iterdir(self): @@ -1123,7 +1365,7 @@ def _copy_from(self, source, follow_symlinks=True, preserve_metadata=False): self.joinpath(child.name)._copy_from( child, follow_symlinks, preserve_metadata) if preserve_metadata: - copy_info(source.info, self) + _copy_info(source.info, self) else: self._copy_from_file(source, preserve_metadata) @@ -1133,7 +1375,7 @@ def _copy_from_file(self, source, preserve_metadata=False): with open(self, 'wb') as target_f: copyfileobj(source_f, target_f) if preserve_metadata: - copy_info(source.info, self) + _copy_info(source.info, self) if copyfile2: # Use fast OS routine for local file copying where available. @@ -1155,12 +1397,12 @@ def _copy_from_file(self, source, preserve_metadata=False): def _copy_from_symlink(self, source, preserve_metadata=False): os.symlink(vfspath(source.readlink()), self, source.info.is_dir()) if preserve_metadata: - copy_info(source.info, self, follow_symlinks=False) + _copy_info(source.info, self, follow_symlinks=False) else: def _copy_from_symlink(self, source, preserve_metadata=False): os.symlink(vfspath(source.readlink()), self) if preserve_metadata: - copy_info(source.info, self, follow_symlinks=False) + _copy_info(source.info, self, follow_symlinks=False) def move(self, target): """ diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py index 6508a9bca0d72b..79a1969d5f83d6 100644 --- a/Lib/pathlib/_os.py +++ b/Lib/pathlib/_os.py @@ -4,7 +4,6 @@ from errno import * from io import TextIOWrapper, text_encoding -from stat import S_ISDIR, S_ISREG, S_ISLNK, S_IMODE import os import sys try: @@ -302,281 +301,3 @@ def ensure_different_files(source, target): err.filename = vfspath(source) err.filename2 = vfspath(target) raise err - - -def copy_info(info, target, follow_symlinks=True): - """Copy metadata from the given PathInfo to the given local path.""" - copy_times_ns = ( - hasattr(info, '_access_time_ns') and - hasattr(info, '_mod_time_ns') and - (follow_symlinks or os.utime in os.supports_follow_symlinks)) - if copy_times_ns: - t0 = info._access_time_ns(follow_symlinks=follow_symlinks) - t1 = info._mod_time_ns(follow_symlinks=follow_symlinks) - os.utime(target, ns=(t0, t1), follow_symlinks=follow_symlinks) - - # We must copy extended attributes before the file is (potentially) - # chmod()'ed read-only, otherwise setxattr() will error with -EACCES. - copy_xattrs = ( - hasattr(info, '_xattrs') and - hasattr(os, 'setxattr') and - (follow_symlinks or os.setxattr in os.supports_follow_symlinks)) - if copy_xattrs: - xattrs = info._xattrs(follow_symlinks=follow_symlinks) - for attr, value in xattrs: - try: - os.setxattr(target, attr, value, follow_symlinks=follow_symlinks) - except OSError as e: - if e.errno not in (EPERM, ENOTSUP, ENODATA, EINVAL, EACCES): - raise - - copy_posix_permissions = ( - hasattr(info, '_posix_permissions') and - (follow_symlinks or os.chmod in os.supports_follow_symlinks)) - if copy_posix_permissions: - posix_permissions = info._posix_permissions(follow_symlinks=follow_symlinks) - try: - os.chmod(target, posix_permissions, follow_symlinks=follow_symlinks) - except NotImplementedError: - # if we got a NotImplementedError, it's because - # * follow_symlinks=False, - # * lchown() is unavailable, and - # * either - # * fchownat() is unavailable or - # * fchownat() doesn't implement AT_SYMLINK_NOFOLLOW. - # (it returned ENOSUP.) - # therefore we're out of options--we simply cannot chown the - # symlink. give up, suppress the error. - # (which is what shutil always did in this circumstance.) - pass - - copy_bsd_flags = ( - hasattr(info, '_bsd_flags') and - hasattr(os, 'chflags') and - (follow_symlinks or os.chflags in os.supports_follow_symlinks)) - if copy_bsd_flags: - bsd_flags = info._bsd_flags(follow_symlinks=follow_symlinks) - try: - os.chflags(target, bsd_flags, follow_symlinks=follow_symlinks) - except OSError as why: - if why.errno not in (EOPNOTSUPP, ENOTSUP): - raise - - -class _PathInfoBase: - __slots__ = ('_path', '_stat_result', '_lstat_result') - - def __init__(self, path): - self._path = str(path) - - def __repr__(self): - path_type = "WindowsPath" if os.name == "nt" else "PosixPath" - return f"<{path_type}.info>" - - def _stat(self, *, follow_symlinks=True, ignore_errors=False): - """Return the status as an os.stat_result, or None if stat() fails and - ignore_errors is true.""" - if follow_symlinks: - try: - result = self._stat_result - except AttributeError: - pass - else: - if ignore_errors or result is not None: - return result - try: - self._stat_result = os.stat(self._path) - except (OSError, ValueError): - self._stat_result = None - if not ignore_errors: - raise - return self._stat_result - else: - try: - result = self._lstat_result - except AttributeError: - pass - else: - if ignore_errors or result is not None: - return result - try: - self._lstat_result = os.lstat(self._path) - except (OSError, ValueError): - self._lstat_result = None - if not ignore_errors: - raise - return self._lstat_result - - def _posix_permissions(self, *, follow_symlinks=True): - """Return the POSIX file permissions.""" - return S_IMODE(self._stat(follow_symlinks=follow_symlinks).st_mode) - - def _file_id(self, *, follow_symlinks=True): - """Returns the identifier of the file.""" - st = self._stat(follow_symlinks=follow_symlinks) - return st.st_dev, st.st_ino - - def _access_time_ns(self, *, follow_symlinks=True): - """Return the access time in nanoseconds.""" - return self._stat(follow_symlinks=follow_symlinks).st_atime_ns - - def _mod_time_ns(self, *, follow_symlinks=True): - """Return the modify time in nanoseconds.""" - return self._stat(follow_symlinks=follow_symlinks).st_mtime_ns - - if hasattr(os.stat_result, 'st_flags'): - def _bsd_flags(self, *, follow_symlinks=True): - """Return the flags.""" - return self._stat(follow_symlinks=follow_symlinks).st_flags - - if hasattr(os, 'listxattr'): - def _xattrs(self, *, follow_symlinks=True): - """Return the xattrs as a list of (attr, value) pairs, or an empty - list if extended attributes aren't supported.""" - try: - return [ - (attr, os.getxattr(self._path, attr, follow_symlinks=follow_symlinks)) - for attr in os.listxattr(self._path, follow_symlinks=follow_symlinks)] - except OSError as err: - if err.errno not in (EPERM, ENOTSUP, ENODATA, EINVAL, EACCES): - raise - return [] - - -class _WindowsPathInfo(_PathInfoBase): - """Implementation of pathlib.types.PathInfo that provides status - information for Windows paths. Don't try to construct it yourself.""" - __slots__ = ('_exists', '_is_dir', '_is_file', '_is_symlink') - - def exists(self, *, follow_symlinks=True): - """Whether this path exists.""" - if not follow_symlinks and self.is_symlink(): - return True - try: - return self._exists - except AttributeError: - if os.path.exists(self._path): - self._exists = True - return True - else: - self._exists = self._is_dir = self._is_file = False - return False - - def is_dir(self, *, follow_symlinks=True): - """Whether this path is a directory.""" - if not follow_symlinks and self.is_symlink(): - return False - try: - return self._is_dir - except AttributeError: - if os.path.isdir(self._path): - self._is_dir = self._exists = True - return True - else: - self._is_dir = False - return False - - def is_file(self, *, follow_symlinks=True): - """Whether this path is a regular file.""" - if not follow_symlinks and self.is_symlink(): - return False - try: - return self._is_file - except AttributeError: - if os.path.isfile(self._path): - self._is_file = self._exists = True - return True - else: - self._is_file = False - return False - - def is_symlink(self): - """Whether this path is a symbolic link.""" - try: - return self._is_symlink - except AttributeError: - self._is_symlink = os.path.islink(self._path) - return self._is_symlink - - -class _PosixPathInfo(_PathInfoBase): - """Implementation of pathlib.types.PathInfo that provides status - information for POSIX paths. Don't try to construct it yourself.""" - __slots__ = () - - def exists(self, *, follow_symlinks=True): - """Whether this path exists.""" - st = self._stat(follow_symlinks=follow_symlinks, ignore_errors=True) - if st is None: - return False - return True - - def is_dir(self, *, follow_symlinks=True): - """Whether this path is a directory.""" - st = self._stat(follow_symlinks=follow_symlinks, ignore_errors=True) - if st is None: - return False - return S_ISDIR(st.st_mode) - - def is_file(self, *, follow_symlinks=True): - """Whether this path is a regular file.""" - st = self._stat(follow_symlinks=follow_symlinks, ignore_errors=True) - if st is None: - return False - return S_ISREG(st.st_mode) - - def is_symlink(self): - """Whether this path is a symbolic link.""" - st = self._stat(follow_symlinks=False, ignore_errors=True) - if st is None: - return False - return S_ISLNK(st.st_mode) - - -PathInfo = _WindowsPathInfo if os.name == 'nt' else _PosixPathInfo - - -class DirEntryInfo(_PathInfoBase): - """Implementation of pathlib.types.PathInfo that provides status - information by querying a wrapped os.DirEntry object. Don't try to - construct it yourself.""" - __slots__ = ('_entry',) - - def __init__(self, entry): - super().__init__(entry.path) - self._entry = entry - - def _stat(self, *, follow_symlinks=True, ignore_errors=False): - try: - return self._entry.stat(follow_symlinks=follow_symlinks) - except OSError: - if not ignore_errors: - raise - return None - - def exists(self, *, follow_symlinks=True): - """Whether this path exists.""" - if not follow_symlinks: - return True - return self._stat(ignore_errors=True) is not None - - def is_dir(self, *, follow_symlinks=True): - """Whether this path is a directory.""" - try: - return self._entry.is_dir(follow_symlinks=follow_symlinks) - except OSError: - return False - - def is_file(self, *, follow_symlinks=True): - """Whether this path is a regular file.""" - try: - return self._entry.is_file(follow_symlinks=follow_symlinks) - except OSError: - return False - - def is_symlink(self): - """Whether this path is a symbolic link.""" - try: - return self._entry.is_symlink() - except OSError: - return False From c8624cd36746b17d8f991cde63705e9419e940de Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Wed, 24 Sep 2025 05:46:05 +0200 Subject: [PATCH 2/3] gh-138860: Lazy import rlcompleter in pdb to avoid deadlock in subprocess (#139185) --- Lib/pdb.py | 15 +++++++++---- Lib/test/test_pdb.py | 22 +++++++++++++++++++ ...-09-20-17-50-31.gh-issue-138860.Y9JXap.rst | 1 + 3 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-09-20-17-50-31.gh-issue-138860.Y9JXap.rst diff --git a/Lib/pdb.py b/Lib/pdb.py index a783583a2b1c38..fd48882e28fe7c 100644 --- a/Lib/pdb.py +++ b/Lib/pdb.py @@ -100,7 +100,6 @@ import _pyrepl.utils from contextlib import ExitStack, closing, contextmanager -from rlcompleter import Completer from types import CodeType from warnings import deprecated @@ -364,6 +363,15 @@ def __init__(self, completekey='tab', stdin=None, stdout=None, skip=None, readline.set_completer_delims(' \t\n`@#%^&*()=+[{]}\\|;:\'",<>?') except ImportError: pass + + # GH-138860 + # We need to lazy-import rlcompleter to avoid deadlock + # We cannot import it during self.complete* methods because importing + # rlcompleter for the first time will overwrite readline's completer + # So we import it here and save the Completer class + from rlcompleter import Completer + self.RlCompleter = Completer + self.allow_kbdint = False self.nosigint = nosigint # Consider these characters as part of the command so when the users type @@ -1186,10 +1194,9 @@ def completedefault(self, text, line, begidx, endidx): conv_vars = self.curframe.f_globals.get('__pdb_convenience_variables', {}) return [f"${name}" for name in conv_vars if name.startswith(text[1:])] - # Use rlcompleter to do the completion state = 0 matches = [] - completer = Completer(self.curframe.f_globals | self.curframe.f_locals) + completer = self.RlCompleter(self.curframe.f_globals | self.curframe.f_locals) while (match := completer.complete(text, state)) is not None: matches.append(match) state += 1 @@ -1204,8 +1211,8 @@ def _enable_rlcompleter(self, ns): return try: + completer = self.RlCompleter(ns) old_completer = readline.get_completer() - completer = Completer(ns) readline.set_completer(completer.complete) yield finally: diff --git a/Lib/test/test_pdb.py b/Lib/test/test_pdb.py index 6b74e21ad73d1a..9a7d855003551a 100644 --- a/Lib/test/test_pdb.py +++ b/Lib/test/test_pdb.py @@ -4688,6 +4688,28 @@ def foo(): stdout, _ = self._run_script(script, commands) self.assertIn("42", stdout) + def test_readline_not_imported(self): + """GH-138860 + Directly or indirectly importing readline might deadlock a subprocess + if it's launched with process_group=0 or preexec_fn=setpgrp + + It's also a pattern that readline is never imported with just import pdb. + + This test is to ensure that readline is not imported for import pdb. + It's possible that we have a good reason to do that in the future. + """ + + script = textwrap.dedent(""" + import sys + import pdb + if "readline" in sys.modules: + print("readline imported") + """) + commands = "" + stdout, stderr = self._run_script(script, commands) + self.assertNotIn("readline imported", stdout) + self.assertEqual(stderr, "") + @support.force_colorized_test_class class PdbTestColorize(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Library/2025-09-20-17-50-31.gh-issue-138860.Y9JXap.rst b/Misc/NEWS.d/next/Library/2025-09-20-17-50-31.gh-issue-138860.Y9JXap.rst new file mode 100644 index 00000000000000..0903eb71ae4346 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-09-20-17-50-31.gh-issue-138860.Y9JXap.rst @@ -0,0 +1 @@ +Lazy import :mod:`rlcompleter` in :mod:`pdb` to avoid deadlock in subprocess. From c4f21d7c7c415a85a975fb878a1e578c12969d82 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 24 Sep 2025 14:19:17 +0900 Subject: [PATCH 3/3] gh-133171: Re-enable JUMP_BACKWARD to free-threading build (gh-137800) --- .github/workflows/jit.yml | 56 +++++++++++++++--------------- Include/internal/pycore_stackref.h | 6 ++++ Python/bytecodes.c | 5 +-- Python/generated_cases.c.h | 5 +-- Python/optimizer.c | 4 +++ configure | 3 +- configure.ac | 2 +- 7 files changed, 47 insertions(+), 34 deletions(-) diff --git a/.github/workflows/jit.yml b/.github/workflows/jit.yml index 52f7d0d2b3df95..80e4ae603a2614 100644 --- a/.github/workflows/jit.yml +++ b/.github/workflows/jit.yml @@ -134,6 +134,34 @@ jobs: make all --jobs 4 ./python -m test --multiprocess 0 --timeout 4500 --verbose2 --verbose3 + jit-with-disabled-gil: + name: Free-Threaded (Debug) + needs: interpreter + runs-on: ubuntu-24.04 + timeout-minutes: 90 + strategy: + fail-fast: false + matrix: + llvm: + - 19 + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + - name: Build with JIT enabled and GIL disabled + run: | + sudo bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)" ./llvm.sh ${{ matrix.llvm }} + export PATH="$(llvm-config-${{ matrix.llvm }} --bindir):$PATH" + ./configure --enable-experimental-jit --with-pydebug --disable-gil + make all --jobs 4 + - name: Run tests + run: | + ./python -m test --multiprocess 0 --timeout 4500 --verbose2 --verbose3 + continue-on-error: true + no-opt-jit: name: JIT without optimizations (Debug) needs: interpreter @@ -160,31 +188,3 @@ jobs: - name: Run tests without optimizations run: | PYTHON_UOPS_OPTIMIZE=0 ./python -m test --multiprocess 0 --timeout 4500 --verbose2 --verbose3 - - # XXX: GH-133171 - # jit-with-disabled-gil: - # name: Free-Threaded (Debug) - # needs: interpreter - # runs-on: ubuntu-24.04 - # timeout-minutes: 90 - # strategy: - # fail-fast: false - # matrix: - # llvm: - # - 19 - # steps: - # - uses: actions/checkout@v4 - # with: - # persist-credentials: false - # - uses: actions/setup-python@v5 - # with: - # python-version: '3.11' - # - name: Build with JIT enabled and GIL disabled - # run: | - # sudo bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)" ./llvm.sh ${{ matrix.llvm }} - # export PATH="$(llvm-config-${{ matrix.llvm }} --bindir):$PATH" - # ./configure --enable-experimental-jit --with-pydebug --disable-gil - # make all --jobs 4 - # - name: Run tests - # run: | - # ./python -m test --multiprocess 0 --timeout 4500 --verbose2 --verbose3 diff --git a/Include/internal/pycore_stackref.h b/Include/internal/pycore_stackref.h index c4e8f10fe05276..062834368bcd29 100644 --- a/Include/internal/pycore_stackref.h +++ b/Include/internal/pycore_stackref.h @@ -464,6 +464,12 @@ PyStackRef_CLOSE_SPECIALIZED(_PyStackRef ref, destructor destruct) PyStackRef_CLOSE(ref); } +static inline int +PyStackRef_RefcountOnObject(_PyStackRef ref) +{ + return (ref.bits & Py_TAG_REFCNT) == 0; +} + static inline _PyStackRef PyStackRef_DUP(_PyStackRef stackref) { diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 9b993188fb73c7..f9f14322df0a5e 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2940,9 +2940,10 @@ dummy_func( }; tier1 op(_SPECIALIZE_JUMP_BACKWARD, (--)) { - #if ENABLE_SPECIALIZATION + #if ENABLE_SPECIALIZATION_FT if (this_instr->op.code == JUMP_BACKWARD) { - this_instr->op.code = tstate->interp->jit ? JUMP_BACKWARD_JIT : JUMP_BACKWARD_NO_JIT; + uint8_t desired = tstate->interp->jit ? JUMP_BACKWARD_JIT : JUMP_BACKWARD_NO_JIT; + FT_ATOMIC_STORE_UINT8_RELAXED(this_instr->op.code, desired); // Need to re-dispatch so the warmup counter isn't off by one: next_instr = this_instr; DISPATCH_SAME_OPARG(); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index e33d15f2e51e16..79328a7b725613 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -7589,9 +7589,10 @@ /* Skip 1 cache entry */ // _SPECIALIZE_JUMP_BACKWARD { - #if ENABLE_SPECIALIZATION + #if ENABLE_SPECIALIZATION_FT if (this_instr->op.code == JUMP_BACKWARD) { - this_instr->op.code = tstate->interp->jit ? JUMP_BACKWARD_JIT : JUMP_BACKWARD_NO_JIT; + uint8_t desired = tstate->interp->jit ? JUMP_BACKWARD_JIT : JUMP_BACKWARD_NO_JIT; + FT_ATOMIC_STORE_UINT8_RELAXED(this_instr->op.code, desired); next_instr = this_instr; DISPATCH_SAME_OPARG(); } diff --git a/Python/optimizer.c b/Python/optimizer.c index 53f1500f3989a4..7b76cddeabff44 100644 --- a/Python/optimizer.c +++ b/Python/optimizer.c @@ -119,6 +119,7 @@ _PyOptimizer_Optimize( PyInterpreterState *interp = _PyInterpreterState_GET(); assert(interp->jit); assert(!interp->compiling); +#ifndef Py_GIL_DISABLED interp->compiling = true; // The first executor in a chain and the MAX_CHAIN_DEPTH'th executor *must* // make progress in order to avoid infinite loops or excessively-long @@ -160,6 +161,9 @@ _PyOptimizer_Optimize( assert((*executor_ptr)->vm_data.valid); interp->compiling = false; return 1; +#else + return 0; +#endif } static _PyExecutorObject * diff --git a/configure b/configure index cd8f2f19c0b92c..7624cbf0d2ae3d 100755 --- a/configure +++ b/configure @@ -10891,7 +10891,8 @@ printf "%s\n" "$tier2_flags $jit_flags" >&6; } if test "$disable_gil" = "yes" -a "$enable_experimental_jit" != "no"; then # GH-133171: This configuration builds the JIT but never actually uses it, # which is surprising (and strictly worse than not building it at all): - as_fn_error $? "--enable-experimental-jit cannot be used with --disable-gil." "$LINENO" 5 + { printf "%s\n" "$as_me:${as_lineno-$LINENO}: WARNING: --enable-experimental-jit does not work correctly with --disable-gil." >&5 +printf "%s\n" "$as_me: WARNING: --enable-experimental-jit does not work correctly with --disable-gil." >&2;} fi case "$ac_cv_cc_name" in diff --git a/configure.ac b/configure.ac index 8312dc55084333..7a7e32d42945b9 100644 --- a/configure.ac +++ b/configure.ac @@ -2799,7 +2799,7 @@ AC_MSG_RESULT([$tier2_flags $jit_flags]) if test "$disable_gil" = "yes" -a "$enable_experimental_jit" != "no"; then # GH-133171: This configuration builds the JIT but never actually uses it, # which is surprising (and strictly worse than not building it at all): - AC_MSG_ERROR([--enable-experimental-jit cannot be used with --disable-gil.]) + AC_MSG_WARN([--enable-experimental-jit does not work correctly with --disable-gil.]) fi case "$ac_cv_cc_name" in