The GC pins the wasm guest never received, and four parity/test fixes - #1073
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (59)
WalkthroughThe PR adds wasm GC environment forwarding, per-script parity environments, frameless class keyword dispatch, C-int seek conversion, related parity tests, and updated synthetic JIT statistics. ChangesGC environment forwarding
Frameless class calls
C-int seek conversion
JIT benchmark statistics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant pyre_wasm_runner
participant pyre_wasm
participant majit_gc
pyre_wasm_runner->>pyre_wasm: query pyre_gc_env_names
pyre_wasm-->>pyre_wasm_runner: return supported GC names
pyre_wasm_runner->>pyre_wasm: send NAME=value entries
pyre_wasm->>majit_gc: call set_supplied_env
majit_gc->>majit_gc: resolve process values before supplied values
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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 9f041c7). 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
|
![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: 307d3d045b
ℹ️ 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".
| //! `bytearray` is already the growable mutable buffer `__bigbuffer` stands | ||
| //! for, so the two tiers — and with them the `AT_END` position sentinel that | ||
| //! only exists to keep the builder tier live — collapse into a single store | ||
| //! here. Holding it as a `bytearray` object rather than a Rust `Vec` is what | ||
| //! lets `getbuffer` hand out a real view of the stream and lets |
There was a problem hiding this comment.
Replace this explicit storage collapse with the literal RStringIO field and method structure: the repository requires PyPy/RPython ports to retain strict line-by-line structural parity rather than substituting a locally convenient representation. The single bytearray changes where resize/export state lives and makes later methods depend on bytearray-specific behavior instead of the upstream StringBuilder/__bigbuffer/AT_END invariants.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| /// `interp_bytesio.py:159` `seek_w`. | ||
| fn seek(&mut self, pos: i64, #[default(0)] whence: i64) -> Result<PyObjectRef, crate::PyError> { | ||
| self.check_closed()?; | ||
| self.seek_to(pos, whence as i32)?; |
There was a problem hiding this comment.
Validate or bind whence as the intended integer type instead of casting it: values such as 4294967296 wrap to 0 and 4294967298 wraps to 2, so seek() performs a valid seek rather than rejecting an unsupported whence. This affects any caller supplying an integer outside the i32 range and silently gives that integer unrelated semantics.
Useful? React with 👍 / 👎.
| return Vec::new(); | ||
| } | ||
| let data = self.data(); | ||
| let start = self.pos.max(0) as usize; |
There was a problem hiding this comment.
Use a checked conversion for positions rather than as usize: on wasm32, after seek(1 << 32), this cast wraps the logical position to zero, so read() returns data from the beginning instead of EOF. The same unchecked conversion pattern in read_line, write_bytes, and truncate_to can likewise read, overwrite, or truncate the wrong part of the stream for offsets or sizes above u32::MAX.
Useful? React with 👍 / 👎.
youknowone
changed the title
307d3d0 to
aa397e7
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/aa397e75e02d7c371d9e2f878c3eddb03d2d2361/pyre-wasm-runner/src/main.rs#L461-L465
When the runner is invoked with --engine wasmi (or PYRE_WASM_ENGINE=wasmi), the dispatch goes to wasmi_host::run and never executes this new forwarding block; I inspected that path through its pyre_alloc/pyre_run_python setup, and it does not query or call either GC-environment export before starting the guest. Consequently PYPY_GC_NURSERY, PYPY_GC_MIN, PYPY_GC_MAX, and the other advertised settings still resolve to defaults under the supported wasmi engine, so configured heap limits and the pinned JIT-stat collection schedule differ from wasmtime. Mirror this exchange in wasmi_host::run or share it between both engine paths.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/call.rs`:
- Around line 4853-4861: In the function containing the `init_subclass_kwargs`
handling, pin every key and value from the source dictionary at function entry
before any `super_check` or `getattr_str` execution. When constructing `kwds`,
reload both objects from the shadow stack and use those rooted references for
`w_str_get_wtf8` and the value passed to `call_with_kwargs_in_ctx`, preserving
the existing RPython/PyPy storage shape and line-by-line structure.
In `@pyre/pyre-interpreter/src/module/_io/bytesio.rs`:
- Around line 79-82: Update the error messages in check_closed and seek_to to
match the upstream strings exactly: remove the trailing period from the
closed-file message and use the specified invalid-whence format with the actual
whence value. Preserve the existing exception types and control flow.
- Around line 398-403: Guard both buffer mutations with check_exports(): update
close() to validate exports before replacing w_buffer, and update __setstate__()
to validate exports before truncating the existing buffer ahead of write(). Add
regression tests covering getbuffer() followed by close() and __setstate__(),
ensuring live views prevent the prohibited length changes.
- Around line 193-196: Update the relative-seek calculation around
`base.checked_add(position)` so negative underflow is clamped to zero before
addition, while positive overflow still returns `OverflowError`. Preserve the
existing `self.pos = target.max(0)` behavior and apply this to both `SEEK_CUR`
and `SEEK_END` paths.
- Around line 107-113: Update the BytesIO read, truncate, and write paths around
the position and size conversions to range-check i64 values before converting
them to usize, including the logic using start and count. Treat positions above
usize::MAX as EOF for reads, compare truncate sizes before conversion, and
return OverflowError before writes mutate storage when the requested size is
unaddressable; preserve normal behavior for representable values.
🪄 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: 2b16c566-12aa-48b2-93c3-83c3f89c512e
📒 Files selected for processing (66)
majit/majit-gc/src/collector.rspyre/bench/synth/arith_int_bool.wasm.jitstatspyre/bench/synth/binary_slice_index.wasm.jitstatspyre/bench/synth/bool_dunder_error_no_leak.wasm.jitstatspyre/bench/synth/build_set_hashability.wasm.jitstatspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/dict_set.wasm.jitstatspyre/bench/synth/divmod_long_int_pair.wasm.jitstatspyre/bench/synth/exc_caught_in_callee_return_loop.wasm.jitstatspyre/bench/synth/exception_escape_hot_callee_tb_node_once.wasm.jitstatspyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstatspyre/bench/synth/exception_inlined_callee_caught.wasm.jitstatspyre/bench/synth/exception_oserror_fields.wasm.jitstatspyre/bench/synth/exception_subclass_attrs.wasm.jitstatspyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstatspyre/bench/synth/exception_traceback_loop_forms.wasm.jitstatspyre/bench/synth/exception_value_op_caught.wasm.jitstatspyre/bench/synth/float_div_zero_caught_loop.wasm.jitstatspyre/bench/synth/foriter_body_return.wasm.jitstatspyre/bench/synth/foriter_exempt_nested_foriter.wasm.jitstatspyre/bench/synth/foriter_exempt_shared_generator.wasm.jitstatspyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstatspyre/bench/synth/gc_deque_backing_list.wasm.jitstatspyre/bench/synth/getattribute_override_no_bind.wasm.jitstatspyre/bench/synth/global_cell_shortpreamble_hot.wasm.jitstatspyre/bench/synth/inline_gate_operand_provenance.wasm.jitstatspyre/bench/synth/inline_subwalk_mutating_residual.wasm.jitstatspyre/bench/synth/inline_subwalk_property_mutates.wasm.jitstatspyre/bench/synth/inline_subwalk_user_iterator.wasm.jitstatspyre/bench/synth/kept_stack_deep_var_shortcircuit.wasm.jitstatspyre/bench/synth/list_ops.wasm.jitstatspyre/bench/synth/loops_comprehension.wasm.jitstatspyre/bench/synth/match_sequence_of_class_patterns.wasm.jitstatspyre/bench/synth/math_isqrt_compare_bridge_resume.wasm.jitstatspyre/bench/synth/mutate_then_raise_caught.wasm.jitstatspyre/bench/synth/newslice_step_hot.wasm.jitstatspyre/bench/synth/pickle_ctor_args.cranelift.jitstatspyre/bench/synth/pickle_ctor_args.dynasm.jitstatspyre/bench/synth/pickle_ctor_args.wasm.jitstatspyre/bench/synth/pickle_terminal_raise_resume.cranelift.jitstatspyre/bench/synth/pickle_terminal_raise_resume.dynasm.jitstatspyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstatspyre/bench/synth/recursion_memo_branch.wasm.jitstatspyre/bench/synth/recursive_call_frame_relocation.wasm.jitstatspyre/bench/synth/sre_pattern_methods.wasm.jitstatspyre/bench/synth/str_fstring.wasm.jitstatspyre/bench/synth/type_metatype_method_call.wasm.jitstatspyre/bench/synth/type_name_setter.wasm.jitstatspyre/bench/synth/type_name_surrogate_reject.wasm.jitstatspyre/bench/synth/unary_negative.wasm.jitstatspyre/bench/synth/unary_positive_resume.wasm.jitstatspyre/bench/synth/unpack_ex_hot.wasm.jitstatspyre/bench/synth/wasm_ca_trampoline_decline.wasm.jitstatspyre/check.pypyre/extra_tests/parity_tests/run.pypyre/extra_tests/parity_tests/thread_start_walk_abort_no_replay.pypyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_io/_io_app.pypyre/pyre-interpreter/src/module/_io/bytesio.rspyre/pyre-interpreter/src/module/_io/mod.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/lib.rspyre/pyre-object/src/pyobject.rspyre/pyre-wasm-runner/src/main.rspyre/pyre-wasm/src/lib.rs
| fn check_closed(&self) -> Result<(), crate::PyError> { | ||
| if self.closed { | ||
| return Err(crate::PyError::value_error("I/O operation on closed file.")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the upstream exception messages.
check_closed adds a trailing period. seek_to uses a different invalid-whence message. These strings are observable API behavior. Use "I/O operation on closed file" and "whence must be between 0 and 2, not {whence}". (raw.githubusercontent.com)
Also applies to: 187-190
🤖 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/_io/bytesio.rs` around lines 79 - 82, Update
the error messages in check_closed and seek_to to match the upstream strings
exactly: remove the trailing period from the closed-file message and use the
specified invalid-whence format with the actual whence value. Preserve the
existing exception types and control flow.
Source: Coding guidelines
| let start = self.pos.max(0) as usize; | ||
| if start >= data.len() { | ||
| return Vec::new(); | ||
| } | ||
| let mut count = data.len() - start; | ||
| if size >= 0 { | ||
| count = count.min(size as usize); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent i64 to usize truncation on wasm32.
On wasm32, these casts wrap positions and sizes above usize::MAX. For example, seek(1 << 32); read() can read from offset zero, and truncate(1 << 32) can truncate the wrong buffer range. Convert only after range checks. Treat an unaddressable read position as EOF, compare read and truncate sizes before conversion, and return OverflowError before a write mutates storage.
Also applies to: 129-135, 150-160, 164-170
🤖 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/_io/bytesio.rs` around lines 107 - 113,
Update the BytesIO read, truncate, and write paths around the position and size
conversions to range-check i64 values before converting them to usize, including
the logic using start and count. Treat positions above usize::MAX as EOF for
reads, compare truncate sizes before conversion, and return OverflowError before
writes mutate storage when the requested size is unaddressable; preserve normal
behavior for representable values.
| fn close(&mut self) { | ||
| self.closed = true; | ||
| self.w_buffer = pyre_object::bytearrayobject::w_bytearray_from_bytes(&[]); | ||
| self.pos = 0; | ||
| pyre_object::gc_hook::try_gc_write_barrier(self as *mut Self as *mut u8); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Block buffer replacement and truncation while a view is exported.
getbuffer() exports w_buffer. close() replaces that buffer, and __setstate__() truncates it before write() checks exports. A live memoryview can therefore observe a prohibited length change. Call check_exports() before either mutation. Add regression tests for getbuffer() followed by close() and __setstate__().
Proposed fix
- fn close(&mut self) {
+ fn close(&mut self) -> Result<(), crate::PyError> {
+ self.check_exports()?;
self.closed = true;
self.w_buffer = pyre_object::bytearrayobject::w_bytearray_from_bytes(&[]);
self.pos = 0;
pyre_object::gc_hook::try_gc_write_barrier(self as *mut Self as *mut u8);
+ Ok(())
}
fn __setstate__(&mut self, w_state: PyObjectRef) -> Result<(), crate::PyError> {
self.check_closed()?;
+ self.check_exports()?;
// validate state
self.truncate_to(0);Also applies to: 444-446
🤖 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/_io/bytesio.rs` around lines 398 - 403,
Guard both buffer mutations with check_exports(): update close() to validate
exports before replacing w_buffer, and update __setstate__() to validate exports
before truncating the existing buffer ahead of write(). Add regression tests
covering getbuffer() followed by close() and __setstate__(), ensuring live views
prevent the prohibited length changes.
Source: Coding guidelines
78dde58 to
85ebd1e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85ebd1ea0f
ℹ️ 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".
| // (`add_to_autoflusher=False`) — an in-memory buffer has nothing to | ||
| // write out at exit. | ||
| let needs_finalizer = !cls.is_null() && !std::ptr::eq(cls, type_object()); | ||
| super::tag_io_instance_with_finalizer(obj, cls, needs_finalizer) |
There was a problem hiding this comment.
Use a registration path that does not call autoflusher_add: despite the comment documenting add_to_autoflusher=False, tag_io_instance_with_finalizer unconditionally adds the object before conditionally registering its finalizer. Consequently, every exact BytesIO is now visited by flush_all_streams; for example, a still-live instance whose flush attribute was replaced gets that callback invoked during _io shutdown, whereas both the previous exact-class allocation path and PyPy's W_BytesIO.__init__ exclude it from the autoflusher.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
85ebd1e to
e43956d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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: e43956d9ea
ℹ️ 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".
| let gc_env_names = instance | ||
| .get_typed_func::<(), u64>(&mut store, "pyre_gc_env_names") | ||
| .ok(); | ||
| let set_gc_env = instance | ||
| .get_typed_func::<(u32, u32), ()>(&mut store, "pyre_set_gc_env") |
There was a problem hiding this comment.
When --engine wasmi or PYRE_WASM_ENGINE=wasmi is selected, main dispatches to wasmi_host::run, which goes directly from resolving pyre_alloc/pyre_run_python to setting the script path and never invokes these new pyre_gc_env_names/pyre_set_gc_env exports. Consequently, check.py's PYPY_GC_NURSERY and PYPY_GC_MIN pins still remain invisible to that supported wasm engine, so it uses the old collection thresholds and can produce guard counts inconsistent with the newly recorded shared .wasm.jitstats baselines. Add the same handshake to wasmi_host::run before its call to pyre_run_python.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/call.rs`:
- Around line 4071-4085: Root managed call inputs before allocation at both
sites in pyre/pyre-interpreter/src/call.rs:4071-4085 and 4853-4861. In the
__prepare__ path, pin prepare, bases, and every prepare_kwds value before
w_str_new(name), then reload them from the shadow stack when constructing the
call; in the __init_subclass__ path, pin every init_subclass_kwargs key and
value at function entry, then reload both after super_check and getattr_str.
Preserve RPython/PyPy storage shape and strict line-by-line structural parity.
🪄 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: 11da880b-950b-4d12-966f-444a952b5a6a
📒 Files selected for processing (60)
majit/majit-gc/src/collector.rspyre/bench/synth/arith_int_bool.wasm.jitstatspyre/bench/synth/binary_slice_index.wasm.jitstatspyre/bench/synth/bool_dunder_error_no_leak.wasm.jitstatspyre/bench/synth/build_set_hashability.wasm.jitstatspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/dict_set.wasm.jitstatspyre/bench/synth/divmod_long_int_pair.wasm.jitstatspyre/bench/synth/exc_caught_in_callee_return_loop.wasm.jitstatspyre/bench/synth/exception_escape_hot_callee_tb_node_once.wasm.jitstatspyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstatspyre/bench/synth/exception_inlined_callee_caught.wasm.jitstatspyre/bench/synth/exception_oserror_fields.wasm.jitstatspyre/bench/synth/exception_subclass_attrs.wasm.jitstatspyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstatspyre/bench/synth/exception_traceback_loop_forms.wasm.jitstatspyre/bench/synth/exception_value_op_caught.wasm.jitstatspyre/bench/synth/float_div_zero_caught_loop.wasm.jitstatspyre/bench/synth/foriter_body_return.wasm.jitstatspyre/bench/synth/foriter_exempt_nested_foriter.wasm.jitstatspyre/bench/synth/foriter_exempt_shared_generator.wasm.jitstatspyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstatspyre/bench/synth/gc_deque_backing_list.wasm.jitstatspyre/bench/synth/gc_iterator_source_drop.wasm.jitstatspyre/bench/synth/getattribute_override_no_bind.wasm.jitstatspyre/bench/synth/global_cell_shortpreamble_hot.wasm.jitstatspyre/bench/synth/inline_gate_operand_provenance.wasm.jitstatspyre/bench/synth/inline_subwalk_mutating_residual.wasm.jitstatspyre/bench/synth/inline_subwalk_property_mutates.wasm.jitstatspyre/bench/synth/inline_subwalk_user_iterator.wasm.jitstatspyre/bench/synth/kept_stack_deep_var_shortcircuit.wasm.jitstatspyre/bench/synth/list_ops.wasm.jitstatspyre/bench/synth/loops_comprehension.wasm.jitstatspyre/bench/synth/match_sequence_of_class_patterns.wasm.jitstatspyre/bench/synth/math_isqrt_compare_bridge_resume.wasm.jitstatspyre/bench/synth/mutate_then_raise_caught.wasm.jitstatspyre/bench/synth/newslice_step_hot.wasm.jitstatspyre/bench/synth/recursion_memo_branch.wasm.jitstatspyre/bench/synth/recursive_call_frame_relocation.wasm.jitstatspyre/bench/synth/sre_pattern_methods.wasm.jitstatspyre/bench/synth/str_fstring.wasm.jitstatspyre/bench/synth/type_name_setter.wasm.jitstatspyre/bench/synth/type_name_surrogate_reject.wasm.jitstatspyre/bench/synth/unary_negative.wasm.jitstatspyre/bench/synth/unary_positive_resume.wasm.jitstatspyre/bench/synth/unpack_ex_hot.wasm.jitstatspyre/bench/synth/wasm_ca_trampoline_decline.wasm.jitstatspyre/check.pypyre/extra_tests/parity_tests/dict_subscript_fold.pypyre/extra_tests/parity_tests/exception_instance_dict_attr.pypyre/extra_tests/parity_tests/memoryio_seek_whence_range.pypyre/extra_tests/parity_tests/object_init_text_signature.pypyre/extra_tests/parity_tests/run.pypyre/extra_tests/parity_tests/thread_start_walk_abort_no_replay.pypyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/module/_io/bytesio.rspyre/pyre-interpreter/src/module/_io/stringio.rspyre/pyre-jit/src/lib.rspyre/pyre-wasm-runner/src/main.rspyre/pyre-wasm/src/lib.rs
e43956d to
2f017d5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f017d5fe0
ℹ️ 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".
| /// object carrying both dunders would be read from the wrong one, and one | ||
| /// carrying only `__int__` would be accepted where 3.14 raises TypeError. | ||
| pub fn index_c_int_w(obj: PyObjectRef) -> Result<i32, PyError> { | ||
| let value = int_w(space_index(obj)?)?; |
There was a problem hiding this comment.
For objects defining only __int__, or defining disagreeing __int__ and __index__, this new helper changes both memory streams from the checked-in PyPy behavior: interp_bytesio.py and interp_stringio.py declare whence/mode with unwrap_spec(...=int), whose c_int_w calls gateway_int_w = int_w, while this line forces space_index. Consequently an IntOnly value is rejected and a Both value selects a different seek origin; retain the C-int range check but route through the existing c_int_w converter rather than introducing an index-only protocol.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pyre/pyre-interpreter/src/call.rs (1)
4071-4085: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRoot keyword-call inputs before caller-side allocations.
call_with_kwargs_in_ctxinstalls roots only after its arguments are evaluated. The caller can therefore pass stale managed pointers after a moving collection.
pyre/pyre-interpreter/src/call.rs#L4071-L4085: Pinprepare,bases, and everyprepare_kwdsvalue beforew_str_new(name). Reload all rooted pointers when building both the keyword and positional call paths.pyre/pyre-interpreter/src/call.rs#L4853-L4861: Pin everyinit_subclass_kwargskey and value beforesuper_check. Reload both objects from the root scope when constructingkwdsaftersuper_checkandgetattr_str.As per coding guidelines, preserve strict line-by-line structural parity with RPython/PyPy.
🤖 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/call.rs` around lines 4071 - 4085, The class-preparation call path at pyre/pyre-interpreter/src/call.rs:4071-4085 must root prepare, bases, and every prepare_kwds value before w_str_new(name), then reload all rooted pointers when constructing both keyword and positional call arguments; preserve strict line-by-line parity with RPython/PyPy. The init-subclass path at pyre/pyre-interpreter/src/call.rs:4853-4861 must root every init_subclass_kwargs key and value before super_check, then reload both objects from the root scope when constructing kwds after super_check and getattr_str.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@pyre/pyre-interpreter/src/call.rs`:
- Around line 4071-4085: The class-preparation call path at
pyre/pyre-interpreter/src/call.rs:4071-4085 must root prepare, bases, and every
prepare_kwds value before w_str_new(name), then reload all rooted pointers when
constructing both keyword and positional call arguments; preserve strict
line-by-line parity with RPython/PyPy. The init-subclass path at
pyre/pyre-interpreter/src/call.rs:4853-4861 must root every init_subclass_kwargs
key and value before super_check, then reload both objects from the root scope
when constructing kwds after super_check and getattr_str.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a0e20657-eadb-4e49-8a3d-3eb335c06077
📒 Files selected for processing (62)
majit/majit-gc/src/collector.rspyre/bench/synth/arith_int_bool.wasm.jitstatspyre/bench/synth/binary_slice_index.wasm.jitstatspyre/bench/synth/bool_dunder_error_no_leak.wasm.jitstatspyre/bench/synth/build_set_hashability.wasm.jitstatspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/dict_set.wasm.jitstatspyre/bench/synth/divmod_long_int_pair.wasm.jitstatspyre/bench/synth/exc_caught_in_callee_return_loop.wasm.jitstatspyre/bench/synth/exception_escape_hot_callee_tb_node_once.wasm.jitstatspyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstatspyre/bench/synth/exception_inlined_callee_caught.wasm.jitstatspyre/bench/synth/exception_oserror_fields.wasm.jitstatspyre/bench/synth/exception_subclass_attrs.wasm.jitstatspyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstatspyre/bench/synth/exception_traceback_loop_forms.wasm.jitstatspyre/bench/synth/exception_value_op_caught.wasm.jitstatspyre/bench/synth/float_div_zero_caught_loop.wasm.jitstatspyre/bench/synth/foriter_body_return.wasm.jitstatspyre/bench/synth/foriter_exempt_nested_foriter.wasm.jitstatspyre/bench/synth/foriter_exempt_shared_generator.wasm.jitstatspyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstatspyre/bench/synth/gc_deque_backing_list.wasm.jitstatspyre/bench/synth/gc_iterator_source_drop.wasm.jitstatspyre/bench/synth/getattribute_override_no_bind.wasm.jitstatspyre/bench/synth/global_cell_shortpreamble_hot.wasm.jitstatspyre/bench/synth/inline_gate_operand_provenance.wasm.jitstatspyre/bench/synth/inline_subwalk_mutating_residual.wasm.jitstatspyre/bench/synth/inline_subwalk_property_mutates.wasm.jitstatspyre/bench/synth/inline_subwalk_user_iterator.wasm.jitstatspyre/bench/synth/kept_stack_deep_var_shortcircuit.wasm.jitstatspyre/bench/synth/list_ops.wasm.jitstatspyre/bench/synth/loops_comprehension.wasm.jitstatspyre/bench/synth/match_sequence_of_class_patterns.wasm.jitstatspyre/bench/synth/math_isqrt_compare_bridge_resume.wasm.jitstatspyre/bench/synth/mutate_then_raise_caught.wasm.jitstatspyre/bench/synth/newslice_step_hot.wasm.jitstatspyre/bench/synth/recursion_memo_branch.wasm.jitstatspyre/bench/synth/recursive_call_frame_relocation.wasm.jitstatspyre/bench/synth/sre_pattern_methods.wasm.jitstatspyre/bench/synth/str_fstring.wasm.jitstatspyre/bench/synth/type_name_setter.wasm.jitstatspyre/bench/synth/type_name_surrogate_reject.wasm.jitstatspyre/bench/synth/unary_negative.wasm.jitstatspyre/bench/synth/unary_positive_resume.wasm.jitstatspyre/bench/synth/unpack_ex_hot.wasm.jitstatspyre/bench/synth/wasm_ca_trampoline_decline.wasm.jitstatspyre/check.pypyre/extra_tests/parity_tests/dict_subscript_fold.pypyre/extra_tests/parity_tests/exception_instance_dict_attr.pypyre/extra_tests/parity_tests/memoryio_seek_whence_range.pypyre/extra_tests/parity_tests/object_init_text_signature.pypyre/extra_tests/parity_tests/run.pypyre/extra_tests/parity_tests/thread_start_walk_abort_no_replay.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/module/_io/bytesio.rspyre/pyre-interpreter/src/module/_io/stringio.rspyre/pyre-jit/src/lib.rspyre/pyre-macros/src/lib.rspyre/pyre-wasm-runner/src/main.rspyre/pyre-wasm/src/lib.rs
`run.py` reads `# parity-env: NAME=VALUE` lines from each script and adds them to the environment of every runner for that script only. `thread_start_walk_abort_no_replay.py` pins `PYRE_FBW_CALLEE_VSTACK=0`. The three kept-stack branch-guard decline hazards are scoped to `!ctx.vstack_valid`, so with the callee operand-stack mirror on by default (7b87e22) an inline sub-walk describes its own stack and the aborting guard is never reached. Measured on a binary built from the commit before the walk-abort fix: the script passes 4/4 at the default setting and fails 4/4 with the mirror off, in both cases at round 97. Verified through `run.py` itself: with the pre-fix binary in `target/release` the run reports this script as its single failure; with both backends built at HEAD the full suite passes. Assisted-by: Claude
…patcher
`build_class`'s non-type metaclass call, its `__prepare__` call and
`_init_subclass` each resolved a caller frame with `gettopframe_raw()` only
to satisfy `call_with_kwargs(frame, …)`, and each took a fallback arm when
that frame was null: the first two dropped the class-definition keywords,
and the third raised `TypeError("__init_subclass__() takes no keyword
arguments")`.
Upstream passes the keywords unconditionally through the frameless
`space.call_args` (descroperation.py:189) at all three sites —
compiling.py:199 for `__prepare__`, :221 for the metaclass, and
typeobject.py:1025-1026 `args = __args__.replace_arguments([])` /
`space.call_args(w_func, args)` for `__init_subclass__`. None of the three
fallback arms has an upstream counterpart. Call `call_with_kwargs_in_ctx`
at each site and delete them.
The fallbacks were not reachable from a script: the execution context's
frame is non-null throughout normal execution, and a metaclass with
`__prepare__` plus class keywords, and `__init_subclass__` with keywords,
already matched CPython 3.14 before this change. What this removes is
three `force_vref` calls and three arms with no upstream basis, leaving
`c_profile_frame` as the only `gettopframe_raw()` in the file.
Also correct two nearby line citations: compiling.py:190-196 -> :194-199
and :213-219 -> :214-221.
Assisted-by: Claude
`check.py` pins `PYPY_GC_NURSERY` and `PYPY_GC_MIN` so the major-collection threshold is a property of the tree rather than of the machine. Both pins reached the two native backends and neither reached the wasm one: the guest is built for `wasm32-unknown-unknown`, whose `std::env` is permanently empty, and `majit-gc` resolves those names through `std::env::var` (collector.rs:93). The guest therefore kept `min_heap_size = nursery * 8` = 32MB, crossed it mid-run, and counted the back-edge eval-breaker poll's bailouts that the natives no longer see. `warn_inert_guest_env` did not report it either — it matches the `PYRE_` and `MAJIT_` prefixes, and these are `PYPY_`. Measured, `recursive_call_frame_relocation`'s guest-side `guard_failures` against `PYPY_GC_MIN`: unset 648, 8MB 695, 256MB 638. Before this change all three read 648. The native backend moves under the same override — 636 -> 639, and `fib_loop` 189 -> 193 — which is what said the variable was reaching one side and not the other. `majit-gc` takes an embedder-supplied environment, read only where `std::env` misses, and publishes the names it resolves; `pyre-wasm` exports `pyre_set_gc_env` / `pyre_gc_env_names` over it and the runner forwards whatever its own environment carries. That is the pair `pyre_set_launch_env` / `pyre_launch_env_names` already form for the launcher's variables, for the same reason. The wasmi engine path is left as it is: it forwards neither this nor the launch environment, so it already runs without `PYTHONSAFEPATH` and is not a jit-stats engine. Assisted-by: Claude
The preceding commit lets `check.py`'s `PYPY_GC_NURSERY` / `PYPY_GC_MIN` reach the wasm guest, which pushes the major-collection threshold past every fixture's working set. The collection those readings were counting is gone, so the counters that counted it move. Forty-nine rows across 47 fixtures, and **every one of them falls**: `guard_failures` by 1 to 202 (`exception_escape_hot_callee_tb_node_once` 1016 -> 814, `exception_inline_callee_tb_frames` 1008 -> 807, `closure_per_call` 470 -> 420), plus `bridges_compiled` 5 -> 4 on those same two — the bridge the loop had spent on the eval-breaker poll. Nothing rises anywhere, and no other backend's baselines are touched, because the pins were already reaching those. Four of these are rows the base measures from a threshold that was still free to move, three of them re-recorded at that free value as recently as #1071: `closure_per_call` 470 -> 468 -> 420, `exception_traceback_frame_lineno` 820 -> 819 -> 817, `recursive_call_frame_relocation` 649 -> 648 -> 638, and `gc_iterator_source_drop` 613 -> 614 -> 613. Recorded with `check.py --snapshot --backend wasm`, which then reports `ALL PASSED: wasm 384/384`. Its unrelated side effect of adding two new `field_pos_*_misplaced` fields to every wasm file was stripped, except on the four files whose committed baselines already carry them. Assisted-by: Claude
`BytesIO.seek` and `StringIO.seek` unwrapped their whence with the machine-int converter, so a value outside a C int's range reached the `0/1/2` range check and came back as `ValueError: invalid whence (4294967296, should be 0, 1 or 2)`. Both `@unwrap_spec(pos=r_longlong, whence=int)` (interp_bytesio.py:162) and `@unwrap_spec(pos=int, mode=int)` (interp_stringio.py:403) take it as a C int, and 3.14 raises `OverflowError: Python int too large to convert to C int` for one that does not fit. Neither existing converter is that parameter. `c_int_w` has the range check but reaches the value through `gateway_int_w` = `int_w` (baseobjspace.py:2043), which converts through `__int__` first; `index_int_w_preserve_negative` is on the index protocol but is a machine int. A whence needs both, so add `index_c_int_w` — `space_index` followed by the 32-bit check — and the `PyIndexCInt` unwrap alias over it, and use it at both sites. Reading the whence through `__int__` is observable, not merely a message difference: an object defining `__index__` as 0 and `__int__` as 2 seeks to the end rather than the start, silently, and one defining only `__int__` is accepted where 3.14 raises TypeError. The parity script asserts the whence by value against a non-empty buffer so neither case can pass, and pins the out-of-range half and the position's wider `Py_ssize_t` range beside it. The position is left alone: upstream's `pos=int` on `StringIO` is a C int too, but 3.14 takes it as a `Py_ssize_t` and accepts `seek(2**32)`. Assisted-by: Claude
2f017d5 to
9f041c7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Six commits. One measurement fix plus four small parity/test fixes.
Local, on this base (
fd709f26ae1, #1079):dynasm 388/388,cranelift 388/388,wasm 384/384, andpyre/extra_tests/parity_testsall green.1. The wasm guest never saw
check.py's GC pinscheck.pypinsPYPY_GC_NURSERY/PYPY_GC_MINso the major-collectionthreshold is a property of the tree rather than of the machine (#1059). Both
pins reached the two native backends and neither reached the wasm one: the
guest is built for
wasm32-unknown-unknown, whosestd::envis permanentlyempty, and
majit-gcresolves those names throughstd::env::var. The guestkept
min_heap_size = nursery * 8= 32MB, crossed it mid-run, and counted theback-edge eval-breaker poll's bailouts the natives no longer see.
Measured —
recursive_call_frame_relocation's guest-sideguard_failuresagainst
PYPY_GC_MIN, with the native backend as the control:fib_loop189→193)One side responding and the other flat is what identified it. Forcing a wrong
value rather than the right one is what makes the row informative — with
env.setdefaultsemantics, pinning the correct value teaches nothing.majit-gctakes an embedder-supplied environment, read only wherestd::envmisses, and publishes the names it resolves (
GC_ENV_NAMES);pyre-wasmexports
pyre_set_gc_env/pyre_gc_env_namesover it, and the runner forwardswhatever its own environment carries. The pair
pyre_set_launch_env/pyre_launch_env_namesalready exist for the launcher's variables, for the samereason and in the same shape.
Re-record: 48 rows across 46 wasm fixtures, and every one of them falls
—
guard_failuresby 1 to 202 (exception_escape_hot_callee_tb_node_once1016→814,
closure_per_call470→420), plusbridges_compiled5→4 on two, thebridge those loops had spent on the eval-breaker poll. Nothing rises anywhere,
and no other backend is touched because the pins already reached those.
The pin moves the guest toward the natives without closing the gap entirely —
exception_escape_hot_callee_tb_node_oncelands exactly on their 814, whilerecursive_call_frame_relocationreads 638 against 636 andexception_inline_callee_tb_frames807 against 606. That residual ispre-existing and separate: on wasm
gc_interp::enabled()defaults tocfg!(target_arch = "wasm32"), so the guest genuinely armsEB_GCwhere thenatives do not. This PR does not claim to close it.
These are the four rows
mainhas been red on in its own ubuntu CI. #1071re-recorded three of them at the free-threshold value and #1077 re-recorded
the top-level wasm files without moving any counter, so they are still open on
main; this replaces them with the pinned measurement.2. Three class-creation hooks through the frameless dispatcher
build_class's non-type metaclass call, its__prepare__call and_init_subclasseach resolved a caller frame withgettopframe_raw()only tosatisfy
call_with_kwargs(frame, …), and each took a fallback arm when thatframe was null: the first two dropped the class-definition keywords, the third
raised
TypeError("__init_subclass__() takes no keyword arguments").Upstream passes the keywords unconditionally through the frameless
space.call_args(descroperation.py:189) at all three sites. None of the threefallback arms has an upstream counterpart, and none was reachable from a script.
What this removes is three
force_vrefcalls and three arms with no upstreambasis.
3.
seek's whence is a C int, on the index protocolBoth memory streams unwrapped
seek's whence with the machine-int converter, soa value outside a C int's range reached the
0/1/2range check and came back asValueError: invalid whence (4294967296, should be 0, 1 or 2).@unwrap_spec(pos=r_longlong, whence=int)(interp_bytesio.py:162) and@unwrap_spec(pos=int, mode=int)(interp_stringio.py:403) take it as a C int,and 3.14 raises
OverflowErrorfor one that does not fit.Neither existing converter is that parameter, and reaching for the nearest one
is a trap this PR walked into and backed out of:
c_int_whas the range checkbut resolves through
gateway_int_w=int_w(baseobjspace.py:2043), whichconverts via
__int__first. That is observable, not cosmetic — an objectdefining
__index__as 0 and__int__as 2 seeks to the end rather than thestart, silently, and one defining only
__int__is accepted where 3.14 raisesTypeError. So the commit adds
index_c_int_w(space_index, then the 32-bitcheck) and the
PyIndexCIntalias over it.seek(0, w)onBytesIO(b"abcdefgh")mainc_int_ww = 2**32__index__→0,__int__→2__int__→0The position is left alone: upstream's
pos=intonStringIOis a C int too,but 3.14 takes it as a
Py_ssize_tand acceptsseek(2**32).The parity script asserts the whence by value against a non-empty buffer, so
the two middle rows cannot pass by accident, and it is non-vacuous against a
real control — a
mainbuild carrying #1079 fails it withexpected OverflowError, got ValueError('invalid whence (4294967296, …)').4. Two test-harness fixes
# parity-env: NAME=VALUE— a script whose defect is only reachable under aparticular runtime configuration can now declare it. Without this a script
keeps passing after the shape it exercises stops being reachable, i.e. covers
nothing while still looking green.
run.py's"OK"sentinel (:121), so every runner — CPython included — reports FAIL. They are
the only three of 336 without it, and they are why
main's parity step hasbeen failing since mapdict, dict, inline: instance-dict backing, live exact-dict lookups, and a materialized callee frame's locals #1067 (run
31080288895, windows). The suite goes from9 failures to none.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
BytesIO.seek()andStringIO.seek()now correctly validatewhence, support__index__, and reject out-of-range or invalid values.Tests