jit, stdlib: PR 1187 review follow-ups; cpython_tests: per-host baseline overlay - #1196
Conversation
`module.__init__` read its name argument through `w_str_get_value`, which
panics on a lone surrogate, so `types.ModuleType('\udcff')` aborted the
process. The import machinery reaches it whenever a filename was decoded with
surrogateescape: `test.test_import.test_unencodable_filename` imports
`TESTFN_UNENCODABLE`, which on Linux is the surrogateescape of byte 0xff. The
module never ran on darwin, where the kernel rejects such a filename outright
(EILSEQ), so the crash showed only on the Linux CPython suite leg.
Store `Module.name` as `Wtf8Buf` rather than `String` -- the same buffer type
`W_UnicodeObject.value` already uses -- and read the argument with
`w_str_get_wtf8`. `w_module_new*` keep taking `&str`, so their callers are
unchanged. `release_frees_nothing` answers `false` for a name that is not
valid UTF-8, since such a name cannot key the `&str` `sys.modules` lookup.
Add `tools/ubuntu24-arm64-repro`, an Apple `container` image that reproduces
Linux-only failures natively on Apple silicon rather than through Rosetta.
Assisted-by: Claude
`run.py` now consults `baseline.<sys.platform>-<machine>.json` ahead of `baseline.json`, and `--update-baseline` splits what it records between the two: a verdict the shared file has never seen is written there, a verdict that differs from the shared one goes to the host overlay, and a host that comes back into agreement drops its overlay entry. The architecture is part of the key because the dynasm backend emits different machine code on each one. Drops `CPYTHON_SUITE_BASELINE_HOST` from `check.py`, which skipped the cpython-suite stage anywhere other than darwin-arm64. Assisted-by: Claude
The module-scope LOAD_NAME builtins fold declined only when `module_dict_cell_slot_direct` found the name in the frame's live globals. That call also answers `None` for a dict it cannot read -- a plain dict, or a module dict that switched to the object strategy -- so on any dict other than the namespace operand its `None` proved nothing, while the fold guarded the operand's version and the residual it replaced read the live dict. Decline unless the two are the same dict. BLAKE2 rejected an oversized salt or person in `_blake2_new`, which runs after the app-level wrapper has range-checked fanout and depth, so `blake2b(salt=b'x' * 17, fanout=256)` reported the fanout. `lib_pypy/_blake2/__init__.py` sets salt and person before the tree parameters and the key after them; validate in that order, measuring `Py_buffer.len` rather than `len()` so a memoryview over a wider itemsize still counts bytes. `_PyModule_ClearDict`'s two name passes were ordering-inert: rebinding a name to `None` frees nothing without refcounting, so `_obj.__del__` read its module's public globals as `None`. Sweep between the passes, once for the whole walk rather than once per module. Add a collection after the walk releases its module roots, which is the only point a value stored under a non-string key becomes unreachable. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 12 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 (10)
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: 26f6272025
ℹ️ 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".
| clear_pass(ShutdownClearPass::PrivateNames); | ||
| collect_and_run_finalizers(ec_ptr); |
There was a problem hiding this comment.
When a private-name finalizer in a newer module reads a private global from an older module, this whole-walk private pass clears the older module's global before running any finalizers. CPython's newest-first module teardown runs that finalizer while the older module is still intact, so a value such as older._secret should remain available rather than becoming None; run the private pass and collection with equivalent per-module ordering instead of batching every module together.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| for source in (overlay, baseline): | ||
| entry = source.get("modules", {}).get(module) | ||
| if entry is None: | ||
| continue | ||
| status = entry.get(backend) or entry.get("dynasm") |
There was a problem hiding this comment.
When a host overlay has only a dynasm override but the shared baseline has an explicit cranelift verdict, a cranelift run returns the overlay's dynasm fallback before consulting the shared cranelift entry. This can deselect a shared-PASS cranelift module or gate it against the wrong status; resolve exact backend entries across overlay and shared baseline first, and only then apply the dynasm fallback.
Useful? React with 👍 / 👎.
| # Salt and person are rejected before the tree parameters and the | ||
| # key after them, the order lib_pypy/_blake2 sets each field in. | ||
| # `blake2b(salt=b'x' * 17, fanout=256)` reports the salt. | ||
| if _buffer_size(salt) > cls.SALT_SIZE: |
There was a problem hiding this comment.
When key is not buffer-compatible and salt is oversized, this new early salt check raises the salt ValueError, whereas CPython converts the earlier key argument first and raises its bytes-like-object TypeError. Before this change _blake2_new likewise called read_hash_buffer(key) before inspecting salt, so the commit introduces an observable error-order regression; acquire or otherwise validate the key buffer before these salt/person size checks.
Useful? React with 👍 / 👎.
| Inside the container: | ||
|
|
||
| ```bash | ||
| scripts/install-charon.sh |
There was a problem hiding this comment.
The first documented in-container command fails immediately because the repository contains scripts/install-charon.py, not scripts/install-charon.sh (confirmed with a repository-wide file search), and the image does not create a shell wrapper. Use python3 scripts/install-charon.py so the new reproduction procedure is executable.
AGENTS.md reference: AGENTS.md:L62-L65
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 26f6272). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
`#1196` changed `Module.name` to a `Wtf8Buf` box and routed `module.__init__` through `w_str_get_wtf8`, so a name carrying a lone surrogate no longer panics. Nothing pins that: the two existing accessor tests only pass an ASCII name, which a `String` box would have held just as well. Round-trip a name with a lone surrogate through `w_module_set_name`/`w_module_get_name` and assert it has no `&str` view. Assisted-by: Claude
`#1196` changed `Module.name` to a `Wtf8Buf` box and routed `module.__init__` through `w_str_get_wtf8`, so a name carrying a lone surrogate no longer panics. Nothing pins that: the two existing accessor tests only pass an ASCII name, which a `String` box would have held just as well. Round-trip a name with a lone surrogate through `w_module_set_name`/`w_module_get_name` and assert it has no `&str` view. Assisted-by: Claude
`pyre/check.py (windows-latest)` fails 18 rows across 10 fixtures — 9 on each native backend — where ubuntu-24.04 and macos-latest both pass. Every failing row is a jit-stats difference; no output snapshot mismatches, so the fixtures still compute the same results there. Values transcribed from the windows job of run 31724482401 (main b0f34c0). Transcription is exact rather than sampled: check.py states that "the recorded surface and the gated surface are the same set", so a FAIL line enumerates every counter that differs and each unnamed counter equals the shared baseline. Each file was cross-checked against the `(observed loops_compiled=N bridges_compiled=M)` parenthetical the same line prints. The three runners were read back before adding these, as the overlay comment requires: at that sha ubuntu reports these rows green (its own failures are cranelift/str_fstring and wasm/exception_traceback_loop_forms) and macos-latest is `success` for the whole job. The divergence appeared with the #1189 squash, but the branch alone does not produce it: that PR's own last windows run, at head 22ac8c9, failed only str_fstring on both backends. Its CI merged into d953ddc, while the squash landed on that plus #1184, #1196 and #1174; main at df365f9 carries those three without the branch and also lacks these rows. So it is an interaction between the two sides, and which pair is responsible is not established here. One caveat for whoever maintains these: inline_chain_depth_typeflip's windows observation already moved once, 3843 -> 3798, between the squash and b0f34c0. The other eight fixtures reported identical numbers across both runs. Assisted-by: Claude
* gc: root every SRE group selector before slicing * gc: preserve GIL across sandbox heap dumps * gc: end action borrow before yielding GIL * gc: make async ticker signal-safe * gc: root the process signal action * gc: root the pairwise iteration state across space.next `next`'s `itertools.pairwise` arm held `self` and `w_prev` in raw Rust locals across two `space.next` calls. A minor collection inside either call forwards the object but not the local, so the field stores and the returned tuple could name pre-collection addresses. The arm now claims four shadow-stack slots — self, iterator, w_prev, w_next — before the first call and reloads each from its slot. The indices are fixed rather than derived from how many roots the taken arm happened to push, so a slot means the same thing on both paths; the two slots that start without a value hold null, which the root walkers already read as "no root". `interp_itertools` gains the field accessors that arm reads and writes through. The setter runs the write barrier, because `W_Pairwise` is allocated old-gen and an iterator may yield a nursery object. The `W_Pairwise` unit test now asserts the GC descriptor's pointer offsets cover `w_iterator` and `w_prev`, not just the object size. Assisted-by: Claude * bench: record the win32 runner jitstats overlays windows-latest reports `pyre/check.py (windows-latest)` fails 18 rows across 10 fixtures — 9 on each native backend — where ubuntu-24.04 and macos-latest both pass. Every failing row is a jit-stats difference; no output snapshot mismatches, so the fixtures still compute the same results there. Values transcribed from the windows job of run 31724482401 (main b0f34c0). Transcription is exact rather than sampled: check.py states that "the recorded surface and the gated surface are the same set", so a FAIL line enumerates every counter that differs and each unnamed counter equals the shared baseline. Each file was cross-checked against the `(observed loops_compiled=N bridges_compiled=M)` parenthetical the same line prints. The three runners were read back before adding these, as the overlay comment requires: at that sha ubuntu reports these rows green (its own failures are cranelift/str_fstring and wasm/exception_traceback_loop_forms) and macos-latest is `success` for the whole job. The divergence appeared with the #1189 squash, but the branch alone does not produce it: that PR's own last windows run, at head 22ac8c9, failed only str_fstring on both backends. Its CI merged into d953ddc, while the squash landed on that plus #1184, #1196 and #1174; main at df365f9 carries those three without the branch and also lacks these rows. So it is an interaction between the two sides, and which pair is responsible is not established here. One caveat for whoever maintains these: inline_chain_depth_typeflip's windows observation already moved once, 3843 -> 3798, between the squash and b0f34c0. The other eight fixtures reported identical numbers across both runs. Assisted-by: Claude * Revert "bench: record the win32 runner jitstats overlays windows-latest reports" An overlay records what a runner observes; it does not change what the runner observes. The 18 files pinned the windows-latest numbers for those rows so the gate would stop reporting them, leaving the divergence itself in place. The pre-existing `str_fstring.cranelift.win32.github-actions.jitstats` is not part of this and stays. `pyre/check.py (windows-latest)` therefore still reports the 18 rows. Assisted-by: Claude
1 participant
Follow-ups to the review findings on #1187, plus the per-host baseline the
CPython suite needs to gate on more than one machine.
Review findings
Each was reproduced against the built binary first; the probe output is below.
Module-scope LOAD_NAME builtins fold specialised against a dict it could not
read. The fold declined only when
module_dict_cell_slot_directfound thename in the frame's live globals, but that call also answers
Nonefor a dictit cannot read at all — a plain dict, or a module dict that ran
switch_to_object_strategy. On any dict other than the namespace operand itsNoneproved nothing, while the fold guarded the operand's version and theresidual it replaced resolved the live dict, so a name present there would read
the builtin instead of the global.
guard_current_frame_globals_identitydoesnot close the gap: it constrains the frame's globals, not the dict the residual
resolves, and the two differ when
frame.pycode != w_code_ptr. Declines nowunless the two are the same dict.
BLAKE2 reported the wrong argument error. The salt/person/key size checks
ran in
_blake2_new, after the app-level wrapper had range-checked fanout anddepth.
lib_pypy/_blake2/__init__.pysets salt and person before the treeparameters and the key after them; the checks moved to those positions and
measure
Py_buffer.lenrather thanlen(), so a memoryview over a wideritemsize still counts bytes.
blake2b(salt=b'x'*17, fanout=256)fanout must be between 0 and 255maximum salt length is 16 bytesmaximum salt length is 16 bytesblake2b(person=b'x'*17, fanout=256)fanout must be between 0 and 255maximum person length is 16 bytesmaximum person length is 16 bytesblake2b(salt=b'x'*17, depth=256)depth must be between 1 and 255maximum salt length is 16 bytesmaximum salt length is 16 bytesAll six probe cases now match both reference interpreters.
_PyModule_ClearDict's two name passes were ordering-inert. Rebinding aname to
Nonefrees nothing without refcounting, so a finalizer released bythe private-name pass read its module's public globals as
Nonewhere CPythonshows them alive. A sweep now runs between the passes — once for the whole
walk, not once per module, which measured 905ms of teardown for
import unittest, re, argparseand timedtest_regrtestout. A furthercollection runs after the walk releases its module roots: that is the only
point a value stored under a non-string key becomes unreachable, and without it
its
__del__never ran.__del__under a non-string module keyNONSTR DEL RANNONSTR DEL RANNonepublic-value-alivepublic-value-aliveTeardown for
import unittest, re, argparsegoes 38.7ms -> 48.7ms for the twoadded collections.
Two further suggestions were declined: rejecting oversized BLAKE2 buffers
before copying is already the effect of measuring
nbytesapp-level, and theruff cleanup of
bench/synth/load_name_builtin_cell_fold.pywould churn a filewhose module-scope
lenrebinding is the fixture's subject and whose jit-statsbaselines are recorded against it, for a linter this repository does not run.
Per-host baseline
run.pyconsultsbaseline.<sys.platform>-<machine>.jsonahead ofbaseline.json, and--update-baselinesplits what it records: a verdict theshared file has never seen is written there, a verdict that differs goes to the
host overlay, and a host that comes back into agreement drops its overlay entry
again. No host is privileged — whichever records first sets the shared answer.
The architecture is part of the key because the dynasm backend emits different
machine code on each. This retires
CPYTHON_SUITE_BASELINE_HOST, which skippedthe
check.pystage anywhere but darwin-arm64.Gates
pyre/check.py --backend dynasm --synthetic-only— ALL PASSED, 408/408cargo fmt --all -- --check— clean🤖 Generated with Claude Code