parity: say why a runner failed, then fix the five Windows failures it was hiding - #1104
Conversation
WalkthroughChangesPlatform-aware object representations
Filesystem and Windows parity
Parity runner diagnostics
JIT diagnostics and baselines
Runtime compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dd0b49b32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| partial = expired.stderr or "" | ||
| return False, f"timed out after {TIMEOUT}s", partial |
There was a problem hiding this comment.
When a parity script exceeds the 30-second timeout after writing to stderr, TimeoutExpired.stderr is bytes even though subprocess.run was called with text=True. Passing it through here makes _report and _annotate render each line as a bytes representation such as b'partial stderr', rather than preserving the child's decoded stderr as promised. Decode the partial output with UTF-8 and replacement handling before storing it in Failure.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 24807b7). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
youknowone
changed the title
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50abf5b3de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # `ns` is split with divmod before anything is narrowed, so a nanosecond count | ||
| # too wide for a `time_t` is only refused when the SECOND it names is. | ||
| os.utime(p, ns=(2**62, 2**62)) | ||
| check(os.stat(p).st_mtime_ns == 4611686018427387900, os.stat(p).st_mtime_ns) |
There was a problem hiding this comment.
When this all-platform fixture runs on a Unix filesystem with nanosecond precision, the requested value remains exactly 2**62, not the Windows-specific 100-ns-rounded value used here. Running the fixture with CPython 3.14.4 on Linux/ext4 produced 4611686018427387904, so the reference runner fails at this assertion before any pyre backend is compared; make the expected value platform-dependent as the earlier -1ns check does.
Useful? React with 👍 / 👎.
| # Nanoseconds run out of an `int64` in 2262, and a file may carry a later time | ||
| # than that — `st_mtime_ns` is the number it is rather than a wrap. | ||
| os.utime(p, (8.8e11, 8.8e11)) | ||
| check(os.stat(p).st_mtime_ns == 880_000_000_000_000_000_000, os.stat(p).st_mtime_ns) |
There was a problem hiding this comment.
On Linux/ext4, 8.8e11 seconds is beyond the filesystem's timestamp range, so CPython 3.14.4 clamps it and reports 15032385535000000000 ns rather than this value. After the preceding precision assertion is fixed, the CPython reference will still fail here; use a post-2262 timestamp within common Unix filesystem ranges so the test isolates Python-integer widening instead of filesystem limits.
Useful? React with 👍 / 👎.
| pyre_object::PY_NULL, | ||
| ) | ||
| })?; | ||
| let cls = crate::_structseq::make_struct_seq_with_extra( |
There was a problem hiding this comment.
On Windows, every call constructs a fresh sys.getwindowsversion class, so type(sys.getwindowsversion()) is type(sys.getwindowsversion()) is false and an object from one call is not an instance of the next call's type. Move this type creation to a process-global OnceLock<usize>, as the existing stat_result and struct_time implementations do, and reuse it for every result.
AGENTS.md reference: AGENTS.md:L148-L155
Useful? React with 👍 / 👎.
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93eb41e6c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pyre_object::w_dict_store( | ||
| vars, | ||
| pyre_object::w_str_new(name), |
There was a problem hiding this comment.
When _sysconfig.config_vars() runs under nursery pressure, w_str_new(name) can trigger a moving collection after vars has been copied into the call expression, leaving w_dict_store with the dictionary's pre-collection address; the store itself can also move the dictionary before the next iteration or final return. Pin the dictionary for the whole construction and reload it from the shadow stack, as the analogous dictionary-building path in call.rs does.
Useful? React with 👍 / 👎.
| sec: n.div_euclid(1_000_000_000), | ||
| nsec: n.rem_euclid(1_000_000_000), | ||
| sec, | ||
| nsec: crate::builtins::space_index_w(w_nsec)?, |
There was a problem hiding this comment.
For a valid ns object whose __divmod__ returns user-defined quotient and remainder objects, converting w_sec can execute __index__ and trigger a moving GC. Neither the returned tuple nor w_nsec is rooted, so this subsequent conversion can dereference the remainder's stale pre-collection address; pin the tuple or both elements and reload the remainder after converting the quotient.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/extra_tests/parity_tests/os_utime_pathconf_truncate.py`:
- Around line 103-106: Update the os.utime timestamp case and its st_mtime_ns
assertion to use a filesystem-storable value just beyond the i64 nanosecond
boundary, such as 10_000_000_000 seconds, or gate the assertion on filesystem
capability; preserve validation of the resulting nanosecond timestamp without
requiring the unsupported 8.8e11-second value.
In `@pyre/extra_tests/parity_tests/run.py`:
- Around line 149-151: Update the subprocess.TimeoutExpired handling in the
timeout path to decode expired.stderr before returning it, while preserving the
empty fallback when stderr is absent. Ensure the returned Failure.stderr value
is text so downstream reports and annotations receive clean traceback lines
rather than a bytes representation.
In `@pyre/extra_tests/parity_tests/surrogate_name_messages.py`:
- Around line 31-32: Replace all remaining percent-style formatting in this test
module, including the ValueError message and the address formatting in at(),
with format-specifier syntax. Preserve the existing output text,
platform-specific width, and casing: uppercase fixed-width addresses on Windows
and lowercase addresses elsewhere.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 1377-1387: Update the sys module initialization and
getwindowsversion result construction so make_struct_seq_with_extra creates the
sys.getwindowsversion struct-sequence type once during initialization. Store
that type in the module state, then reuse it for every subsequent
sys.getwindowsversion() call instead of recreating it per result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf7f3a53-5759-4ee2-bc0c-f149dfd936d8
📒 Files selected for processing (23)
pyre/bench/synth/pypy_type_surface.cranelift.jitstatspyre/bench/synth/pypy_type_surface.dynasm.jitstatspyre/bench/synth/pypy_type_surface.wasm.jitstatspyre/extra_tests/parity_tests/frame_clear_finalization.pypyre/extra_tests/parity_tests/os_utime_pathconf_truncate.pypyre/extra_tests/parity_tests/run.pypyre/extra_tests/parity_tests/surrogate_name_messages.pypyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_contextvars/mod.rspyre/pyre-interpreter/src/module/_ctypes/cdata.rspyre/pyre-interpreter/src/module/_pickle/pickler.rspyre/pyre-interpreter/src/module/_weakref/interp__weakref.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/thread/mod.rspyre/pyre-interpreter/src/pycode.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyrex/src/lib.rs
| return "0x%0*X" % (2 * (8 if sys.maxsize > 2**32 else 4), id(obj)) | ||
| return "0x%x" % id(obj) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ruff check pyre/extra_tests/parity_tests/surrogate_name_messages.py --select UP031Repository: youknowone/pyre
Length of output: 4646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file sections and the dynamic assertions/proposed substitutions.
sed -n '1,150p' pyre/extra_tests/parity_tests/surrogate_name_messages.py | nl -ba | sed -n '1,150p'
python3 - <<'PY'
# Probe the exact current format semantics and proposed replacements for representative values.
class S:
def __init__(self, value):
self.value = value
def __repr__(self):
return f"<S({self.value:`#x`})>"
def current_at(obj):
import sys
if sys.platform == "win32":
return "0x%0*X" % (2 * (8 if sys.maxsize > 2**32 else 4), id(obj))
return "0x%x" % id(obj)
def new_at(obj):
import sys
width = 2 * (8 if sys.maxsize > 2**32 else 4)
return f"0x{id(obj):0{width}X}"
for platform, old, new in [(None, current_at, new_at), ("win32", current_at, new_at)]:
print("platform:", platform)
for obj in [object(), object()]:
print(old(obj), "==", new(obj))
PYRepository: youknowone/pyre
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -u
echo "== relevant file section =="
sed -n '1,150p' pyre/extra_tests/parity_tests/surrogate_name_messages.py
echo
echo "== semantic probe =="
python3 - <<'PY'
import sys
class S:
def __init__(self, value):
self.value = value
def __repr__(self):
return f"<S({self.value:`#x`})>"
def current_at(obj):
if sys.platform == "win32":
return "0x%0*X" % (2 * (8 if sys.maxsize > 2**32 else 4), id(obj))
return "0x%x" % id(obj)
def new_at(obj):
width = 2 * (8 if sys.maxsize > 2**32 else 4)
return f"0x{id(obj):0{width}X}"
for platform in [None, "win32"]:
print("platform:", platform)
for obj in [object(), object()]:
print(old := current_at(obj), "==", new := new_at(obj))
if old != new:
raise SystemExit("at() replacement mismatch")
PYRepository: youknowone/pyre
Length of output: 4780
Replace the remaining percent-formatted assertions with format specifiers.
Run Ruff UP031 on any leftover % formatting, including the ValueError: %s\n case, and update the assertions to keep the current output text. For at(), preserve the platform-specific width and case semantics: use an upper-case fixed-width address on Windows, and the lower-case address elsewhere.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 31-31: Use format specifiers instead of percent format
(UP031)
[warning] 32-32: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/extra_tests/parity_tests/surrogate_name_messages.py` around lines 31 -
32, Replace all remaining percent-style formatting in this test module,
including the ValueError message and the address formatting in at(), with
format-specifier syntax. Preserve the existing output text, platform-specific
width, and casing: uppercase fixed-width addresses on Windows and lowercase
addresses elsewhere.
Source: Linters/SAST tools
|
Thanks — three of the four were real, and one of them was worse than reported.
New Timed-out child's stderr: fixed. The 8.8e11-second
Both spellings now come from one f-strings in Verified after the change, with LLBC re-extracted first: |
The per-failure detail and the `N failure(s)` count went to stderr while the 213-row result table went to stdout. A piped stdout is block-buffered and stderr is not, so the whole report arrived in the log *above* the run's own header, and the last thing before the runner's non-zero exit was a passing row -- a failed job named none of what failed in it. The report is printed last now, on stdout, and echoes each child's stderr verbatim instead of a `repr` of the whole thing on one line: a traceback only reads as one when its line breaks survive. Each failing row also carries its one-line verdict beneath it, and under `GITHUB_ACTIONS` every failure emits an `::error file=` annotation carrying that verdict and the exception line, which shows on the pull request without opening the log. stdout is line-buffered so a run that dies mid-way names the scripts it got through, and pinned to UTF-8 because the report echoes a child's stderr: these scripts are largely about names no console codepage can spell, and printing one of those raised `UnicodeEncodeError` out of the runner instead of the failure it was in the middle of explaining. `_run` returns the reason and the stderr as separate values, which `Failure` carries.
`os.utime` on Windows turned away every time before 1970. The host call
takes its times as a `Duration`, which has no second below its epoch at all,
so `u64::try_from(sec)` was the whole pre-epoch range's refusal. A FILETIME
counts 100ns ticks from 1601-01-01, so shifting the epoch is what makes such
a second a positive tick count; `SetFileTime` is called here now, over the
handle `host_env::fs::open_write_with_custom_flags` opens, with the same
wrapping `__int64` arithmetic `time_t_to_FILE_TIME` is written in.
Measured against CPython 3.14 on the same host, `os.utime(p, times=(-5.0,
-6.0))` now reads back as -6_000_000_000 where it raised, and `ns=(-1, -1)`
as the -100 a FILETIME's granularity leaves.
Six more answers along the same argument, each measured against 3.14:
('a', 'b') ValueError: could not convert string to float
-> TypeError: argument must be int or float, not str
(1e30, 0) ValueError: utime: timestamp out of range
-> OverflowError: timestamp out of range for platform time_t
(2**200, 0) the same, and by the exact integer rather than through a
float that rounded the seconds it could not hold
(nan, 0) ValueError: utime: timestamp out of range
-> ValueError: Invalid value NaN (not a number)
(1,) utime: 'times' must be a tuple of two ints
-> ... must be either a tuple of two ints or None
ns=(2**80, 0) OverflowError -> written. `split_py_long_to_s_and_ns`
splits with `divmod` BEFORE it narrows anything, so a
nanosecond count too wide for a `time_t` is refused only
when the second it names is; 2**80 ns is a second that
fits. Dividing after the narrowing turned away the range.
`divmod` is also what answers for `ns=('a', 'b')`.
`os.truncate`/`os.ftruncate` on Windows read their length with a bare
`int_w`: no `__index__`, and `int too large to convert to int` where
`Py_off_t_converter` says `int too big to convert`. Both now go through
`truncate_length_w`, which is hoisted out of the unix arm and names the C
type the platform's converter names.
`st_atime_ns` and its two siblings took `sec * 1_000_000_000` in `i64`, which
runs out in 2262 -- a file dated later, which every FILETIME up to the year
30828 can be, read back as the wrap. The product is taken in `i128` and the
field is an int of whatever width it needs.
`parity_tests/os_utime_pathconf_truncate` covers all of it and no longer
skips its negative-time section on Windows; only the exact-nanosecond value
is platform-dependent there. Its `pathconf` section is now gated on the name
existing, because neither CPython nor this build carries `pathconf` on
Windows and the reference failed the script before any backend could -- which
is what made the whole script red on every Windows runner.
`is_exact_builtin_instance` (pyobject.rs:149-163) reads a null `w_class` as a second spelling of "exact builtin", beside the one where the slot holds the canonical type object, and `is_plain_int1` (listobject.rs:424-460) accepts both for `int` and for a fits-int `W_LongObject`. `walker_guard_exact_w_class` reads the slot and pins a single value, so an operand admitted under the null spelling and pinned against the canonical gets a guard its own recorded operand fails — and nothing writes the slot afterwards, so it fails on every execution without converging, one bridge per `trace_eagerness` bucket. That is the shape `try_walker_specialize_load_type_name_attr` shipped with, where the fold took its metaclass from `typedef::type`'s `gettypefor(ob_type)` fallback while the guard read the raw field. The 40-odd other call sites establish the operand carries what they pin — through `walker_exact_builtin_class`, which returns `None` on a null slot, or through `is_plain_int1` / a local `is_exact_int` — but nothing checked that they do. Measured across `bench/synth`, `bench` and `extra_tests/parity_tests`, under a probe that reported the recorded slot against the pinned value at every site: 1217 pins over 131 files, all carrying what they pin, and no site reaching the guard without a concrete operand. The null spelling is reachable — `bool`, `None`, functions, generators, iterators, sets and the itertools objects are all built with a null slot, and `SMALL_INTS` is written that way behind `WITHPREBUILTINT` — so what holds is that no admitting predicate currently pairs one with a canonical pin, not that it could not. `debug_assert!` rather than a decline: there is no live site to decline, release codegen is unchanged, and the next occurrence fails loudly instead of costing a 20x jit-stats drift that takes a baseline diff to notice.
… none `pyre/check.py (windows-latest)` on #1109 ends on a passing row and exit code 1; the three scripts it failed on are 200 lines up, in the stderr the neighbouring commit here moves. They are main's, not that PR's, and independent of each other. **`keyboard_interrupt_exit_status`.** A Win32 process has no SIGINT to die of. `app_main.py:1146-1151` restores `SIG_DFL` and calls `raise(SIGINT)`, saying the MSVC runtime then exits with `STATUS_CONTROL_C_EXIT`; measured, that pair returns through the CRT's default action and the process ends with status 3 — `signal.signal(SIGINT, SIG_DFL); signal.raise_signal(SIGINT)` under CPython exits 3 as well, while an uncaught `KeyboardInterrupt` there exits 0xC000013A. `raise` never terminates, so `terminate_by_sigint` fell through to `process::abort`, whose status 3 is what the fixture read. Windows exits with `STATUS_CONTROL_C_EXIT` directly; both callers have already finalized. **`builtin_module_loader_spec`.** Two missing names, both reached from `test.support`'s import: - `_sysconfig.config_vars()` answered an empty dict. `sysconfig._init_non_posix` SUBSCRIPTS `Py_GIL_DISABLED` and `Py_DEBUG` to spell `ABIFLAGS`, so under `os.name == 'nt'` an absent key is a `KeyError` out of the first `get_config_var` rather than the `None` the `.get()` readers take. Both are 0, which is what an empty `sys.abiflags` already says. `EXT_SUFFIX` and `SOABI`, the other two keys the call carries, name an extension ABI that `_imp.extension_suffixes()` says does not exist here, so they stay absent. - `sys.getwindowsversion` was absent and `_init_config_vars` subscripts `sys._vpath` beside it. The version is a five-field sequence with five named-only fields over it, built the way `os.stat_result` carries its `st_*_ns` extras, off `host_env::windows::get_windows_version`. Every field matches CPython 3.14 on the same host except `build`: the sequence reports kernel32's file version, because `GetVersionEx` answers with the version an unmanifested binary is shimmed to, and `platform_version` — the field that exists because of that shimming — agrees with it here instead of correcting it. **`frame_clear_finalization`** imported `resource` for one CPU-time bound at the end. It is a POSIX module, absent from CPython on Windows too, so the import took all six checks off the platform and left the reference failing beside the backends. `time.process_time` is the same measurement and is everywhere. extra_tests/parity_tests: 213/213 on both backends, bar `builtin_module_loader_spec` under a local CPython with no `test` package installed — the runner that CI uses has it and reported `cpython=OK`.
`PyUnicode_FromFormat`'s `%p` hands the pointer to the platform's own `printf`
and normalizes only the prefix — guaranteed to start with a literal `0x`
"regardless of what the platform's printf yields". The platforms disagree about
everything after it: the MSVC runtime pads to the pointer width and uppercases,
glibc does neither. So on Windows CPython reads
`<function f at 0x000001B7AF7FFCC0>` where it reads `<function f at
0x1b7af7ffcc0>` elsewhere, and Rust's `{:p}` — along with `{:?}` on a raw
pointer and a hand-written `0x{:x}` — is only ever the second spelling.
Every address-bearing repr was therefore wrong on Windows. Measured against
CPython 3.14 on the same host, fifteen kinds disagreed: function, object,
generator, coroutine, async_generator, bound and built-in method,
method-wrapper, cell, weakref, memoryview, ContextVar, Token, code, frame and
both `_thread` locks. They now go through one `display::repr_addr`, and the
shapes are identical. `_pickle`'s cyclic-object message names an address the
same way and is the one such site that is not a repr.
`surrogate_name_messages` asserted the glibc spelling against `id()`, so it was
the REFERENCE that failed it on every Windows runner — a script that fails on
CPython measures nothing, and the backends were being compared against a failing
oracle. It builds the platform's spelling now.
The frame repr's `file '...'` is left raw: `pyframe.py:849-853` interpolates
`'%s'`, and CPython's `%R` there escapes the backslashes a Windows path is full
of. That is a parity-source disagreement rather than this fix's business.
`bridges_compiled 102 -> 5` and `guard_failures 20497 -> 1011`, byte-identical on dynasm, cranelift and wasm. The recorded 102/20497 are the counters the `Cls.__name__` fold produced while its metaclass guard could not be discharged — it read the raw `w_class` slot where the fast path answers through the `gettypefor(ob_type)` fallback, so a receiver reached through that fallback got a `guard_value(NULL, type)` that failed on every execution. #1106 narrowed the fold; the baseline still names what the defect measured. Read back rather than predicted: ubuntu-24.04, macos-latest and windows-latest all report 5/1011 on main, and so does a local windows run. `retraces_compiled` joins the file because the recorder now writes it; it is 0, which is what the comparison already read for its absence.
…nd two NTFS-only assertions `sys.getwindowsversion` built its structseq type inside the call, so every answer was an instance of a different class and `type(sys.getwindowsversion()) is type(sys.getwindowsversion())` read False where CPython reads True. It was the one of pyre's ten structseq types that did not already cache its type in a `OnceLock` — `stat_result`, `terminal_size`, `uname_result`, `statvfs_result`, `waitid_result`, `times_result`, `struct_time`, `_ExceptHookArgs` and `UnraisableHookArgs` all do — and it now does the same. Measured: a probe over every structseq both runtimes carry reports the types equal for all nine others and for all of CPython's, so this constructor was the whole divergence. That the cache is a bare `usize` the GC cannot see is the established pattern rather than a new bet, and it holds: 200 `os.stat` answers dropped across forced collections plus 60MB of churn leave the cached type identical and its fields readable. `structseq_type_identity` pins the property for every structseq the host carries, and fails on the binary built before this commit naming exactly `sys.getwindowsversion`. The parity runner decoded its child's stderr on the normal path only. `subprocess.run(text=True)` decodes what `communicate()` returns; the timeout path raises with the raw chunks it had joined, which is bytes on POSIX and str on Windows. A timed-out script would have reported one `b'...\n...'` line — the unreadable shape the rest of this branch exists to remove. Two assertions in `os_utime_pathconf_truncate` could only ever have held on NTFS. The step that runs them has not reached ubuntu or macos: `check.py` runs first in that job and has been failing, so the parity suite never starts there. * `ns=(2**62, 2**62)` read back `4611686018427387900`, which is 2**62 rounded down to a FILETIME's 100ns tick. ext4 and APFS keep the nanosecond and answer `...904`. Both spellings now come from one `storable()`, which also replaces the `-1`/`-100` conditional above it. * `utime(p, (8.8e11, 8.8e11))` names the year 29880, which no filesystem but NTFS reaches — an APFS timestamp is itself an int64 of nanoseconds, so 2262 bounds it too, and ext4 stores a 34-bit second and stops in 2446. What every platform is held to is the identity the `i128` widening buys, `st_mtime_ns == int(st_mtime) * 1_000_000_000`, wherever the write succeeds; the exact value is asserted only where the second survived the round trip. The remaining review note asked for f-strings in `surrogate_name_messages`. Left alone: the file builds every expected repr with `%` formatting, including the assertions this branch did not touch, and nothing lints for it.
…ut run, and decode the report `classify` recorded `last_stderr_line` for a FAIL. This runner sets `MAJIT_STATS`, so the last thing every process writes is the JIT summary: every FAIL in the suite recorded `rc=1 Compilation time: <n>ms`, a line that names no test. The nightly report carries 118 of them, all the same. A FAIL now carries unittest's own account instead — the closing `FAILED (...)` and the `FAIL:`/`ERROR:` case headers, up to four of them and a count of the rest. An IMPORTERROR never reached unittest, so it keeps the tail. `TimeoutExpired` was caught and discarded, leaving `timeout 120s`. It carries the output the child had produced, which for a unittest module is the progress dots — the record of which case it stopped in. `text=True` decodes what `communicate()` returns, not what the timeout path raises, so the partial arrives as bytes on POSIX and str on Windows and both are accepted. The report's box-drawing killed the run with a `UnicodeEncodeError` on a console whose codepage cannot spell it, before any test ran; stdout is reconfigured the way the parity runner's is.
…tlocale that reaches the CRT The `LC_*` values came from libc under `cfg(unix)` and from a hardcoded POSIX table everywhere else, so Windows published `LC_CTYPE=0, LC_ALL=6` where the MSVC CRT numbers them `LC_ALL=0, LC_COLLATE=1, LC_CTYPE=2`. Every constant named a different category than the one it was passed to. They now come from libc there too, which carries the CRT's own numbering. `LC_MESSAGES` is a POSIX category the MSVC CRT has no counterpart for, and CPython does not define it on Windows; it is registered under `cfg(unix)`. `setlocale` was gated on `all(unix, host_env)`, so on Windows it fell to the no-libc arm and answered "C" for every name it was handed, reporting success for a locale that was not installed. `host_env::locale::setlocale` is not unix-only — it calls `libc::setlocale`, which Windows has. The gate now admits Windows, and an uninstallable name raises `locale.Error` as CPython's does. Measured against CPython 3.14 on Windows: the six category numbers agree, neither has `LC_MESSAGES`, `en_US.iso88591` raises `locale.Error` on both and `en_US.utf8` and `English_United States.1252` are accepted by both. `locale_categories` pins all three properties and fails on the prior binary.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/module/_locale/interp_locale.rs (1)
297-313: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep Windows locale readers consistent with
setlocale.This window now changes the Windows host locale, but
localeconv,strcoll, andstrxfrmstill use Unix-only host-backed branches. After a Windows caller setsLC_NUMERICorLC_COLLATE, these APIs still return fixed C-locale or bytewise results. Extend the host-backed reader paths to Windows, or keep Windows locale state fixed, and verifyrustpython_host_env::locale::localeconv_data,strcoll, andstrxfrmsupport Windows before changing cfg guards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs` around lines 297 - 313, Keep Windows behavior consistent across locale APIs: either extend the host-backed branches for localeconv_data, strcoll, and strxfrm to Windows after confirming rustpython_host_env::locale supports them, or constrain setlocale’s host-backed path so Windows locale state remains fixed. Ensure Windows calls to localeconv, strcoll, and strxfrm reflect the same state established by setlocale rather than falling back to Unix-only or bytewise behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 1377-1393: Replace the process-global OnceLock<usize> in the
getwindowsversion path with the existing interpreter-owned structseq state used
by stat_result_seq_type. Store the created type as a traced GC root, integrating
it with pin_root and walk_gc_refs as appropriate, and retrieve it through that
state before new_instance_with_extra. Do not retain the PyObjectRef as an
untraced static pointer.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs`:
- Around line 297-313: Keep Windows behavior consistent across locale APIs:
either extend the host-backed branches for localeconv_data, strcoll, and strxfrm
to Windows after confirming rustpython_host_env::locale supports them, or
constrain setlocale’s host-backed path so Windows locale state remains fixed.
Ensure Windows calls to localeconv, strcoll, and strxfrm reflect the same state
established by setlocale rather than falling back to Unix-only or bytewise
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 60e5f7c4-d874-4484-a190-31a9738c4fa1
📒 Files selected for processing (10)
pyre/bench/synth/ca_bridge_multiframe_resume_double_call.wasm.jitstatspyre/bench/synth/recursion_memo_branch.wasm.jitstatspyre/bench/synth/wasm_ca_trampoline_decline.wasm.jitstatspyre/cpython_tests/run.pypyre/extra_tests/parity_tests/locale_categories.pypyre/extra_tests/parity_tests/os_utime_pathconf_truncate.pypyre/extra_tests/parity_tests/run.pypyre/extra_tests/parity_tests/structseq_type_identity.pypyre/pyre-interpreter/src/module/_locale/interp_locale.rspyre/pyre-interpreter/src/module/sys/vm.rs
| // Built once, like [`stat_result_seq_type`]: the type is the | ||
| // answer's identity, so making a fresh one per call would leave | ||
| // `type(sys.getwindowsversion())` a different class each time. | ||
| static SEQ_TYPE: std::sync::OnceLock<usize> = std::sync::OnceLock::new(); | ||
| let cls = *SEQ_TYPE.get_or_init(|| { | ||
| crate::_structseq::make_struct_seq_with_extra( | ||
| "sys.getwindowsversion", | ||
| &["major", "minor", "build", "platform", "service_pack"], | ||
| &[ | ||
| "service_pack_major", | ||
| "service_pack_minor", | ||
| "suite_mask", | ||
| "product_type", | ||
| "platform_version", | ||
| ], | ||
| ) as usize | ||
| }) as PyObjectRef; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Store the cached structseq type in GC-aware interpreter state.
SEQ_TYPE stores a PyObjectRef as usize in a process-global OnceLock. The collector cannot trace or forward this value. A moving collection can leave cls stale before new_instance_with_extra uses it.
Create the type in the existing interpreter-owned structseq state and preserve it as a traced root. Do not keep the object pointer in an untraced static.
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
'stat_result_seq_type|make_struct_seq_with_extra|OnceLock<usize>|pin_root|walk_gc_refs' \
pyre/pyre-interpreter/srcAs per coding guidelines: “Before choosing a Rust collection or thread-local, locate the corresponding RPython/PyPy owner and preserve its storage shape and semantics.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 1377 - 1393, Replace
the process-global OnceLock<usize> in the getwindowsversion path with the
existing interpreter-owned structseq state used by stat_result_seq_type. Store
the created type as a traced GC root, integrating it with pin_root and
walk_gc_refs as appropriate, and retrieve it through that state before
new_instance_with_extra. Do not retain the PyObjectRef as an untraced static
pointer.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f0b915ec1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub(crate) fn repr_addr(addr: usize) -> String { | ||
| if cfg!(windows) { | ||
| format!("0x{addr:0width$X}", width = size_of::<usize>() * 2) | ||
| } else { |
There was a problem hiding this comment.
On Windows, this uppercase X formatter makes every affected repr diverge from PyPy. PyPy's W_Root.getaddrstring() explicitly emits a fixed-width sequence using lowercase a–f on every platform, rather than delegating to the CRT %p; therefore Windows function, generator, frame, code, object, and related reprs now use the wrong observable spelling. Use fixed-width lowercase hexadecimal to retain the upstream representation.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| ("st_atime_ns", pyre_object::w_int_new(st_atime_ns)), | ||
| ("st_mtime_ns", pyre_object::w_int_new(st_mtime_ns)), | ||
| ("st_ctime_ns", pyre_object::w_int_new(st_ctime_ns)), | ||
| ("st_atime_ns", w_time_ns(st_atime_ns)), |
There was a problem hiding this comment.
For a file dated after 2262, w_time_ns(st_atime_ns) creates a GC-managed Python long, but it is stored only in the unrooted extras vector while the following w_time_ns calls allocate additional bigint payloads and may collect. new_instance_with_extra does not pin these values until after the entire argument vector has been evaluated, so under collection pressure the first long can be reclaimed and the eventual structseq receives a stale pointer. Pin each wide value while constructing the vector, or otherwise root the complete set before any subsequent allocation.
Useful? React with 👍 / 👎.
#1106 `4555e3d76b` dropped the wasm32 arm of the self-recursive root-bridge inline and recorded what that moved: `ca_bridge_multiframe_resume_double_call` 2581 -> 2592, `wasm_ca_trampoline_decline` 404 -> 601 and `recursion_memo_branch` 4724 -> 4704. #1086 `e5eff81684` wrote all three back to their pre-#1106 values, and ubuntu has failed on them every run since. #1086 resizes 130 fixtures so pypy's side clears the measurement floor, and re-recording a resized fixture's counters is part of that. It moves 34 baseline values; 26 sit in fixtures whose `.py` it also changed. These three — and `pypy_type_surface`, restored in 958f66c — are among the eight it moved with no workload change beside them, which is a snapshot taken on a base predating #1106. Measured rather than reverted: a local wasm run reads 2592, 601 and 4704, which are #1106's figures and the ones ubuntu observes. The remaining two of those eight, `closure_per_call` 418 and `recursive_call_frame_relocation` 638, read what they already record and are left alone. The lower counts are not the better state. #1106 measured removing the decline at -20.1%/-20.7% wasm CPU on `wasm_ca_trampoline_decline`; the +197 guard failures buy that. `ca_bridge_multiframe_resume_double_call` pays +1.2%/+2.7% and in exchange reports the 16 bridges / 0 aborts the dynasm baseline records, where the decline left wasm at 16/1. `retraces_compiled=0` comes with the re-record: these were the only wasm baselines missing the key, which `_parse_jit_stats` was defaulting.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24807b7794
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pyre_object::w_tuple_new(vec![ | ||
| w_int_new(info.major as i64), | ||
| w_int_new(info.minor as i64), | ||
| w_int_new(info.build as i64), | ||
| ]), |
There was a problem hiding this comment.
On Windows, this publishes (major, minor, build) as platform_version, but the corresponding implementation in pypy/module/sys/vm.py:280-298 deliberately stores None there so platform.py uses the five primary GetVersionEx fields. Code that checks sys.getwindowsversion().platform_version therefore observes a pyre-only tuple; preserve the upstream empty value rather than introducing alternate CPython-style semantics.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
1 participant
A failed
pyre/extra_tests/parity_testsjob named none of what failed in it.This one
ends:
So the runner now says why, and then everything it was failing to say — five
scripts, red on every Windows runner — is fixed.
1. The runner reports where a CI log is read
The reason was in the log, 200 lines above the run's own header. The
per-failure detail and the
N failure(s)count went to stderr while the213-row table went to stdout, and a piped stdout is block-buffered where
stderr is not, so the report overtook everything it was meant to explain and
the last line before the non-zero exit was a passing row.
verbatim rather than a
reprof the whole thing on one line — a tracebackonly reads as one when its line breaks survive.
::error file=annotations underGITHUB_ACTIONS, one per failure,carrying the verdict and the exception line. They show on the pull request
without opening the log.
got through.
echoes a child's stderr, and these scripts are largely about names no console
codepage can spell, so printing one raised
UnicodeEncodeErrorout of therunner instead of the failure it was explaining — the same no-reason death,
one level up.
_runreturns the reason and the stderr as separate values, which the newFailuretuple carries; nothing about what counts as a pass moves.It works: the run this branch was rebased over reported its own failure as
surrogate_name_messages.py [cpython]: exited 1 … AssertionError: '<function q\udcffn at 0x000002A594587E20>', traceback and all, which is §5 below.2. The failure it was hiding
Two independent things, both Windows:
CPython has no
os.pathconfthere. The reference failed the script atline 96 before any backend could, and comparing a backend against a failing
reference measures nothing. That section is now gated on the name existing —
hasattr, not a platform list, because what it needs is the name.os.utimerefused every time before 1970. The host call takes its timesas a
Duration, which has no second below its epoch at all, sou64::try_from(sec)was the whole pre-epoch range's refusal.#1107 landed a Windows
SetFileTimepath over the same lines while thisbranch was out, and the rebase takes its structure —
CreateFileWwithFILE_WRITE_ATTRIBUTESandFILE_FLAG_BACKUP_SEMANTICS, which is whatrposix.py:1558-1575opens with and what lets the name open a directory or aread-only file. What is kept from this side is the arithmetic:
r_longlongin
rwin32file.py:320-323, and the__int64of the C it transcribes, bothwrap. A checked conversion answers a
ValueErrorfor a tick count neitherruntime produces, and by then anything no
time_tcan hold has already beenrefused with the
OverflowErrorbelow.3. What measuring it turned up
Every row below was measured against CPython 3.14 on the same host, before
and after:
utime(p, times=(-5.0, -6.0))ValueError: utime: timestamp out of rangest_mtime_ns == -6_000_000_000utime(p, ns=(-1, -1))-100— a FILETIME's granularityutime(p, ('a', 'b'))ValueError: could not convert string to floatTypeError: argument must be int or float, not strutime(p, (1e30, 0))ValueError: utime: timestamp out of rangeOverflowError: timestamp out of range for platform time_tutime(p, (2**200, 0))utime(p, (nan, 0))ValueError: utime: timestamp out of rangeValueError: Invalid value NaN (not a number)utime(p, (1,))utime: 'times' must be a tuple of two ints... must be either a tuple of two ints or Noneutime(p, ns=(2**80, 0))OverflowErrortruncate(p, 2**64)int too large to convert to intint too big to convertst_mtime_nsof a file dated 30000-5443715538058477568880000000000000000000Three of those are worth their own sentence:
ns=2**80.split_py_long_to_s_and_nssplits withdivmodbefore itnarrows anything, so a nanosecond count too wide for a
time_tis refusedonly when the second it names is — and 2**80 ns is a second that fits.
Dividing after the narrowing turned away the whole range. Going through
divmodis also what answers forns=('a', 'b').truncate. The Windows arms read their length with a bareint_w: no__index__, and the wrong message. Both now go throughtruncate_length_w,hoisted out of the unix arm, which names the C type the platform's own
Py_off_t_converternames.st_*_ns. Thesec * 1_000_000_000product was taken ini64, whichruns out in 2262 — not an exotic date for a filesystem whose FILETIME
reaches the year 30828. It is taken in
i128now and the field is an int ofwhatever width it needs.
parity_tests/os_utime_pathconf_truncatepins all of it and no longer skipsits negative-time section on Windows; only the exact-nanosecond value is
platform-dependent there.
4. The three failures it was hiding everywhere else
The same job on #1109
ends the same way — a passing row, then exit code 1 — over three more scripts,
all Windows, all main's and independent of each other:
keyboard_interrupt_exit_status. A Win32 process has no SIGINT to dieof.
app_main.pyrestoresSIG_DFLand callsraise(SIGINT)saying the MSVCruntime then exits with
STATUS_CONTROL_C_EXIT; measured, that pair returnsand the process ends with status 3 — under CPython too.
raiseneverterminating meant
terminate_by_sigintfell through toprocess::abort,whose status 3 was what the fixture read. Windows exits with
STATUS_CONTROL_C_EXITdirectly now.builtin_module_loader_spec._sysconfig.config_vars()answered an emptydict, but
sysconfig._init_non_posixsubscriptsPy_GIL_DISABLEDandPy_DEBUG, so underos.name == 'nt'an absent key is aKeyErrorand notthe
Nonethe.get()readers take. Behind it,sys._vpathandsys.getwindowsversionwere both absent — the latter now a five-fieldsequence with five named-only fields over it, off
host_env::windows::get_windows_version, matching CPython 3.14 field forfield bar
build(see the commit for why kernel32's file version is thehonest one for an unmanifested binary).
frame_clear_finalizationimportedresourcefor one CPU-time bound atthe end; it is POSIX-only, absent from CPython on Windows too, so the import
took all six checks off the platform and left the reference failing beside the
backends.
time.process_timeis the same measurement and is everywhere.5. Every repr's address was wrong on Windows
surrogate_name_messageswas red on Windows withcpython=FAIL— thereference failing it, so the backends were being compared against a failing
oracle. It asserts
"<function %s at 0x%x>" % (S, id(f)), and that is the glibcspelling.
PyUnicode_FromFormat's%phands the pointer to the platform's ownprintfand normalizes only the prefix — guaranteed to start with a literal
0x"regardless of what the platform's printf yields". Everything after it is the
platform's: the MSVC runtime pads to the pointer width and uppercases, glibc
does neither. Rust's
{:p},{:?}on a raw pointer, and a hand-written0x{:x}are all only ever the second spelling, so every address-bearingrepr was wrong there. Measured against CPython 3.14 on the same host, all of
them disagreed:
repr(f)<function f at 0x1b7af7ffcc0><function f at 0x000001B7AF7FFCC0>…and the same for object, generator, coroutine, async_generator, bound and
built-in method, method-wrapper, cell, weakref, memoryview, ContextVar, Token,
code, frame and both
_threadlocks — fifteen kinds, now through onedisplay::repr_addr, verified shape-identical._pickle's cyclic-objectmessage names an address the same way and is the one such site that is not a
repr.
The frame repr's
file '...'is left raw:pypy/interpreter/pyframe.py:851interpolates
'%s'where CPython's%Rescapes the backslashes a Windows pathis full of. That is a parity-source disagreement rather than this fix's
business.
6. And the failure that was already there
synth/pypy_type_surfaceis red onmainitself, on all three runners andbyte-identically on all three backends —
bridges_compiled 102 -> 5, guard_failures 20497 -> 1011inubuntu,
macos
and windows
— so the branch could not be green while it stood.
The recorded 102/20497 are what the
Cls.__name__fold measured while itsmetaclass guard could not be discharged: the guard reads the raw
w_classslot, the fast path answers through
typedef::type'sgettypefor(ob_type)fallback, and a receiver reached through that fallback got a
guard_value(NULL, type)that failed on every execution and that nothing laterdischarged. Taking the fixture apart puts every one of the 97 excess bridges in
check_descriptor_kinds, which readstype(type.__dict__[name]).__name__; thegetset_descriptortype object is the one class that reaches it, built lazilyfrom inside the init loop so it never enters the registry the post-loop sweep
walks to stamp the slot.
#1106 landed the fold's narrowing while this branch was rebasing, so the
cherry-pick dropped out; what is left is the baseline, re-recorded here from
what the three runners and a local windows run all report rather than from a
prediction.
Also left of it is a
debug_asserton the helper:is_exact_builtin_instanceand
is_plain_int1both read a nullw_classas a second spelling of "exactbuiltin", so an operand admitted under that spelling and pinned against the
canonical gets a guard its own recorded operand fails. Measured across
bench/synth,benchandparity_tests— 1217 pins over 131 files — everysite already establishes what it pins, and nothing checked that they did.
7. The review round
sys.getwindowsversionbuilt its structseq type inside the call, sotype(sys.getwindowsversion()) is type(sys.getwindowsversion())read Falsewhere CPython reads True. It was the one of pyre's ten structseq types not
already caching in a
OnceLock; a probe over every structseq both runtimescarry reports the types equal for all nine others and for all of CPython's,
so this constructor was the whole divergence.
structseq_type_identitypinsthe property and fails on the binary built before the fix.
subprocess.run(text=True)decodeswhat
communicate()returns; the timeout path raises with the raw chunks ithad joined. Reporting those would have printed one
b'...\n...'line — theshape §1 exists to remove.
utimeassertions could only hold on NTFS, and the reason neitherwas caught is that the parity step has never reached ubuntu or macos:
check.pyruns first in that job and has been failing, so the suite neverstarts.
ns=(2**62, 2**62)expected 2**62 rounded to a FILETIME tick, and(8.8e11, 8.8e11)names the year 29880 — an APFS timestamp is itself anint64 of nanoseconds so 2262 bounds it too, and ext4 stops in 2446. Both go
through one
storable()now, and the far date is held to the identity thei128widening buys wherever the write succeeds.Verification
Rebased onto
a7eb493079; re-extracted LLBC before every measurement a staleimage could have fabricated.
extra_tests/parity_testson Windows — 214/214 on both backends, barbuiltin_module_loader_specunder a local CPython with notestpackageinstalled; CI's reference has it and reported
cpython=OK. It was 208/213before, five scripts red on every Windows runner.
pyre/check.py— 410/410 on dynasm and 410/410 on cranelift, bothall-green.
synth/type_name_attr_foldstill reads its own5/0/1/4, sojit(wasm): drop the wasm32 arm of the self-recursive root-bridge inline #1106's decline is narrow enough that the fold keeps applying wherever its
guard holds.
cargo test --all --no-default-features --features dynasm, and the craneliftsubset CI re-runs after it — both clean.
cargo fmt --all -- --checkclean.utime/truncate/statshapes,diffed line by line against CPython 3.14 — 0 differences.
Summary by CodeRabbit
New Features
sys.getwindowsversion(), locale operations, and platform-specific filesystem behavior.Bug Fixes
Tests