Repository navigation
jit: gate the module-dict strategy read on the dict actually being one - #985
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 |
`W_ModuleDictObject.mstrategy` sits at the same offset as `W_DictObject`'s
`dstrategy`, which is a `&'static dyn DictStrategy` fat pointer, so an
unchecked `w_module_dict_get_strategy` on a plain dict returns one half of that
pair. Every strategy singleton is a zero-sized static, so the result is non-null
and points into read-only memory: `.is_null()` cannot reject it, and the caller
goes on to read — or atomically write — past the static.
`exec("for i in range(200000):\n x = i\n", {})` reached this through the new
quasi-immut force check and died with SIGBUS in `QuasiImmutField::invalidate`,
a Mutex compare-and-swap 36 bytes past `EMPTY_KWARGS_DICT_STRATEGY`.
`exec(src, {})` and `types.FunctionType(code, {})` both hand the frame a plain
`W_DictObject` for globals.
- `w_module_dict_strategy_or_null` is `w_module_dict_get_strategy` with the
`is_module_dict` test, for callers that have not proven the layout.
- All three walker sites take it: the force check
(`try_walker_force_quasi_immut_namespace_write`), the MAKE_FUNCTION fold and
`walker_pin_namespace_version`. The latter two predate this branch and their
`.is_null()` tests now mean what their comments claim.
- `global_store_plain_dict_globals` covers a hot STORE_GLOBAL / DELETE_GLOBAL
under `exec(src, {})` and under `FunctionType(code, {})`, plus a module-scope
loop so the plain-dict decline cannot be mistaken for "never traced".
Also in this pass, from the same review round:
- The `w_locals` module-scope gate applies to STORE_NAME only. STORE_NAME writes
`get_or_create_w_locals`, so an ABSENT `w_locals` is a fresh throwaway mapping
rather than globals and the fold must decline; STORE_GLOBAL names globals
outright and its frame's `w_locals` is legitimately null, so the same gate
there declined every function-scope global store (`store_global_hot`
0.09s → 14x gate).
- `store_bump_prediction_matches_the_write` pins `store_would_bump_version`
against `write_cell` over all six slot/value shapes.
- Drop the `PyreHelperKind` "keep this at the enum tail" note: serde encodes the
positional variant index, nothing decodes the tag numerically, and `descrs.bin`
is written and read within one build whose codegen cache is keyed on the file.
check.py: dynasm 2 failed / 367 passed, cranelift 2 failed / 367 passed —
`exception_args_virtual` and `list_length_hint_validate`, both in main's own
failure set at a72d9cb (CI run 30738774634).
Assisted-by: Claude
![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: e1e3d0dcb8
ℹ️ 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".
| // fold set the module cell. STORE_GLOBAL names globals outright, and its | ||
| // frame is a function frame whose `w_locals` is legitimately null, so the | ||
| // gate must not apply to it. | ||
| if helper == majit_ir::PyreHelperKind::StoreName { |
There was a problem hiding this comment.
When an inlined non-portal callee executes STORE_GLOBAL after a bridge/blackhole resume, the register-form frame operand can still identify the outer caller—codewriter.rs:14704-14709 explicitly documents the equivalent cross-module namespace collapse. Bypassing the locals gate for every StoreGlobal now lets this fold derive w_globals from that shared caller frame and eagerly mutate the caller's IntMutableCell; in particular, a caller with materialized locals was declined by the previous gate but can now update the wrong module's same-named global. Thread the callee's red frame into this specialization rather than enabling the fold from the shared frame anchor.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit e1e3d0d). 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)None. 4. Structural adaptations
|
Follow-up to #977, from its review round. The first item is a crash.
The crash
W_ModuleDictObject.mstrategysits at the same byte offset asW_DictObject'sdstrategy, which is a&'static dyn DictStrategy— a fat pointer. So anunchecked
w_module_dict_get_strategyon a plain dict returns one half of thatpair. Every strategy singleton is a zero-sized static, so the result is non-null
and points into read-only memory: a caller testing
.is_null()to mean "not amodule dict" gets
false, and then reads — or atomically writes — past thestatic.
dies with SIGBUS in
QuasiImmutField::invalidate, on the parking_lot Mutexcompare-and-swap, 36 bytes past
EMPTY_KWARGS_DICT_STRATEGYin__TEXT.__const.exec(src, {})andtypes.FunctionType(code, {})both hand the frame a plainW_DictObjectfor globals —exec_or_evalpassesglobals_argthroughverbatim — and #977's force check read the strategy off it.
w_module_dict_strategy_or_nullisw_module_dict_get_strategyplus theis_module_dicttest, for callers that have not proven the layout. All threewalker sites take it. Two of them predate #977 — the MAKE_FUNCTION fold and
walker_pin_namespace_version— and their.is_null()tests now mean whattheir comments claim instead of being unconditionally false.
Declining is also the orthodox answer, not a heuristic: a plain dict has no
version?field, sohook_setfield(rclass.py:714-718) emits nojit_force_quasi_immutablefor its write and there is nothing to abandon thetrace for.
global_store_plain_dict_globalscovers a hot STORE_GLOBAL / DELETE_GLOBALunder
exec(src, {})and underFunctionType(code, {}), plus a module-scopeloop so the plain-dict decline cannot be mistaken for "never traced".
The
w_localsgateThe module-scope gate now applies to STORE_NAME only.
STORE_NAME writes
get_or_create_w_locals, so an ABSENTw_localsis a freshthrowaway mapping rather than globals, and a fold that set the module cell would
write the wrong dict — the LOAD fold's null arm is correct (
load_name_valuereally does fall through to globals) but the store's was not.
STORE_GLOBAL names globals outright and its frame is a function frame whose
w_localsis legitimately null, so the same gate applied there declined everyfunction-scope global store. That showed up as
store_global_hot0.09s againsta 14x ratio gate on both native backends, which is how the over-application was
caught.
Two smaller items
store_bump_prediction_matches_the_writepinsstore_would_bump_versionagainst
write_cellacross all six slot/value shapes. The two were splitapart in jit: install the quasi-immutable mutate field at the record, and abort the walk at the namespace write #977 precisely so the tracer's prediction could not drift from the
write it predicts; this is the test that holds that.
PyreHelperKind"keep this at the enum tail" note. Serde derivesserialize_unit_variantwith the positional variant index, nothing decodesthe tag numerically, and the one bincode artefact carrying it (
descrs.bin)is written and read back within a single build whose codegen cache is
content-keyed on that file. The note was also attached to a variant that has
not been the tail since jit: fold the immutable-type STORE_ATTR/DELETE_ATTR raise to a traced inline exception #815.
Verification
pyre/check.py, all three backends:exception_args_virtualandlist_length_hint_validatefail identically inmain's own CI at
a72d9cb8d0c(run 30738774634), with the same numeric deltas.One item I could not attribute and did not re-record: wasm
pickle_terminal_raise_resumereadsloops_aborted 54 -> 59locally, and main'sbase CI passes it. The native backends are bit-identical to that fixture's
committed baseline (33 loops / 0 aborted / 487 guard failures) and
abort: force quasi-immutis 0 there, so it is not this change's semantics; thecounter is layout-sensitive on that fixture. Whether it reproduces on the CI
host is the open question this PR's wasm leg answers.
🤖 Generated with Claude Code