gate brake both directions, and the 108 opcodes behind the codewriter catch-all - #1109
Merged
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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: 6c0c1ad97b
ℹ️ 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".
| /// | ||
| /// A form belonging to the other language costs nothing: `env::var(` cannot | ||
| /// occur in Python, nor `environ.get(` in Rust. | ||
| const READ_FORMS: [&str; 4] = ["env::var", "host_os::var", "getenv", "environ.get"]; |
There was a problem hiding this comment.
When harness code uses ordinary single-quoted syntax such as os.environ.get('PYRE_FOO') or os.getenv('PYRE_FOO'), the newly added read form is found, but gates_read_by rejects the argument because it only strips a double quote. An undocumented single-quoted gate therefore leaves both sets unchanged and passes the brake, while documenting it causes the reverse-direction test to report it as stale. Accept and test both Python string delimiters.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 21a6425). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
The brake reached only Rust. Six gates are read from `check.py`, `check_synthetic.py`, the `extra_tests` runners and `scripts/llbc_extract.py` and from no `.rs` file at all, so every census so far — including §6's — missed all six. `READ_FORMS` gains `environ.get`, and the walk gains `pyre/` and `scripts/`. Only unambiguous reads count. The harness also writes gates into a child's environment, and a subscript cannot be told from a read without parsing; writing a gate for a child is not owning it, and the child's read is in Rust. The widened census reported exactly two names undocumented, and both turned out to be documented already: `gate-triage.md` wrote `PYRE_SYNTH_PYRE` and `PYRE_SYNTH_PYTHON` in its run-on shorthand, as `_PYRE` and `_PYTHON`, which a whole-token match cannot resolve. They are spelled out, and §6 now asks for every name in full at least once. Section granularity could not express a mixed section. §1d's heading reads *Parity verdicts*, so it counts as live, while its table marks `PYRE_FBW_VABLE_SCALAR_CA` **RETIRED** — a reader for that gate coming back would have passed the brake on its own obituary. Any row saying "retired" now documents nothing, wherever it sits, with an assertion pinning that gate out. Counts after the widening: 111 names read (105 from `*.rs`, 6 only from the harness), 137 (file, name) pairs, 113 documented live, 0 undocumented. F5's debt drops to one name, `PYRE_FBW_REC_UNROLL`: the other two it listed are recorded as retired in prose inside live-reading sections, which the per-row rule now honours. `PYRE_CLASS_DESCRIPTORS` is added to §2. It is the only `PYRE_*` name outside Rust and Python source, and it is a linkme distributed slice, not an env var. Assisted-by: Claude
`_other => emit_abort_permanent!(py_pc)` covered every `Instruction` variant the walk's dispatch does not name. Measured against the enum: 227 variants, 119 named, 108 in the catch-all — 82 adaptive specializations, 21 `Instrumented*` forms, and 5 interpreter/JIT-internal opcodes (`EnterExecutor`, `InterpreterExit`, the two `JumpBackward` forms, `Reserved`). Replace it with those three arms, each declining exactly as before. The match is then exhaustive over `Instruction`, so a variant added upstream fails to compile here instead of declining a loop with no record of which opcode did it. `flowspace/flowcontext.rs` classifies the same three groups. Behaviour is unchanged: same `emit_abort_permanent!` call, same arms, only the patterns are spelled out. `cargo check -p pyre-jit --features dynasm` passes without the catch-all; dropping one pattern from the list fails with `E0004: non-exhaustive patterns: Instruction::ToBoolBool not covered`. Assisted-by: Claude
…REC_UNROLL The brake only checked one direction: a read with no live entry. A live entry whose reader was deleted stayed green. `PYRE_FBW_REC_UNROLL` sat in §5's config-switch list from 2026-07-06, when PR#374 (`9a97c47f6e9`) deleted `fbw_unroll_bound()`; its successor knob `PYRE_FBW_REC_UNROLL_DEPTH` was never listed and lost its reader too (PR#887, `e5546b2ed36`). Move it to §1c and add `every_live_gate_triage_entry_still_has_a_reader`, which reports the difference the other way and points at §1c rather than at deleting the row. The document writes `PYRE_*` and `PYRE_FBW_*` for families, and the token scan stops at the `*`; a documented token ending in `_` is a wildcard stem, not a name. With those two set aside the two sets are equal at 111. Counts corrected in the summary table: §1c 10 -> 11, not-gates 11 -> 12 (§2's own heading already said 12), config switches ~18 -> ~17. rework.md: F5 is closed and its section deleted, with the sweep's two costly lessons kept in Settled. F4 rewritten to what the catch-all measurement showed — the unlisted surface was opcodes this compiler cannot emit, so what remains of F4 is the eight unported opcodes. Assisted-by: Claude
`snapshot_map_from_trace_snapshots` called its `SnapshotFramePcs` local `pc_map`, and three of its five call sites bound the result as `snapshot_pc_map`. `pc_map` is a live name elsewhere in the tree: in `pyre-jit/src/jit/codewriter.rs` it is the `-live-` marker table keyed by Python pc (`pc_map[py_pc]`). This one is keyed by snapshot id and holds one `(jitcode_index, pc, py_pc)` per frame. Rename to `frame_pcs_map`, and the three call sites to `snapshot_frame_pcs`, which is what the other two already used and what the field they feed is called. cargo test -p pyre-jit --features dynasm: 33 passed, 0 failed. Assisted-by: Claude
…ion it The reader-direction test failed on this base: `PYRE_CARRIER_EXC_RESUME` is recorded retired in §5 with its reader (`carrier_exc_resume_enabled`) deleted, but §1e — a live section — discusses the seam that gate used to guard and names it three more times without the word "retired" on those lines. Skipping retirement *lines* is not enough when the same gate is written elsewhere. Retirement is a property of the gate, not of the line: collect the retired names, then drop them from the live set wherever else they appear. Only the first name on a retirement line is its subject. §1c's row for PYRE_AUTHORITATIVE ends "`PYRE_PROBE_AUTHORITATIVE` is separate and remains live" — the clause exists precisely because the second name is not the one being retired, and retiring every name on the line would have taken that gate and PYRE_JIT with it. Both directions are empty on this base: 110 read, 110 documented live. Anchored on PYRE_CARRIER_EXC_RESUME alongside the three anchors already there. Assisted-by: Claude
Owner
Author
|
Rebased onto
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21a64251e0
ℹ️ 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".
| /// | ||
| /// A form belonging to the other language costs nothing: `env::var(` cannot | ||
| /// occur in Python, nor `environ.get(` in Rust. | ||
| const READ_FORMS: [&str; 4] = ["env::var", "host_os::var", "getenv", "environ.get"]; |
There was a problem hiding this comment.
When harness code reads a gate with ordinary mapping syntax such as os.environ["PYRE_NEW"], gates_read_by ignores it because only getenv and environ.get are recognized. An undocumented gate using this form therefore passes the forward brake, while documenting it makes the reverse-direction test call it stale. Parse Python subscripts sufficiently to distinguish load contexts from assignments/deletions so direct reads are included.
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 8, 2026
… 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`.
youknowone
added a commit
that referenced
this pull request
Aug 8, 2026
… 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`.
youknowone
added a commit
that referenced
this pull request
Aug 9, 2026
… 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`.
youknowone
added a commit
that referenced
this pull request
Aug 9, 2026
…t was hiding (#1104) * extra_tests: report the parity runner's failures where a CI log is read 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. * posix: the pre-epoch utime Windows refused, and six answers beside it `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. * jit: assert `guard_exact_w_class` pins a `w_class` its operand carries `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. * sys, pyrex: the three Windows parity failures behind a job that named 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`. * display: spell a repr's address the way the platform's `%p` spells it `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. * bench/synth: re-record pypy_type_surface's jit-stats baseline `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. * sys, parity: a structseq type built once, a decoded timeout stderr, and 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. * cpython_tests: report the cases unittest named, the tail of a timed-out 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. * _locale: the Windows category numbers, no LC_MESSAGES there, and a setlocale 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. * bench/synth: restore the three wasm jit-stats baselines #1086 reverted #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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #1105, which merged while these were being written. Four commits: two
close
rework.md's F5, one re-measures and shrinks F4, one renames a local whosename is taken elsewhere in the tree.
487d825— the brake now scans the harness Pythongate_triage_complete.rswalked only workspace-member Rust. Addingenviron.getto
READ_FORMSandpyre/+scripts/to the walk made six gates visiblethat no
.rsfile reads —PYRE_CHECK_PYPY3,PYRE_CHECK_PYTHON3,PYRE_SHARED_BUILD,PYRE_SYNTH_PYPY,PYRE_SYNTH_PYRE,PYRE_SYNTH_PYTHON.Every census to date, including §6's own, had missed all six.
An allow-list, not a tree walk: the repository vendors ~4600 upstream
.pyfilesunder
lib-python/,pypy/,rpython/,lib_pypy/, and untracked scratch sitsat the root. Only unambiguous reads count — the harness also writes gates into
a child's environment (
env[…] = …), a subscript cannot be told from a readwithout parsing, and writing a gate for a child is not owning it.
Two names came back undocumented and both turned out to be documented all along:
gate-triage.mdwrote them as_PYRE,_PYTHONin its run-on shorthand, whichwhole-token matching cannot resolve. They are spelled out now, and §6 carries the
rule. A census can only find what the document is willing to say.
Also from #1105's review: §1d's heading reads Parity verdicts, so the section
counts as live, yet its table marks
PYRE_FBW_VABLE_SCALAR_CARETIRED — amixed section that section-level granularity cannot express. A row that says
"retired" now documents nothing wherever it sits, anchored on that gate.
fb9f53f— the catch-all was not what the finding said_other => emit_abort_permanent!(py_pc)was recorded inrework.mdas "anopcode nobody has looked at". Measured against the enum: 227
Instructionvariants, 119 named by the dispatch, 108 in the catch-all — and all 108
are opcodes this compiler cannot emit.
BinaryOpAddInt,LoadAttrSlot,ToBoolBool, …)Instrumented*(sys.monitoring)EnterExecutor,InterpreterExit, bothJumpBackwardforms,Reserved)pyre's eval loop does not quicken — nothing calls
replace_opoutside acorruption test — so the specializations cannot reach the walk, and
deoptimize()is deliberately not called on them: a stream carrying aspecialization this interpreter never wrote is corrupt, not something to fold
back. So the unlisted cliff was never a live cliff. What mattered was the
silence: had one appeared, the loop declined with no record of which opcode did
it.
Classifying all 108 and deleting
_othermakes the match exhaustive, so avariant added upstream fails to compile here instead of vanishing into a
catch-all.
majit-translate'sflowspace/flowcontext.rsalready classifies thesame three groups; this brings the walker in line with it.
Behaviour is unchanged — same arms, same macro, only the patterns are spelled
out. The coverage claim is load-bearing rather than decorative: dropping one
pattern fails with
error[E0004]: non-exhaustive patterns: Instruction::ToBoolBool not covered.4b4ef3b— the brake's other direction, and one retirementThe brake failed on a read with no live entry. A live entry whose reader was
deleted stayed green, and that is the quieter half: it survives the code it
describes, and the next sweep spends its time re-deriving that the name is
already gone.
PYRE_FBW_REC_UNROLLsat in §5's config-switch list from2026-07-06, when PR#374 (
9a97c47f6e9) deletedfbw_unroll_bound(); itssuccessor knob
PYRE_FBW_REC_UNROLL_DEPTHwas never listed at all and lost itsreader too (PR#887,
e5546b2ed36).every_live_gate_triage_entry_still_has_a_readerreports the difference theother way, and points at §1c rather than at deleting the row — deleting it loses
why the gate existed. Planting a live row for a name nothing reads fails the test
by name, which is what keeps it from passing vacuously.
The document writes
PYRE_*andPYRE_FBW_*for families and the token scanstops at the
*; a documented token ending in_is a wildcard stem, not aname. With those set aside the two sets are equal at 111.
6c0c1ad—pc_mapnames something else in this treesnapshot_map_from_trace_snapshotscalled itsSnapshotFramePcslocalpc_map,and three of five call sites bound it as
snapshot_pc_map. Inpyre-jit/src/jit/codewriter.rs,pc_mapis the live-live-marker tablekeyed by Python pc (
pc_map[py_pc]). This one is keyed by snapshot id and holdsone
(jitcode_index, pc, py_pc)per frame. Renamed toframe_pcs_map/snapshot_frame_pcs, which is what the other two call sites already used.Documents
rework.md: F5 deleted (closed), its two costly lessons kept in Settled; F4rewritten to what the measurement showed, so what remains of it is the eight
unported opcodes.
gate-triage.md: §1c 10 → 11, not-gates 11 → 12 (§2's headingalready said 12), config switches ~18 → ~17.
Verification
cargo test -p pyre-jit --features dynasm33 passed · brake 3 passed ·cargo fmt --all -- --checkrc=0 ·check.py --backend dynasm404 passed, 1failed — the one failure being
synth/pypy_type_surface, which is main's own(#1099 names it) and reproduces without either commit.
Two local-only failures appeared first and were not real:
synth/make_function_inlinewrong output andsynth/del_cellvar_walk_commitjit-stats. Both came from extracting one crate's LLBC —
extract-llbc.py pyre-jitleavespyre-object/pyre-interpreterat their previous base, andthis branch's base had moved. Re-extracting the whole set cleared both. CI never
sees that mixture because it extracts every crate fresh, which is exactly what
made the failure look like a regression CI was missing.
These numbers were measured before the rebase onto
0b1dee0; re-measurement onthe new base is running.
🤖 Generated with Claude Code