run: support directory and zip targets and reduce list GC memory - #1163
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR hardens moving-GC handling across virtual references, lists, interpreter code, and JIT paths. It refactors script and module execution, adds package-target parity coverage, updates runtime compatibility behavior, refreshes test and JIT baselines, and pins RustPython dependencies. ChangesMoving-GC safety
Script and module execution
Runtime compatibility and baselines
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95700c64eb
ℹ️ 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".
| crate::gc_hook::try_gc_alloc_collecting_rooted( | ||
| W_LIST_GC_TYPE_ID, | ||
| W_LIST_OBJECT_SIZE, | ||
| &mut allocation_root, | ||
| &mut needs_write_barrier, | ||
| ) |
There was a problem hiding this comment.
When a young list reaches a GC safepoint during an operation, the collector updates shadow-stack roots but cannot update an existing Rust &mut W_ListObject. Switching headers to this collecting allocator therefore makes existing paths unsafe: W_ListObject::object_grow allocates a backing block and then continues through self, while w_list_pop may box an item before calling list.sync_allocated; path_hook_accepts likewise calls one hook and then indexes the same raw list again. If that allocation or hook triggers a minor collection, these paths access the evacuated nursery address, causing corruption or incorrect directory/zip target detection. The owner must be rooted and reloaded after every safepoint, including accounting for the address-striped list lock, before headers can safely move.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit cbd0267). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fb1871711
ℹ️ 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".
| if let Err(e) = init_importlib_bootstrap(canonical, ec_ptr) { | ||
| eprintln!("pyre: importlib bootstrap failed: {}", e.message_text()); | ||
| } | ||
| path_hook_accepts(filename, canonical, ec_ptr) |
There was a problem hiding this comment.
When sitecustomize installs or removes a path hook, this probes only the bootstrap hooks and does not call import_site until after a target has already been accepted. PyPy imports site in pypy/interpreter/app_main.py:875-882 before iterating sys.path_hooks at lines 1054-1061, so custom executable-path formats are ignored or misclassified here; move site initialization ahead of this probe.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1070a68ed7
ℹ️ 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".
| if let Err(e) = runpy_run_module_as_main(canonical, ec_ptr, "__main__", false) { | ||
| handle_main_error(e, canonical, ec_ptr); | ||
| } | ||
| finish_main(canonical, ec_ptr); |
There was a problem hiding this comment.
When a directory or zip target is run with -i, this branch calls finish_main before returning to the dispatcher, which then invokes repl::run_repl; that function creates a fresh execution context and replaces __main__ with new globals (pyre/pyrex/src/repl.rs:53-77). Consequently the prompt cannot inspect names defined by the target, and it starts after the original runtime has already been marked finalizing. PyPy instead passes the existing mainmodule to interactive_console and performs shutdown only after the prompt exits (app_main.py:1158-1172), so preserve the package target's context/module through the REPL and finalize afterward.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 13129-13133: Update the comment above emit_module_dict_cell_fold
to remove IntMutableCell from the unfoldable states, since
emit_namespace_cell_fold handles it via the live integer-field fold; describe
the false result generically as an absent name or present entry that could not
be folded.
In `@pyre/pyre-object/src/listobject.rs`:
- Around line 999-1008: After the `try_gc_alloc_collecting_rooted` call in the
allocation fallback, reload `items_block` from `block_root` before evaluating or
handling `raw.is_null()`. Ensure the boxed header initialization uses this
refreshed block address when allocation returns null, while preserving the
existing non-null allocation path.
In `@pyre/pyrex/src/lib.rs`:
- Around line 1284-1302: Root path_hooks before invoking hooks and retrieve the
rooted value from its shadow-stack slot on each iteration. Update the loop
around call_function_impl_result so w_list_getitem uses that rooted path_hooks
reference, while preserving the existing hook iteration and error handling.
- Around line 1354-1378: Update run_module’s error-handling tail to delegate to
handle_main_error, and route its successful finalization through finish_main
instead of duplicating the shutdown logic inline. Preserve the existing
exception-printing, runtime-finalization, JIT-statistics, keyboard-interrupt,
and process-exit ordering by passing the existing error, canonical object, and
execution context to these helpers.
🪄 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: 55fd3669-c347-4b97-953d-b5ab166a92ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/resume.rsmajit/majit-metainterp/src/virtualref.rspyre/bench/synth/bytes_split_whitespace_maxsplit.cranelift.jitstatspyre/bench/synth/bytes_split_whitespace_maxsplit.dynasm.jitstatspyre/bench/synth/bytes_split_whitespace_maxsplit.wasm.jitstatspyre/bench/synth/str_fstring.cranelift.linux.github-actions.jitstatspyre/check.pypyre/cpython_tests/baseline.jsonpyre/cpython_tests/run.pypyre/extra_tests/parity_tests/script_target_is_a_package.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_socket/interp_socket.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/tests/gc_stress.rspyre/pyre-object/src/listobject.rspyre/pyre-object/src/lltype.rspyre/pyrex/src/lib.rs
| // `emit_module_dict_cell_fold` returns `false` for BOTH an absent name and | ||
| // a present-but-unfoldable one (`IntMutableCell` / movable / strategy | ||
| // switched). Only an ABSENT name may fall through to the builtins fold — a | ||
| // a present-but-unfoldable one (`IntMutableCell` / strategy switched). | ||
| // Only an ABSENT name may fall through to the builtins fold — a | ||
| // present global shadows the builtin, so keep the residual (which reads the | ||
| // live globals slot) when the slot still exists. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the IntMutableCell fold description.
emit_module_dict_cell_fold forwards a non-null IntMutableCell to emit_namespace_cell_fold. That function has an is_int_cell branch that emits the live integer-field fold. Do not describe IntMutableCell as unfoldable here.
Use a generic description such as “a present entry that could not be folded,” or list only the actual non-foldable states.
🤖 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-jit-trace/src/jitcode_dispatch/specialize.rs` around lines 13129 -
13133, Update the comment above emit_module_dict_cell_fold to remove
IntMutableCell from the unfoldable states, since emit_namespace_cell_fold
handles it via the live integer-field fold; describe the false result
generically as an absent name or present entry that could not be folded.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ddf6efbd6
ℹ️ 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".
| match result { | ||
| Ok(true) => { | ||
| importing::restage_sys_path_0(std::ffi::OsStr::new(filename)); | ||
| import_site(no_site, canonical, ec_ptr); |
There was a problem hiding this comment.
When a directory or ZIP target is run and sitecustomize inspects __main__, this branch imports site while prepare_main_module has initialized only __name__ and __builtins__; the package path bypasses eval_source_in_main, so attributes such as __loader__, __spec__, and __package__ are not installed until runpy executes afterward. A customization that reads these standard attributes therefore sees missing state or raises during package-target startup. PyPy's app_main.py:869-880 initializes the main module and its loader before importing site, so seed the standard main-module state before this call.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🔇 Additional comments (4)
pyre/pyre-object/src/listobject.rs (4)
1039-1048: The Box fallback still stores the pre-collectionitems_block.
try_gc_alloc_collecting_rootedcan collect and relocate the pinned block before it returns null. Theraw.is_null()path at lines 1057-1069 writes the staleitems_blockinto the boxed header, because the reload fromblock_rootruns only at lines 1071-1073. Move the reload to immediately after the allocator call.Proposed fix
.filter(|p| !p.is_null()) .unwrap_or(std::ptr::null_mut()); + // Re-read the (possibly relocated) nursery items block after the header alloc. + if let Some(s) = block_root { + items_block = crate::gc_roots::shadow_stack_get(s) as *mut ItemsBlock; + } // `pop_roots` for the two typed blocks: this was the last allocation they- // Re-read the (possibly relocated) nursery items block after the header alloc. - if let Some(s) = block_root { - items_block = crate::gc_roots::shadow_stack_get(s) as *mut ItemsBlock; - } unsafe {
214-245: LGTM!Also applies to: 265-283, 295-310, 380-415, 435-435, 450-455, 805-819, 831-833, 1455-1457, 1473-1477
1032-1048: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that
header.w_classsurvives the now-collecting header allocation.Line 1015-1018 captures
header.w_class = get_instantiate(&LIST_TYPE)in a plain local before the allocation, and line 1074-1087 writes it into the new object. The previoustry_gc_alloc_stablepath never collected, so the capture could not go stale.try_gc_alloc_collecting_rootedcan run a collection. Ifget_instantiate(&LIST_TYPE)returns a movable object, this stores a stale pointer into every list header.Confirm that the returned
w_classis immortal. If it is not, pin it with the surroundingpush_rootsframe and rebuildheaderfrom the reloaded slot after the allocation.Also confirm the
try_gc_alloc_collecting_rootedparameter order and theneeds_write_barrieroutput contract, so thattruemeans "header placed outside the nursery".The learning about not reloading pointers after allocation applies to
try_gc_alloc_stable/try_gc_alloc_stable_rawonly, because those never collect; it does not cover this collecting allocator. Based on learnings.
1089-1095: LGTM!
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09665c49-d189-47f6-8250-f079bb5b3a3a
📒 Files selected for processing (1)
pyre/pyre-object/src/listobject.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pyre/pyre-interpreter/src/module/_socket/interp_socket.rs (4)
1020-1031: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCopy the fallback address before the next resolver call.
first_addrpoints into the process-globalhostentreturned bygethostbyname. The latergethostbyaddrcall can overwrite that resolver storage while it readsaddr_ptr. Copy the address bytes into caller-owned storage before callinggethostbyaddr.🤖 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/_socket/interp_socket.rs` around lines 1020 - 1031, In the resolver flow around first_addr and the subsequent gethostbyaddr call, copy the h.h_length bytes from first_addr into caller-owned storage before invoking gethostbyaddr, then pass the copied buffer as addr_ptr. Preserve the existing address type, length, and empty-address-list error handling.
55-56: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winHandle null pointer arrays before pointer arithmetic.
gethostbynamecalls this helper before it checksh_addr_listfor an empty entry. If the resolver supplies a nullh_addr_list,array.add(0)has undefined behavior beforeread_unalignedruns. Return a null pointer whenarray.is_null().🤖 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/_socket/interp_socket.rs` around lines 55 - 56, Update hostent_pointer_at to check array.is_null() before performing pointer arithmetic, returning a null character pointer for a null array; retain the existing unaligned read behavior for non-null arrays.
2062-2082: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the timeout deadline across EINTR.
Each EINTR retries
pollwith the original full timeout. A signal handler that returns normally can extend a finite socket timeout indefinitely. Compute a monotonic deadline once and poll only for the remaining duration.🤖 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/_socket/interp_socket.rs` around lines 2062 - 2082, Update the polling loop around the socket timeout logic to compute a monotonic deadline once before entering the loop, then recalculate and pass only the remaining timeout to libc::poll after EINTR retries. Preserve immediate timeout errors when the deadline is reached, while retaining signal checking and existing error handling for non-EINTR failures.
2582-2593: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the descriptor before invoking
py_repr_wtf8.
py_repr_wtf8(obj)can run a subclass__repr__. That code can close the socket and reusefdbefore Line 2592 closes the stale saved value. Mark_fdclosed and close the saved descriptor before formatting the warning.🤖 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/_socket/interp_socket.rs` around lines 2582 - 2593, Update the socket cleanup block around socket_get_attr_i64 and py_repr_wtf8 to mark the object’s _fd as closed and close the saved fd before invoking py_repr_wtf8. Then format and emit the ResourceWarning using the already-closed object state, preserving the existing warning behavior without closing a stale descriptor.
♻️ Duplicate comments (1)
pyre/pyre-object/src/listobject.rs (1)
1060-1094: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReload
items_blockbefore the boxed fallback.
try_gc_alloc_collecting_rootedcan collect and relocate the rooted items block, then still return null. In that case lines 1078-1090 build the boxed header from the pre-collectionitems_blockaddress. Move theblock_rootreload above theraw.is_null()branch.Proposed fix
.filter(|p| !p.is_null()) .unwrap_or(std::ptr::null_mut()); + // Re-read the (possibly relocated) nursery items block after the header alloc. + if let Some(s) = block_root { + items_block = crate::gc_roots::shadow_stack_get(s) as *mut ItemsBlock; + } storage.reload_typed_blocks(); @@ - // Re-read the (possibly relocated) nursery items block after the header alloc. - if let Some(s) = block_root { - items_block = crate::gc_roots::shadow_stack_get(s) as *mut ItemsBlock; - }🤖 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-object/src/listobject.rs` around lines 1060 - 1094, Move the block_root reload of items_block to immediately after try_gc_alloc_collecting_rooted returns, before the raw.is_null() fallback branch. Ensure both the boxed W_ListObject construction and the non-null allocation path use the potentially relocated ItemsBlock pointer.
🤖 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-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 10099-10107: Update the call to w_list_switch_to_strategy_for to
consume its returned relocated (list, value) pair and use those values when
setting the concrete oprefs, replacing the discarded result and redundant
shadow-stack reloads while preserving the existing tracing behavior.
In `@pyre/pyre-object/src/listobject.rs`:
- Around line 1040-1058: Update switch_to_object_strategy to root list before
boxing typed values with w_int_new or w_float_new, then reload it from the
caller’s shadow-stack slot after conversion. Ensure the typed append and insert
paths use this refreshed list reference before calling object_push or
recursively invoking w_list_insert.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/_socket/interp_socket.rs`:
- Around line 1020-1031: In the resolver flow around first_addr and the
subsequent gethostbyaddr call, copy the h.h_length bytes from first_addr into
caller-owned storage before invoking gethostbyaddr, then pass the copied buffer
as addr_ptr. Preserve the existing address type, length, and empty-address-list
error handling.
- Around line 55-56: Update hostent_pointer_at to check array.is_null() before
performing pointer arithmetic, returning a null character pointer for a null
array; retain the existing unaligned read behavior for non-null arrays.
- Around line 2062-2082: Update the polling loop around the socket timeout logic
to compute a monotonic deadline once before entering the loop, then recalculate
and pass only the remaining timeout to libc::poll after EINTR retries. Preserve
immediate timeout errors when the deadline is reached, while retaining signal
checking and existing error handling for non-EINTR failures.
- Around line 2582-2593: Update the socket cleanup block around
socket_get_attr_i64 and py_repr_wtf8 to mark the object’s _fd as closed and
close the saved fd before invoking py_repr_wtf8. Then format and emit the
ResourceWarning using the already-closed object state, preserving the existing
warning behavior without closing a stale descriptor.
---
Duplicate comments:
In `@pyre/pyre-object/src/listobject.rs`:
- Around line 1060-1094: Move the block_root reload of items_block to
immediately after try_gc_alloc_collecting_rooted returns, before the
raw.is_null() fallback branch. Ensure both the boxed W_ListObject construction
and the non-null allocation path use the potentially relocated ItemsBlock
pointer.
🪄 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: 97a4f69f-88ba-4653-8a36-041c4f1bf7b0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/resume.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_socket/interp_socket.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-object/src/listobject.rs
| let _ = unsafe { pyre_object::w_list_switch_to_strategy_for(inner_self, value) }; | ||
| inner_self = pyre_object::gc_roots::shadow_stack_get(root_base); | ||
| value = pyre_object::gc_roots::shadow_stack_get(root_base + 1); | ||
| ctx.trace_ctx.set_opref_concrete( | ||
| self_ref, | ||
| majit_ir::Value::Ref(majit_ir::GcRef(inner_self as usize)), | ||
| ); | ||
| ctx.trace_ctx | ||
| .set_opref_concrete(value_op, Value::Ref(majit_ir::GcRef(value as usize))); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider using the returned relocated pair.
w_list_switch_to_strategy_for now returns the forwarded (list, value) pair, and this call discards it with let _ =. The shadow-slot reload produces the same addresses, so behavior is equivalent. Consuming the return value keeps the new API contract visible at the call site and makes a future divergence between the two sources impossible.
Proposed refactor
- let _ = unsafe { pyre_object::w_list_switch_to_strategy_for(inner_self, value) };
- inner_self = pyre_object::gc_roots::shadow_stack_get(root_base);
- value = pyre_object::gc_roots::shadow_stack_get(root_base + 1);
+ (inner_self, value) =
+ unsafe { pyre_object::w_list_switch_to_strategy_for(inner_self, value) };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let _ = unsafe { pyre_object::w_list_switch_to_strategy_for(inner_self, value) }; | |
| inner_self = pyre_object::gc_roots::shadow_stack_get(root_base); | |
| value = pyre_object::gc_roots::shadow_stack_get(root_base + 1); | |
| ctx.trace_ctx.set_opref_concrete( | |
| self_ref, | |
| majit_ir::Value::Ref(majit_ir::GcRef(inner_self as usize)), | |
| ); | |
| ctx.trace_ctx | |
| .set_opref_concrete(value_op, Value::Ref(majit_ir::GcRef(value as usize))); | |
| (inner_self, value) = | |
| unsafe { pyre_object::w_list_switch_to_strategy_for(inner_self, value) }; | |
| ctx.trace_ctx.set_opref_concrete( | |
| self_ref, | |
| majit_ir::Value::Ref(majit_ir::GcRef(inner_self as usize)), | |
| ); | |
| ctx.trace_ctx | |
| .set_opref_concrete(value_op, Value::Ref(majit_ir::GcRef(value as usize))); |
🤖 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-jit-trace/src/jitcode_dispatch/specialize.rs` around lines 10099 -
10107, Update the call to w_list_switch_to_strategy_for to consume its returned
relocated (list, value) pair and use those values when setting the concrete
oprefs, replacing the discarded result and redundant shadow-stack reloads while
preserving the existing tracing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-interpreter/src/builtins.rs (2)
7624-7626: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a prebuilt text for the empty result.
PrebuiltText::getis valid for Python return values. It lazily creates one immortalW_UnicodeObjectwithw_str_newand returns the same object on later calls. Replace the repeated allocation in thelen == 0branch with a function-local staticPrebuiltText.🤖 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/builtins.rs` around lines 7624 - 7626, Update the len == 0 branch in the surrounding builtin function to use a function-local static PrebuiltText initialized with w_str_new, returning PrebuiltText::get() instead of allocating a new empty string on each call. Preserve the existing Ok return behavior.
9434-9443: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRoot and reload the exception receiver across formatting dispatch
exc_user_dunder_objcallscall_and_check, whileexception_kind_str_wtf8callspy_repr_wtf8andpy_str_wtf8. These calls can execute Python and trigger GC. Root and reload the receiver before later accesses:
builtins.rs#L9434-L9443: reloadobjbefore passing it toexception_str_method.builtins.rs#L7651-L7660: pass the reloaded receiver tobase_exception_str_method, not the originalargsslice.display.rs#L1505-L1513: reloadobjafterexc_user_dunder_objbefore reading its kind or fields.- Apply the same reload pattern inside
exception_kind_str_wtf8after formatting calls and before subsequent exception-slot accesses.🤖 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/builtins.rs` around lines 9434 - 9443, Root and reload exception receivers after Python-executing formatting calls before subsequent access: in builtins.rs lines 9434-9443, reload obj before passing it to exception_str_method; in builtins.rs lines 7651-7660, pass the reloaded receiver to base_exception_str_method instead of the original args slice and apply the same reload pattern in exception_kind_str_wtf8 after formatting calls; in display.rs lines 1505-1513, reload obj after exc_user_dunder_obj before reading its kind or fields.
🤖 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/bench/synth/str_fstring.dynasm.jitstats`:
- Line 11: Restore the shared guard_failures baseline in
str_fstring.dynasm.jitstats to 658, and leave 659 only in the Darwin GitHub
Actions overlay file.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 7624-7626: Update the len == 0 branch in the surrounding builtin
function to use a function-local static PrebuiltText initialized with w_str_new,
returning PrebuiltText::get() instead of allocating a new empty string on each
call. Preserve the existing Ok return behavior.
- Around line 9434-9443: Root and reload exception receivers after
Python-executing formatting calls before subsequent access: in builtins.rs lines
9434-9443, reload obj before passing it to exception_str_method; in builtins.rs
lines 7651-7660, pass the reloaded receiver to base_exception_str_method instead
of the original args slice and apply the same reload pattern in
exception_kind_str_wtf8 after formatting calls; in display.rs lines 1505-1513,
reload obj after exc_user_dunder_obj before reading its kind or fields.
🪄 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: 35093457-2530-49a3-b930-d4544a8b958d
📒 Files selected for processing (3)
pyre/bench/synth/str_fstring.cranelift.jitstatspyre/bench/synth/str_fstring.dynasm.jitstatspyre/pyre-interpreter/src/builtins.rs
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=658 | ||
| guard_failures=659 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n pyre/bench/synth/str_fstring.dynasm.jitstats
printf '%s\n' '--- baseline selection references ---'
rg -n -C 8 'jitstats|guard_failures|platform|macos|ubuntu|windows' pyre/check.py
printf '%s\n' '--- related baseline files ---'
find pyre/bench/synth -maxdepth 2 -type f -name '*.jitstats' -print | sortRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- str_fstring baseline variants ---'
find pyre/bench -type f -name 'str_fstring*.jitstats' -print -exec sh -c 'echo "--- $1"; cat -n "$1"' sh {} \;
printf '%s\n' '--- relevant selection and comparison code ---'
sed -n '1531,1582p' pyre/check.py
sed -n '1610,1663p' pyre/check.py
printf '%s\n' '--- benchmark call sites for str_fstring ---'
rg -n -C 4 'str_fstring' pyre/check.py pyre/benchRepository: youknowone/pyre
Length of output: 11967
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- jit-stats comparison semantics ---'
sed -n '919,1006p' pyre/check.py
printf '%s\n' '--- baseline selection behavior for representative runners ---'
python3 - <<'PY'
from pathlib import Path
import os
source = Path("pyre/bench/synth/str_fstring.py")
backend = "dynasm"
for github, platform in [
("true", "linux"),
("true", "win32"),
("true", "darwin"),
("false", "linux"),
("false", "darwin"),
]:
candidates = []
if github == "true":
candidates.append(source.with_name(
f"{source.stem}.{backend}.{platform}.github-actions.jitstats"
))
candidates.append(source.with_name(
f"{source.stem}.{backend}.{platform}.jitstats"
))
candidates.append(source.with_name(
f"{source.stem}.{backend}.jitstats"
))
chosen = next((p for p in candidates if p.exists()), candidates[-1])
value = None
if chosen.exists():
for line in chosen.read_text().splitlines():
if line.startswith("guard_failures="):
value = line.split("=", 1)[1]
print(f"GITHUB_ACTIONS={github} sys.platform={platform}: {chosen} guard_failures={value}")
PYRepository: youknowone/pyre
Length of output: 5401
Restore the shared DynASM baseline to 658.
pyre/check.py uses this file on ubuntu-24.04 and windows-latest; both report guard_failures=658. The macOS GitHub Actions overlay reports 659. With the shared value set to 659, common runners classify their 658 result as a regression, and a later 659 result can hide drift from the measured baseline.
Keep 659 only in pyre/bench/synth/str_fstring.dynasm.darwin.github-actions.jitstats.
Proposed correction
-guard_failures=659
+guard_failures=658📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guard_failures=659 | |
| guard_failures=658 |
🤖 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/bench/synth/str_fstring.dynasm.jitstats` at line 11, Restore the shared
guard_failures baseline in str_fstring.dynasm.jitstats to 658, and leave 659
only in the Darwin GitHub Actions overlay file.
2be9988 to
1b8c6ce
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/1b8c6ce3c9dde0c51c7e52f5a72716e465eaa724/pyre-object/src/listobject.rs#L762-L764
Once another mutator has existed, either pin_root call can wait behind a minor collection; the collector and pin_root update the shadow-stack slot, but not the raw w_item local. The subsequent is_plain_int1(w_item) and is_float_strategy_item(w_item) therefore dereference the evacuated nursery address during an empty-list append or insert, potentially selecting the wrong strategy or accessing reused memory. Reload w_item from root_base + 1 before performing these type checks.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/0bc57490c393de7f580de60ce1d6a079babf7d1d/pyre-object/src/listobject.rs#L1068-L1069
When the collecting GC hook is installed but returns null, this collapses Some(null) together with None and takes the unmanaged Box fallback. The hook contract in gc_hook.rs explicitly distinguishes these cases: Some(null) means the active GC failed and must abort, because a headerless object must not enter its managed graph. Moreover, the failed collecting attempt may already have relocated the Object-strategy items block, while the fallback stores the stale pre-collection items_block because its root is reloaded only after this branch. Under allocation pressure this can therefore construct a list with reclaimed storage rather than reporting allocation failure; classify the result with GcAllocOutcome::from_hook(...).allocated_or_abort(...) and reserve boxing for NoRoute.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f1759938c
ℹ️ 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".
| if !unsafe { pyre_object::is_list(path_hooks) } { | ||
| return Ok(false); |
There was a problem hiding this comment.
Once site is imported before this probe, a sitecustomize that replaces sys.path_hooks with a tuple or another iterable causes this exact-list gate to return false without invoking any hooks. PyPy uses generic iteration at pypy/interpreter/app_main.py:1054, so the directory or ZIP should still be accepted; here it instead falls into read_script_source and attempts to execute the target itself as source. Iterate through the Python iterator protocol rather than requiring W_ListObject.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 992e1aea6e
ℹ️ 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".
| hook, | ||
| &[shadow_stack_get(filename_slot)], | ||
| ) { | ||
| Ok(_) => return Ok(true), |
There was a problem hiding this comment.
When a path hook returns None, this branch marks the target as accepted and later runs runpy against it. PyPy's app_main.py:1054-1062 stores the hook result in importer and treats None as no importer, so a custom hook that explicitly declines this way should allow an ordinary source file to follow the source-execution path; here it instead becomes sys.path[0] and fails looking for __main__. Check the returned object for None before returning true.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| if pos > data.len() { | ||
| data.resize(pos, 0); |
There was a problem hiding this comment.
On the non-fd, non-Unix pathname fallback, Python can seek to a very large valid offset and then write a byte. This infallible Vec::resize attempts to allocate the entire hole and can panic or abort the interpreter instead of raising a Python exception; the subsequent checked_add runs too late to protect it. Validate the position and use fallible reservation before resizing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/180b2cae2a4b98e8d3e9cec63f59dce34e9ebd09/pyre-interpreter/src/builtins.rs#L16210
When an in-memory pathname stream seeks beyond EOF and writes an empty buffer, end == pos, so this branch resizes the backing buffer and flushes a zero-filled hole even though a zero-byte write must leave the file size unchanged (PyPy's interp_bytesio.py:123-126 returns before writing, while FileIO delegates the empty buffer to os.write). Return without growing when bytes.is_empty(), while preserving the current position.
AGENTS.md reference: AGENTS.md:L231-L232
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ed438bf80
ℹ️ 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".
| // `for hook in sys.path_hooks` is a plain iteration, so a `sitecustomize` | ||
| // that replaces the list with a tuple or any other iterable still gets its | ||
| // hooks called. | ||
| let hooks = pyre_interpreter::baseobjspace::unpackiterable(path_hooks, -1)?; |
There was a problem hiding this comment.
When sys.path_hooks is a lazy iterable, such as a generator installed by sitecustomize, the generic-iterable follow-up now exhausts it with unpackiterable before calling any hook. For example, if it yields an accepting hook first and raises on the next iteration, PyPy calls the first hook and breaks at app_main.py:1054-1057, while this code raises before the accepting hook runs and aborts target startup. Iterate and invoke hooks interleaved so acceptance stops iteration immediately.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21ce002a6c
ℹ️ 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".
| import_site(no_site, canonical, ec_ptr); | ||
| if let Err(e) = runpy_run_module_as_main(canonical, ec_ptr, "__main__", false) { |
There was a problem hiding this comment.
When a directory or ZIP target contains a top-level runpy.py, import_site first consumes the restaged entry via add_sys_path_0, and the following runpy_run_module_as_main imports runpy through the now-shadowed live sys.path. This executes the target's runpy.py or fails when it lacks _run_module_as_main, instead of running __main__.py. PyPy imports the standard-library runpy at app_main.py:1102 before inserting the target at line 1103, so preserve that ordering.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
`RunMode::Script` read and compiled the file before the interpreter was up, so
a directory exited with `cannot open '<path>': Is a directory` and a zipfile
was decoded as source text. Both oracles instead put the path on `sys.path[0]`
and run the `__main__` module it contains.
Choosing between the two requires `sys.path_hooks`, which only exists after
`init_importlib_bootstrap`. The script run now creates `__main__`'s globals
with `fresh_module_globals`, imports `sys` and installs the bootstrap, and only
then walks `sys.path_hooks` with the absolutized filename, as
`app_main.py:1054-1105` does; the read and compile follow on the branch that
needs them. `PyFrame::new_with_context_and_globals` builds the frame over the
globals the bootstrap already ran in, and `new_with_context` delegates to it.
A hook that accepts the path restages the pending `sys.path[0]` entry to it and
runs `runpy._run_module_as_main("__main__", False)`. Only `ImportError` moves
on to the next hook. The restaged entry is set even under the safe-path flags,
which suppress the entry for an ordinary script.
`is_keyboard_interrupt` and the new `is_import_error` share
`raised_is_instance_of`.
test_zipimport_support: `FAILED (failures=2)` -> `Ran 4 tests, OK`.
Assisted-by: Claude
… statuses 22 modules were recorded `dynasm: PASS` with no cranelift entry, so a cranelift-backed gate run deselected them. Each was run under cranelift and re-confirmed at the nightly's own `--timeout 120`. test.test_datetime is the one module of those 23 left unrecorded: it aborts at `minor_custom_trace_target` during a nursery collection entered from cranelift code, but not on every run — the same binary and environment gave rc=1 under the runner and rc=134 standalone. test.test_zipimport and test.test_zipimport_support were both recorded IMPORTERROR. They import: the support module now passes (4 tests, three consecutive runs), and test_zipimport runs all 91 tests with one error, `testZip64LargeFile`. Assisted-by: Claude
`interp_func.py:405-411` takes a bytes name as it stands and fsencodes a str one, so a byte with no UTF-8 spelling reaches the syscall as itself; anything else is a TypeError naming those two types. `fsencode_w` also accepts a `__fspath__` object, which this entry point does not. The pinned RustPython rev this commit used to carry is now behind the base's own, which contains it, so only this hunk remains. Assisted-by: Claude
Left as None, `test.support.use_resources` enables every resource, so the module's Zip64 arms build archives past 4 GiB: it took 290s and over 6 GB of RSS against the runner's 300s timeout. With the empty set libregrtest and PyPy's conftest use, it runs 91 tests in 0.3s and passes on both backends, which the baseline now records. Assisted-by: Claude
`grow_list_items_block_gc` asked `try_gc_owns_object` whether the old block was GC-owned before rooting it. That query's cross-thread path can wait behind a collection, which relocates the block; the copy then read reclaimed nursery slots and installed stale item pointers in the new block, which a later remembered-set walk read as an invalid type_id. Root `old` unconditionally instead. The shadow-stack walker ignores non-GC addresses, so rooting the `std::alloc` fallback is safe and removes the ownership-query safepoint entirely. Assisted-by: Claude
The non-fd file fallback appended every write. `zipfile` writes a provisional local header, then seeks back and patches it, so appending left the provisional bytes in place and produced an archive whose central directory pointed at a non-`PK\x03\x04` local header; `zipimport._get_data` then rejected it as a bad local file header. Track the write position and overwrite at it, keeping append mode on the end-of-data path. `file_write_at` is target-neutral so the seek-back behavior is covered by the ordinary test build. Assisted-by: Claude
`w_str_new` allocates its value with `malloc_raw`, which carries no GC header and is never paired with a `Box::from_raw`, so every string a per-call function returned leaked its payload. `category`, `bidirectional`, `east_asian_width`, `decomposition`, `name` and `lookup` build a fresh string on each call, so they take `w_str_new_managed`; the module-level version attributes stay immortal. test_unicodedata peaks at 1379 MB rather than 2086 MB, with its 15 failures and 1 skip unchanged. Assisted-by: Claude
The win32 dynasm leg reports 658 where the recorded baseline holds 659, matching the cranelift baseline already re-recorded for this branch. Assisted-by: Claude
`app_main.py:1054-1062` walks `for hook in sys.path_hooks` and keeps `importer = hook(filename)` from the first hook that does not raise `ImportError`, treating `importer is None` as "no importer claimed the path". The probe instead required `sys.path_hooks` to be an exact list, so a `sitecustomize` replacing it with another iterable silently skipped every hook, and it accepted any non-raising hook, including one that declines by returning None. Assisted-by: Claude
Seeking to a large offset and writing one byte asks the fallback backing to materialize the whole hole. The infallible growth aborts the process; reserve first so the request raises MemoryError. Assisted-by: Claude
A hook call runs arbitrary Python and can drive a collection that moves the hook objects. The unpacked hooks lived in a Rust vector, which the root walker does not scan, so publish each one in a shadow-stack slot and read it back for the call. Assisted-by: Claude
`file_write_at` resized the backing buffer up to the seek position for a zero-length write, so writing `b''` past the end zero-filled the hole and changed the file size. `interp_bytesio.py:124-125` returns before writing when the buffer is empty. Assisted-by: Claude
…ckage `run.py` refuses to enumerate the suite unless every script carries `# CPython-suite gap:` and `# parity-tests reason:` in its first 20 lines, so the whole parity run aborted with a RuntimeError. Assisted-by: Claude
`path_hook_accepts` drained the iterable with `unpackiterable` before calling any hook, so an iterable that raises after yielding an accepting hook aborted startup. `app_main.py:1054-1057` breaks out of the `for` on the first hook that does not raise `ImportError`, without asking the iterator for another item. Assisted-by: Claude
The directory/zipfile arm reached `import site` with `__main__` carrying only `__name__`, so a `sitecustomize` that reads `__main__.__loader__` raised. `app_main.py:869-870` binds it at module creation, ahead of the `app_main.py:875-882` site import. Assisted-by: Claude
…refix `canonicalize` answers in the `\\?\` form on Windows, so `sys.path[0]` for a script carried a prefix no upstream entry has — `resolvedirof` (`initpath.py:66-78`) builds it with `rabspath`. A program comparing the entry against `os.path.dirname(__file__)` saw two different spellings. Assisted-by: Claude
1 participant
Summary
runpywhile preserving the canonical__main__frame/globalstest_zipimportValidation
cargo check --features dynasmcargo test --features dynasmpython3 pyre/check.py target/release/pyre-dynasm --backend dynasm --no-synthetic --no-cpython-suite(17/17passed)Summary by CodeRabbit
New Features
runpysupport.socket.sethostname()now accepts strings and bytes.Bug Fixes
=alignment.Tests