gc: #396 globalize per-thread GC hook fn-ptr cells - #447
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (28)
WalkthroughThe change migrates GC hooks to process-global storage, adds reentrant synchronization and multi-mutator root traversal, routes production backends through ChangesGC synchronization and collection
Backend and runtime integration
Captured root areas
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PyJit
participant gc_sync
participant MiniMarkGC
participant MutatorRoots
PyJit->>gc_sync: initialize and register mutator
PyJit->>gc_sync: request collection
gc_sync->>MutatorRoots: quiesce and walk captured root areas
MutatorRoots-->>MiniMarkGC: forwarded roots
MiniMarkGC->>gc_sync: complete collection
gc_sync-->>PyJit: resume execution
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 54f8e94). 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9561387149
ℹ️ 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".
| pub fn install_gc_standalone() { | ||
| let mut handle: Box<dyn majit_gc::GcAllocator> = Box::new(majit_gc::GcHandle); | ||
| handle.freeze_types(); | ||
| install_gc_box(handle); | ||
| majit_gc::gc_sync::gc_op(|gc| gc.freeze_types()); | ||
| let supports_guard_gc_type = majit_gc::gc_sync::gc_query(|gc| gc.supports_guard_gc_type()); | ||
| register_active_hooks(supports_guard_gc_type); |
There was a problem hiding this comment.
New through gc_sync after dropping TLS
When a Dynasm consumer enables set_new_via_gc(true), compiled New/NewWithVtable still call dynasm_new_alloc, which only checks DYNASM_ACTIVE_GC and falls back to libc::malloc on None. This production install path now deliberately leaves that TLS empty, so those compiled allocations stop going through the GC singleton while the rest of the runtime assumes GC-managed objects; please either keep a forwarding handle for this path or make dynasm_new_alloc use the same gc_sync fallback as the other trampolines.
Useful? React with 👍 / 👎.
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 (1)
pyre/pyre-jit/src/eval.rs (1)
2305-2307: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate this stale thread-local hook comment.
These hooks now write process-global fn-pointer cells, so the comment still saying “thread-local
Cellslots” contradicts the newPYRE_OBJECT_HOOKS_INSTALLED.call_once(...)path.Proposed wording
-/// pyre-object GC hook trampolines — safe to install at boot because -/// they only store function pointers in pyre-object's thread-local -/// `Cell` slots and do not touch interpreter state. +/// pyre-object GC hook trampolines — safe to install once at boot because +/// they only store function pointers in pyre-object's process-global +/// hook cells and do not touch interpreter state.🤖 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/src/eval.rs` around lines 2305 - 2307, Update the stale hook comment near the pyre-object GC hook trampoline setup in eval.rs so it matches the current PYRE_OBJECT_HOOKS_INSTALLED.call_once(...) behavior. The comment currently says the hooks only store function pointers in thread-local Cell slots, but that is no longer true; revise it to describe the process-global fn-pointer cells and keep the rest of the safety explanation aligned with the actual install path.
🤖 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-object/src/gc_hook.rs`:
- Around line 285-289: The root hook registration order in
register_gc_root_hooks is unsafe because try_gc_add_root may observe
GC_ADD_ROOT_HOOK before GC_REMOVE_ROOT_HOOK is visible, causing
try_gc_remove_root to fail later. Update register_gc_root_hooks so the
remove-root hook is published first, then the add-root hook, and keep the
behavior localized to the GC_ADD_ROOT_HOOK / GC_REMOVE_ROOT_HOOK setters in
gc_hook::register_gc_root_hooks.
- Around line 12-14: Update the documentation comment in gc_hook.rs to remove
the misleading “every thread installs the same pointer” wording and instead
state that the pyre-object GC hook is installed once via std::sync::Once, with
all threads observing the same global hook pointer afterward. Keep the
explanation aligned with the gc_hook installation path and the
majit_gc::global_hook behavior so the comment accurately reflects the singleton
install semantics.
---
Outside diff comments:
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 2305-2307: Update the stale hook comment near the pyre-object GC
hook trampoline setup in eval.rs so it matches the current
PYRE_OBJECT_HOOKS_INSTALLED.call_once(...) behavior. The comment currently says
the hooks only store function pointers in thread-local Cell slots, but that is
no longer true; revise it to describe the process-global fn-pointer cells and
keep the rest of the safety explanation aligned with the actual install path.
🪄 Autofix (Beta)
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
Run ID: 356c25f2-3a23-42ee-b37d-cdcee31b6be8
📒 Files selected for processing (11)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-gc/src/gc_sync.rsmajit/majit-gc/src/hook_cell.rsmajit/majit-gc/src/lib.rsmajit/majit-metainterp/src/pyjitpl.rspyre/pyre-interpreter/src/eval.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/gc_hook.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f05733c8f8
ℹ️ 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".
| // Assertions on a mock's *invocation* are a separate concern: the alloc and | ||
| // root hooks are read only behind `gc_interp::enabled()` / a registered | ||
| // backend type id, both off by default in this binary, so no concurrent | ||
| // production path invokes those mocks — their arg-recording statics are safe |
There was a problem hiding this comment.
When pyre-object tests run in parallel, this lock only serializes the hook tests themselves, but the mocked cells are now process-global. While invokes_registered_hook_with_args or stable_hook_is_independent_from_nursery_hook has mock_hook installed, any concurrently running pyre-object test that constructs a GC-routed dict/items block/weakref can hit try_gc_alloc (for example object_array.rs:290 or dictmultiobject.rs:881) and receive the dummy pointer returned by the mock instead of falling back to the normal allocator. That was impossible with thread-local hooks and can make unrelated tests dereference bogus addresses, so the mock installs need to be isolated from all allocation-capable tests, not just this module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85dc8998e4
ℹ️ 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".
| }) { | ||
| return r; | ||
| } | ||
| majit_gc::gc_sync::gc_op(|g| g.alloc_nursery_no_collect_typed(type_id, size)) |
There was a problem hiding this comment.
When DynasmBackend::set_gc_allocator installs these now-process-global hooks without a gc_sync singleton, any call from a thread that has no DYNASM_ACTIVE_GC box reaches this fallback and gc_sync::gc_op panics in singleton_mut() instead of returning the documented null/no-GC result. This regresses the old per-thread behavior where an uninstalled thread saw no hook and fell back normally; please guard gc_sync::is_initialized() here (or route through with_dynasm_active_gc_mut(...).unwrap_or(GcRef(0))) before assuming the singleton exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a99baad62
ℹ️ 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".
| Ok(None) => majit_gc::gc_sync::gc_query_reentrant(|g| g.is_managed_heap_object(addr)), | ||
| Err(_) => CRANELIFT_ACTIVE_GC_RAW.with(|raw| match raw.get() { | ||
| Some(ptr) => unsafe { (&*ptr).is_managed_heap_object(addr) }, | ||
| None => majit_gc::gc_sync::gc_query_reentrant(|g| g.is_managed_heap_object(addr)), |
There was a problem hiding this comment.
When a CraneliftBackend that installed a test GC is dropped, Drop clears CRANELIFT_ACTIVE_GC but the now-process-global ACTIVE_GC_OWNS_OBJECT hook remains installed. A later no-GC ownership query on that thread reaches the Ok(None)/raw-None branches here and unconditionally calls gc_query_reentrant, which panics if the process never initialized the gc_sync singleton; before this change the same absent-runtime case returned false. Please preserve the no-GC fallback by checking gc_sync::is_initialized() before querying the singleton.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbd031a891
ℹ️ 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 REGISTERED_THREADS.load(Ordering::Acquire) <= 1 { |
There was a problem hiding this comment.
This fast return assumes registered_threads() <= 1 means the collecting thread is the only mutator, but gc_op now explicitly permits unregistered callers. If an unregistered helper/backend thread triggers collect_nursery or collect_full while exactly one Python mutator is registered, this returns an inactive guard; the collector then uses the single-thread walk_my_* paths and neither parks nor scans the real mutator, so its nursery roots can be moved or swept while it is still running. Please only take this shortcut when the current thread is the sole registered running mutator; otherwise drain the registered thread.
Useful? React with 👍 / 👎.
| majit_gc::shadow_stack::register_mutator(); | ||
| register_thread_root_areas(); | ||
| majit_gc::gc_sync::register_thread(); |
There was a problem hiding this comment.
These calls make the current thread's TLS root areas visible in MUTATOR_REGISTRY before gc_sync::register_thread() adds the thread to the RUNNING count. In a multi-threaded startup, a collector on another registered thread can enter STW in this window, see this new registry entry in walk_all_extra_areas, but not wait for this thread because it is not counted yet; the *_area walkers then dereference this thread's RefCell/TLS storage while the owner is still running, violating their safety contract. The root-area registration needs to be atomic with, or after, the quiescence registration in a way that collectors cannot observe an uncounted mutator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-gc/src/collector.rs (1)
2218-2239: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winQuiesce mutators here as well
do_collect_oldgen_nonmovingcan run with other registered mutators still active, soseed_major_roots()may miss their roots andoldgen.sweep()can reclaim a still-reachable object. Mirrordo_collect_nursery/do_collect_fulland wrap this path inquiesce_mutators().🤖 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 `@majit/majit-gc/src/collector.rs` around lines 2218 - 2239, Wrap the entire collection workflow in do_collect_oldgen_nonmoving with quiesce_mutators(), including root seeding, marking, finish_incremental_cycle(), and the subsequent old-generation sweep. Mirror the mutator-quiescing pattern used by do_collect_nursery and do_collect_full, ensuring the guard remains active until all collection work that depends on a consistent root set is complete.
🤖 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.
Outside diff comments:
In `@majit/majit-gc/src/collector.rs`:
- Around line 2218-2239: Wrap the entire collection workflow in
do_collect_oldgen_nonmoving with quiesce_mutators(), including root seeding,
marking, finish_incremental_cycle(), and the subsequent old-generation sweep.
Mirror the mutator-quiescing pattern used by do_collect_nursery and
do_collect_full, ensuring the guard remains active until all collection work
that depends on a consistent root set is complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a6b342b-f31c-4b72-bec2-87c4bf4b59c7
📒 Files selected for processing (29)
gc-stw.plan.mdmajit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/gc_sync.rsmajit/majit-gc/src/hook_cell.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-metainterp/src/pyjitpl.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_codecs/mod.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/pycode.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/gc_roots.rspyre/pyre-object/src/interp_sre.rspyre/pyre-object/src/weakref.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f647696336
ℹ️ 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".
| thread_local! { | ||
| static GC_COLLECT_OLDGEN_HOOK: Cell<Option<GcCollectOldgenHookFn>> = const { Cell::new(None) }; | ||
| } | ||
| majit_gc::global_hook!(static GC_COLLECT_OLDGEN_HOOK: GcCollectOldgenHookFn); |
There was a problem hiding this comment.
With this hook now process-global, any thread's gc_interp safepoint can invoke collect_oldgen_nonmoving on the shared singleton. MiniMarkGC::do_collect_oldgen_nonmoving does not call quiesce_mutators, so seed_major_roots observes mutators_quiesced() == false and walks only the collecting thread's roots; in a two-mutator run, old-gen objects reachable only from another registered frame can be left unmarked and swept. Please wrap the non-moving major in the same STW guard as the other collection paths before exposing it through the global hook.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b02e9191d1
ℹ️ 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".
| static IMPORT_ROOT_AREA: ImportRootArea = ImportRootArea { | ||
| modules: SYS_MODULES.with(|modules| modules as *const _), | ||
| modules_dict: SYS_MODULES_DICT.with(|dict| dict as *const _), | ||
| }; |
There was a problem hiding this comment.
When pyrex sets argv before run_source, set_sys_argv creates a GC-managed list and stores it only in SYS_ARGV_PENDING until the sys module later drains it with take_pending_sys_argv. This new per-thread import root area captures sys.modules and its dict but not that pending argv cell, so a collection in this startup window can sweep the old-gen list or leave its nursery backing block unforwarded before sys.argv is installed. Please include SYS_ARGV_PENDING in the captured/walked import roots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-gc/src/collector.rs (1)
1730-1790: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftQuiesce multi-mutator incremental GC before seeding or sweeping.
seed_major_rootsscans all threads only when STW is already active, butgc_stepat Lines 2581-2593 can start and finish a global major cycle without quiescing. Objects rooted only by another mutator can therefore be omitted and swept. Acquire STW for multi-threadedgc_stepwork, including cycle completion, and add a cross-thread-root regression test.🤖 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 `@majit/majit-gc/src/collector.rs` around lines 1730 - 1790, Update the major-GC flow around seed_major_roots and gc_step so multi-mutator incremental collection acquires and maintains stop-the-world quiescence before scanning roots, seeding, sweeping, and completing a global cycle. Ensure the guard covers the full gc_step operation, including cycle completion, while preserving the existing single-mutator behavior; add a regression test proving objects rooted exclusively by another mutator survive.
🤖 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 `@majit/majit-backend-wasm/src/lib.rs`:
- Around line 503-527: Update wasm_jit_ca_alloc_frame to access the active
collector through with_wasm_active_gc_mut instead of directly borrowing
WASM_ACTIVE_GC. Preserve the existing nursery allocation, zero-handle fallback,
JitFrame initialization, shadow-stack push, and returned pointer behavior while
ensuring standalone GC installation works when CA lowering is enabled.
In `@majit/majit-gc/src/gc_sync.rs`:
- Around line 235-266: Update register_thread to serialize registration with
ongoing GC fast-path activity and quiesce existing mutators before publishing
the new thread. Ensure both the 0→1 and 1→2 transitions acquire the operation
gate represented by IN_FAST_PATH, wait for active GC work to finish, and only
then update GC_SYNC running/registered state or call gc_op to disable inline
allocation. Preserve the existing duplicate-registration assertion and waiting
behavior for stw_requested.
- Around line 342-419: Update gc_op_slow and the fast-path operation around
OpGuard::enter so IN_FAST_PATH ownership is managed by an RAII guard whose Drop
clears the gate with Release ordering. Ensure the guard is created immediately
after each successful gate acquisition and remains active through f, so
unwinding releases the gate while preserving existing reentrant
self_holds_fast_path behavior.
- Around line 195-216: Update gc_query_reentrant so its in_gc_op() branch does
not call singleton_ref_reentrant or create a shared reference directly from
GC_STORE while gc_op holds the mutable borrow. Reuse the active &mut borrow with
a callback-scoped reborrow, or pass the singleton through raw pointers, while
preserving the read-only callback behavior and the gc_query path for top-level
calls.
In `@majit/majit-gc/src/shadow_stack.rs`:
- Around line 258-293: Update register_mutator_extra_area to make the extra-area
data lifetime boundary explicit, preferably by marking the API unsafe and
documenting that callers must keep data valid until unregister_mutator or thread
cleanup; preserve the existing registration behavior and ownership-thread
restriction.
---
Outside diff comments:
In `@majit/majit-gc/src/collector.rs`:
- Around line 1730-1790: Update the major-GC flow around seed_major_roots and
gc_step so multi-mutator incremental collection acquires and maintains
stop-the-world quiescence before scanning roots, seeding, sweeping, and
completing a global cycle. Ensure the guard covers the full gc_step operation,
including cycle completion, while preserving the existing single-mutator
behavior; add a regression test proving objects rooted exclusively by another
mutator survive.
🪄 Autofix (Beta)
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
Run ID: d41dca32-23f4-45cb-bdf0-1c80c81cf87f
📒 Files selected for processing (28)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/gc_sync.rsmajit/majit-gc/src/hook_cell.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-metainterp/src/pyjitpl.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_codecs/mod.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/pycode.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/gc_roots.rspyre/pyre-object/src/interp_sre.rspyre/pyre-object/src/weakref.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-gc/src/collector.rs (1)
1730-1790: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftQuiesce multi-mutator incremental GC before seeding or sweeping.
seed_major_rootsscans all threads only when STW is already active, butgc_stepat Lines 2581-2593 can start and finish a global major cycle without quiescing. Objects rooted only by another mutator can therefore be omitted and swept. Acquire STW for multi-threadedgc_stepwork, including cycle completion, and add a cross-thread-root regression test.🤖 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 `@majit/majit-gc/src/collector.rs` around lines 1730 - 1790, Update the major-GC flow around seed_major_roots and gc_step so multi-mutator incremental collection acquires and maintains stop-the-world quiescence before scanning roots, seeding, sweeping, and completing a global cycle. Ensure the guard covers the full gc_step operation, including cycle completion, while preserving the existing single-mutator behavior; add a regression test proving objects rooted exclusively by another mutator survive.
🤖 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 `@majit/majit-backend-wasm/src/lib.rs`:
- Around line 503-527: Update wasm_jit_ca_alloc_frame to access the active
collector through with_wasm_active_gc_mut instead of directly borrowing
WASM_ACTIVE_GC. Preserve the existing nursery allocation, zero-handle fallback,
JitFrame initialization, shadow-stack push, and returned pointer behavior while
ensuring standalone GC installation works when CA lowering is enabled.
In `@majit/majit-gc/src/gc_sync.rs`:
- Around line 235-266: Update register_thread to serialize registration with
ongoing GC fast-path activity and quiesce existing mutators before publishing
the new thread. Ensure both the 0→1 and 1→2 transitions acquire the operation
gate represented by IN_FAST_PATH, wait for active GC work to finish, and only
then update GC_SYNC running/registered state or call gc_op to disable inline
allocation. Preserve the existing duplicate-registration assertion and waiting
behavior for stw_requested.
- Around line 342-419: Update gc_op_slow and the fast-path operation around
OpGuard::enter so IN_FAST_PATH ownership is managed by an RAII guard whose Drop
clears the gate with Release ordering. Ensure the guard is created immediately
after each successful gate acquisition and remains active through f, so
unwinding releases the gate while preserving existing reentrant
self_holds_fast_path behavior.
- Around line 195-216: Update gc_query_reentrant so its in_gc_op() branch does
not call singleton_ref_reentrant or create a shared reference directly from
GC_STORE while gc_op holds the mutable borrow. Reuse the active &mut borrow with
a callback-scoped reborrow, or pass the singleton through raw pointers, while
preserving the read-only callback behavior and the gc_query path for top-level
calls.
In `@majit/majit-gc/src/shadow_stack.rs`:
- Around line 258-293: Update register_mutator_extra_area to make the extra-area
data lifetime boundary explicit, preferably by marking the API unsafe and
documenting that callers must keep data valid until unregister_mutator or thread
cleanup; preserve the existing registration behavior and ownership-thread
restriction.
---
Outside diff comments:
In `@majit/majit-gc/src/collector.rs`:
- Around line 1730-1790: Update the major-GC flow around seed_major_roots and
gc_step so multi-mutator incremental collection acquires and maintains
stop-the-world quiescence before scanning roots, seeding, sweeping, and
completing a global cycle. Ensure the guard covers the full gc_step operation,
including cycle completion, while preserving the existing single-mutator
behavior; add a regression test proving objects rooted exclusively by another
mutator survive.
🪄 Autofix (Beta)
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
Run ID: d41dca32-23f4-45cb-bdf0-1c80c81cf87f
📒 Files selected for processing (28)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/gc_sync.rsmajit/majit-gc/src/hook_cell.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-metainterp/src/pyjitpl.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_codecs/mod.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/pycode.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/gc_roots.rspyre/pyre-object/src/interp_sre.rspyre/pyre-object/src/weakref.rs
🛑 Comments failed to post (5)
majit/majit-backend-wasm/src/lib.rs (1)
503-527: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n 'ca_deopt_helper_slot' --type=rust -C4 rg -n 'install_gc_standalone' --type=rust -C2Repository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the file and inspect the relevant section. git ls-files 'majit/majit-backend-wasm/src/lib.rs' wc -l majit/majit-backend-wasm/src/lib.rs sed -n '460,560p' majit/majit-backend-wasm/src/lib.rs # Search for the GC routing helpers and CA-related symbols. rg -n 'with_wasm_active_gc_mut|WASM_ACTIVE_GC|gc_sync::gc_op|install_gc_standalone|ca_deopt_helper_slot|wasm_jit_ca_alloc_frame|wasm_jit_alloc_array|wasm_jit_alloc|wasm_alloc_nursery_typed|bh_new' majit/majit-backend-wasm/src/lib.rs majit -SRepository: youknowone/pyre
Length of output: 36233
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '280,310p' majit/majit-backend-wasm/src/lib.rs sed -n '730,745p' majit/majit-backend-wasm/src/lib.rs sed -n '1418,1436p' majit/majit-backend-wasm/src/lib.rs sed -n '1798,1816p' majit/majit-backend-wasm/src/lib.rs sed -n '998,1020p' majit/majit-backend-wasm/src/codegen.rsRepository: youknowone/pyre
Length of output: 5991
Route
wasm_jit_ca_alloc_framethroughwith_wasm_active_gc_mutinstall_gc_standaloneleavesWASM_ACTIVE_GCempty, but CA lowering is still enabled whenca_deopt_helper_slot() != 0, so this allocator can return 0 on production recursive frames.🤖 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 `@majit/majit-backend-wasm/src/lib.rs` around lines 503 - 527, Update wasm_jit_ca_alloc_frame to access the active collector through with_wasm_active_gc_mut instead of directly borrowing WASM_ACTIVE_GC. Preserve the existing nursery allocation, zero-handle fallback, JitFrame initialization, shadow-stack push, and returned pointer behavior while ensuring standalone GC installation works when CA lowering is enabled.majit/majit-gc/src/gc_sync.rs (3)
195-216: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C4 --type rust '\b(gc_query_reentrant|singleton_ref_reentrant)\s*\(' .Repository: youknowone/pyre
Length of output: 15845
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' majit/majit-gc/src/gc_sync.rs printf '\n---\n' sed -n '660,780p' majit/majit-gc/src/lib.rsRepository: youknowone/pyre
Length of output: 15461
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C6 'pub fn gc_op|fn gc_op|GC_OP_OWNER|singleton_mut\(' majit/majit-gc/src/gc_sync.rsRepository: youknowone/pyre
Length of output: 5101
Avoid creating a shared reference from the singleton while
gc_opstill holds&mut.gc_query_reentrantre-derives&dyn GcAllocatorfromGC_STOREon the reentrant path, which aliases the live mutable borrow to the same singleton. Reborrow from the active&mutwith a callback-scoped lifetime, or switch this path to raw pointers.🤖 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 `@majit/majit-gc/src/gc_sync.rs` around lines 195 - 216, Update gc_query_reentrant so its in_gc_op() branch does not call singleton_ref_reentrant or create a shared reference directly from GC_STORE while gc_op holds the mutable borrow. Reuse the active &mut borrow with a callback-scoped reborrow, or pass the singleton through raw pointers, while preserving the read-only callback behavior and the gc_query path for top-level calls.
235-266: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Quiesce existing GC activity before publishing a new mutator.
The
0→1transition skipsIN_FAST_PATH, allowing registration during an unregistered thread’s collection. The1→2transition disables inline allocation without stopping the existing mutator, which may still be executing the non-atomic nursery bump path whilepublished_nursery_topis rewritten. Serialize registration and quiesce existing mutators before exposing the new thread or changing publication state.🤖 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 `@majit/majit-gc/src/gc_sync.rs` around lines 235 - 266, Update register_thread to serialize registration with ongoing GC fast-path activity and quiesce existing mutators before publishing the new thread. Ensure both the 0→1 and 1→2 transitions acquire the operation gate represented by IN_FAST_PATH, wait for active GC work to finish, and only then update GC_SYNC running/registered state or call gc_op to disable inline allocation. Preserve the existing duplicate-registration assertion and waiting behavior for stw_requested.
342-419: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release
IN_FAST_PATHthrough RAII.If
funwinds, Lines 346 or 417 never clear the gate. Other threads then spin indefinitely in registration or slow-path acquisition. Move ownership into a guard whoseDropperforms the release.🤖 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 `@majit/majit-gc/src/gc_sync.rs` around lines 342 - 419, Update gc_op_slow and the fast-path operation around OpGuard::enter so IN_FAST_PATH ownership is managed by an RAII guard whose Drop clears the gate with Release ordering. Ensure the guard is created immediately after each successful gate acquisition and remains active through f, so unwinding releases the gate while preserving existing reentrant self_holds_fast_path behavior.majit/majit-gc/src/shadow_stack.rs (1)
258-293: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C4 --type rust \ '\b(register_mutator|unregister_mutator|register_mutator_extra_area)\s*\(' .Repository: youknowone/pyre
Length of output: 5313
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the mutator registration implementation and its call sites. sed -n '240,360p' majit/majit-gc/src/shadow_stack.rs printf '\n---- eval.rs relevant section ----\n' sed -n '840,880p' pyre/pyre-jit/src/eval.rs printf '\n---- gc_sync.rs relevant section ----\n' sed -n '560,610p' majit/majit-gc/src/gc_sync.rs printf '\n---- extra-area registration call sites ----\n' rg -n -C2 --type rust 'register_mutator_extra_area\s*\(' .Repository: youknowone/pyre
Length of output: 9304
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the thread-local registration block around GC_MUTATOR_REGISTRATION. sed -n '842,872p' pyre/pyre-jit/src/eval.rs printf '\n---- thread-local declaration context ----\n' sed -n '1,80p' majit/majit-gc/src/shadow_stack.rs printf '\n---- all register_mutator_extra_area call sites with context ----\n' rg -n -C3 --type rust 'register_mutator_extra_area\s*\(' majit pyreRepository: youknowone/pyre
Length of output: 6359
Make the extra-area registration carry an explicit lifetime boundary
register_mutatoris already tied toGC_MUTATOR_REGISTRATION, so the mutator entry is cleaned up on thread exit. The remainingdatapointer here still relies on an unchecked lifetime contract; make thisunsafeor tie the registered extra area to a guard that owns the lifetime.🤖 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 `@majit/majit-gc/src/shadow_stack.rs` around lines 258 - 293, Update register_mutator_extra_area to make the extra-area data lifetime boundary explicit, preferably by marking the API unsafe and documenting that callers must keep data valid until unregister_mutator or thread cleanup; preserve the existing registration behavior and ownership-thread restriction.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5439e28929
ℹ️ 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".
| next_instr: usize, | ||
| _ec: *const PyExecutionContext, | ||
| ) -> PyResult { | ||
| majit_gc::gc_sync::safepoint_poll(); |
There was a problem hiding this comment.
This is currently the only safepoint_poll() call, so it only runs when execution is back in PyPyJitDriver::dispatch. A registered mutator already executing a compiled loop that does not call a GC trampoline (for example an unboxed arithmetic loop) never reaches this site; if another thread triggers collect_nursery/collect_full, quiesce_mutators() waits for RUNNING to drop, but the compiled mutator has no poll where it can park, so the collection can hang indefinitely. Please also emit/check the STW safepoint on compiled backedges or other compiled-code safepoints.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-gc/src/collector.rs (1)
1730-1790: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftQuiesce incremental-cycle root seeding and completion.
seed_major_rootsscans all mutators only when STW is already owned, butgc_step()can call bothstart_incremental_cycle()andfinish_incremental_cycle()without acquiring it. In multi-threaded execution this omits foreign TLS roots and can sweep objects still reachable from another mutator. Acquire STW around cycle start and sweep completion.🤖 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 `@majit/majit-gc/src/collector.rs` around lines 1730 - 1790, Update the incremental-cycle flow so both start_incremental_cycle() and finish_incremental_cycle() acquire the stop-the-world synchronization before calling seed_major_roots() or completing the sweep. Ensure the guard covers root seeding and sweep completion, including foreign mutator roots, while preserving existing behavior for callers that already hold STW.
🤖 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 `@majit/majit-backend-wasm/src/lib.rs`:
- Around line 211-230: Update with_wasm_active_gc to use a non-panicking TLS
borrow via try_borrow instead of borrow when checking WASM_ACTIVE_GC. Preserve
the existing boxed-allocator path when the borrow succeeds, and use the raw
mirror fallback pattern from wasm_gc_owns_object when the test-box borrow is
already held, before routing to gc_query_reentrant or returning None.
In `@majit/majit-gc/src/collector.rs`:
- Around line 347-354: The published nursery-top value must be updated
atomically because mutators can read it while GC updates it. Change
published_nursery_top and the related refresh_published_nursery_top()
publication path to use an atomic type with appropriate load/store ordering, and
ensure nursery_top_addr() exposes the stable atomic-backed slot without
unsynchronized plain usize access; apply the same synchronization to
set_inline_alloc_enabled() if generated code observes that flag concurrently.
In `@majit/majit-gc/src/gc_sync.rs`:
- Around line 155-175: Update the GC operation guard around gc_op_slow and the
IN_FAST_PATH handling to use RAII guards whose Drop implementations restore
prior state during unwinding. Replace the debug-only exclusivity check in
OpGuard::enter with an unconditional failure for nested ownership, ensuring
indirect reentrant gc_op calls cannot borrow the singleton again in release
builds.
In `@majit/majit-gc/src/lib.rs`:
- Around line 1424-1452: Update set_active_root_hooks so enabling callbacks
publishes ACTIVE_REMOVE_ROOT before ACTIVE_ADD_ROOT, preventing gc_add_root from
observing an incomplete hook pair; when clearing hooks, preserve the existing
add-first ordering by clearing ACTIVE_ADD_ROOT before ACTIVE_REMOVE_ROOT.
In `@majit/majit-gc/src/shadow_stack.rs`:
- Around line 224-337: Change register_mutator to return an RAII guard whose
Drop implementation removes the current thread via unregister_mutator,
preventing safe callers from leaving TLS entries registered. Mark
register_mutator_extra_area unsafe to require callers to uphold data lifetime
and validity, and make walk_all_extra_areas plus all analogous foreign
walk_all_* APIs unsafe or gate them with mutators_quiesced() before
dereferencing registered TLS. Update the affected registration, walking, and
cleanup call sites to use the guard and satisfy the new safety contract.
In `@pyre/pyre-interpreter/src/importing.rs`:
- Around line 1161-1196: Update walk_import_roots_area to access area.modules
through its RefCell raw-pointer as_ptr() path instead of modules.borrow(), since
the owning thread is quiesced but may retain an outstanding mutable borrow.
Iterate the referenced module map directly while preserving the existing null
and module checks and visitor behavior.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 2684-2687: Update jit_driver_pair_from_root_area to use a
RefCell-style non-panicking borrow guard, matching
walk_jitcode_constants_refs_area: return the JitDriverPair only when the root
area is not already mutably borrowed, and return None during re-entrant walks.
Adjust the JIT_DRIVER storage/access path as needed so no second &mut
JitDriverPair can be created.
---
Outside diff comments:
In `@majit/majit-gc/src/collector.rs`:
- Around line 1730-1790: Update the incremental-cycle flow so both
start_incremental_cycle() and finish_incremental_cycle() acquire the
stop-the-world synchronization before calling seed_major_roots() or completing
the sweep. Ensure the guard covers root seeding and sweep completion, including
foreign mutator roots, while preserving existing behavior for callers that
already hold STW.
🪄 Autofix (Beta)
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
Run ID: 7fdd9388-e8d6-48d4-ab7e-f8fed73fa7f3
📒 Files selected for processing (28)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/gc_sync.rsmajit/majit-gc/src/hook_cell.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-metainterp/src/pyjitpl.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_codecs/mod.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/pycode.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/gc_hook.rspyre/pyre-object/src/gc_roots.rspyre/pyre-object/src/interp_sre.rspyre/pyre-object/src/weakref.rs
| /// Root structures owned by one registered mutator thread. | ||
| /// | ||
| /// The pointers remain valid until `unregister_mutator` runs on the owning | ||
| /// thread. Foreign access is only permitted while gc_sync has quiesced every | ||
| /// registered mutator, so none of the underlying TLS structures can move or | ||
| /// be mutably accessed by its owner during a walk. | ||
| struct MutatorEntry { | ||
| thread_id: std::thread::ThreadId, | ||
| shadow_stack: *const RefCell<ShadowStack>, | ||
| jf_root_stack: *const RefCell<JitFrameShadowStack>, | ||
| bh_regs_stack: *const RefCell<Vec<BhRegsEntry>>, | ||
| resume_ref_roots_stack: *const RefCell<Vec<(*mut i64, usize)>>, | ||
| extra_areas: Vec<MutatorExtraArea>, | ||
| } | ||
|
|
||
| /// Walker for one opaque root area owned by a registered mutator. | ||
| /// | ||
| /// The callback runs on the collecting thread. It must derive every | ||
| /// thread-specific address from `data` and must not consult caller TLS. | ||
| pub type MutatorExtraWalkFn = unsafe fn(*const (), &mut dyn FnMut(&mut GcRef)); | ||
|
|
||
| #[derive(Clone, Copy)] | ||
| struct MutatorExtraArea { | ||
| walk: MutatorExtraWalkFn, | ||
| data: *const (), | ||
| } | ||
|
|
||
| // The raw pointers refer to TLS owned by `thread_id`. The registry only moves | ||
| // pointer values between threads; dereferencing them requires the STW | ||
| // quiescence established by gc_sync. | ||
| unsafe impl Send for MutatorEntry {} | ||
|
|
||
| static MUTATOR_REGISTRY: Mutex<Vec<MutatorEntry>> = Mutex::new(Vec::new()); | ||
|
|
||
| /// Register the current thread's four TLS root structures for STW root walks. | ||
| pub fn register_mutator() { | ||
| let thread_id = std::thread::current().id(); | ||
| let shadow_stack = SHADOW_STACK.with(|stack| stack as *const _); | ||
| let jf_root_stack = JF_ROOT_STACK.with(|stack| stack as *const _); | ||
| let bh_regs_stack = BH_REGS_STACK.with(|stack| stack as *const _); | ||
| let resume_ref_roots_stack = RESUME_REF_ROOTS_STACK.with(|stack| stack as *const _); | ||
|
|
||
| let mut registry = MUTATOR_REGISTRY.lock().unwrap(); | ||
| assert!( | ||
| !registry.iter().any(|entry| entry.thread_id == thread_id), | ||
| "mutator thread registered twice" | ||
| ); | ||
| registry.push(MutatorEntry { | ||
| thread_id, | ||
| shadow_stack, | ||
| jf_root_stack, | ||
| bh_regs_stack, | ||
| resume_ref_roots_stack, | ||
| extra_areas: Vec::new(), | ||
| }); | ||
| } | ||
|
|
||
| /// Append an opaque root area to the current registered mutator. | ||
| /// | ||
| /// The owning thread must keep `data` valid until [`unregister_mutator`]. | ||
| /// Foreign-thread invocation is restricted to the STW walk. | ||
| pub fn register_mutator_extra_area(walk: MutatorExtraWalkFn, data: *const ()) { | ||
| let thread_id = std::thread::current().id(); | ||
| let mut registry = MUTATOR_REGISTRY.lock().unwrap(); | ||
| let entry = registry | ||
| .iter_mut() | ||
| .find(|entry| entry.thread_id == thread_id) | ||
| .expect("register_mutator_extra_area called before register_mutator"); | ||
| entry.extra_areas.push(MutatorExtraArea { walk, data }); | ||
| } | ||
|
|
||
| /// Walk every registered mutator's opaque extra root areas during STW. | ||
| pub fn walk_all_extra_areas(mut visitor: impl FnMut(&mut GcRef)) { | ||
| let registry = MUTATOR_REGISTRY.lock().unwrap(); | ||
| for mutator in registry.iter() { | ||
| for area in mutator.extra_areas.iter() { | ||
| // SAFETY: gc_sync has quiesced every registered owner, and each | ||
| // area remains valid until its MutatorEntry is removed. | ||
| unsafe { (area.walk)(area.data, &mut visitor) }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Walk the current mutator's opaque extra root areas. | ||
| /// | ||
| /// This is the single-thread collection path; callers without a registered | ||
| /// mutator have no per-thread areas and are a no-op. | ||
| pub fn walk_my_extra_areas(mut visitor: impl FnMut(&mut GcRef)) { | ||
| let thread_id = std::thread::current().id(); | ||
| let registry = MUTATOR_REGISTRY.lock().unwrap(); | ||
| let Some(mutator) = registry.iter().find(|entry| entry.thread_id == thread_id) else { | ||
| return; | ||
| }; | ||
| for area in mutator.extra_areas.iter() { | ||
| // SAFETY: this is the owning thread's synchronous collection path. | ||
| unsafe { (area.walk)(area.data, &mut visitor) }; | ||
| } | ||
| } | ||
|
|
||
| /// Remove the current thread from the all-thread root registry. | ||
| /// | ||
| /// Call this before gc_sync::unregister_thread so RUNNING cannot reach zero | ||
| /// while an entry whose TLS is being destroyed remains visible to the | ||
| /// collector. | ||
| pub fn unregister_mutator() { | ||
| let thread_id = std::thread::current().id(); | ||
| let mut registry = MUTATOR_REGISTRY.lock().unwrap(); | ||
| let index = registry | ||
| .iter() | ||
| .position(|entry| entry.thread_id == thread_id) | ||
| .expect("unregistering an unregistered mutator thread"); | ||
| registry.swap_remove(index); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Enforce the registry’s lifetime and STW invariants in the API.
Safe callers can leave dangling TLS entries by missing unregister_mutator, register arbitrary invalid data, or invoke walk_all_* without owning STW, making undefined behavior reachable through safe APIs. Return an RAII registration guard, make extra-area registration unsafe, and make foreign walkers unsafe or assert mutators_quiesced() before dereferencing.
Also applies to: 414-433, 609-631, 768-786, 836-852
🤖 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 `@majit/majit-gc/src/shadow_stack.rs` around lines 224 - 337, Change
register_mutator to return an RAII guard whose Drop implementation removes the
current thread via unregister_mutator, preventing safe callers from leaving TLS
entries registered. Mark register_mutator_extra_area unsafe to require callers
to uphold data lifetime and validity, and make walk_all_extra_areas plus all
analogous foreign walk_all_* APIs unsafe or gate them with mutators_quiesced()
before dereferencing registered TLS. Update the affected registration, walking,
and cleanup call sites to use the guard and satisfy the new safety contract.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/78fbc57ff939a8c0eabdf307cfca83d19c9854b4/pyre-object/src/gc_hook.rs#L290
When a thread that has not run init_gc_subsystem hits an existing root guard, this process-global GC_ADD_ROOT_HOOK now succeeds and stores that thread's raw stack slot in MiniMarkGC::RootSet; gc_sync::stw_required() only counts registered mutators, while do_collect_nursery dereferences every self.roots entry unconditionally. In a run with one registered mutator collecting and an unregistered helper holding a rooted slot, the collector can read or update the helper's stack while it is still running, so the root trampoline needs to fail/force registration for unregistered threads or make these stack roots participate in STW.
ℹ️ 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".
set_active_root_hooks set ACTIVE_ADD_ROOT first, so a concurrent gc_add_root could register a root while ACTIVE_REMOVE_ROOT was still unset, leaving that root without a working removal path. Install remove-first and clear add-first, matching the pyre-object gc_hook ordering. Assisted-by: Claude Assisted-by: Codex
refresh_published_nursery_top and set_inline_alloc_enabled updated the published nursery-top word with plain stores while generated inline allocation code can read the slot from an unparked thread. Store the published limit through Box<AtomicUsize> with Release ordering and load it with Acquire on the Rust-side readers; the baked address exposed by nursery_top_addr is unchanged. Assisted-by: Claude Assisted-by: Codex
…ty in release IN_FAST_PATH was cleared with plain stores after the gc_op closure returned, so a panic inside the closure left the gate held and every later gc_op on the thread stuck behind it. Clear it from a Drop guard in both the fast path and the slow path's non-self-holding case. OpGuard::enter only debug_asserted that no other owner held the singleton; a nested gc_op in release would reach a second &mut silently. Upgrade the check to a release assert. Assisted-by: Claude Assisted-by: Codex
…area walk walk_import_roots_area used modules.borrow(), which panics when the quiesced owning thread parked with an outstanding borrow_mut. Read the map through as_ptr() like the other quiesced root walkers; the owner is stopped, so the map is stable. Assisted-by: Claude Assisted-by: Codex
…eentrant The rebase brought in GATE_HELD_DEPTH/GateHeldGuard, which let a gate-holding thread re-enter gc_op directly on the singleton. This branch already forbids nested &mut gc_op (OpGuard exclusivity assert) and routes collection-time reads through gc_query_reentrant, and the valve only covered the slow path, so a fast-path nested query tripped the gc_op entry assert. No runtime path nests: the finalizer trigger fired during collection only sets the UserDelAction flag, and next_dead drains at the mutator-level action checkpoint. Rewrite the nested-query test to use gc_query_reentrant inside gc_op. Assisted-by: Claude Assisted-by: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b634f04025
ℹ️ 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".
| @@ -168,12 +206,32 @@ fn install_gc_box(gc: Box<dyn majit_gc::GcAllocator>) { | |||
| ); | |||
There was a problem hiding this comment.
In the production install_gc_standalone path there is intentionally no DYNASM_ACTIVE_GC box, but these process-global finalizer hooks still point at dynasm_register_finalizer / dynasm_finalizer_next_dead, which only inspect that TLS box and silently do nothing/return None when it is absent. On dynasm builds, objects registered with a finalizer after GC init will therefore never enter the singleton GC's finalizer queues, so __del__/FinalizerQueue semantics are skipped; please make these callbacks use the same gc_sync fallback as the other demoted trampolines.
Useful? React with 👍 / 👎.
| @@ -250,11 +289,33 @@ fn install_gc_box(gc: Box<dyn majit_gc::GcAllocator>) { | |||
| ); | |||
There was a problem hiding this comment.
With the new production install_gc_standalone path, WASM_ACTIVE_GC remains None, but the globally registered finalizer callbacks still call wasm_register_finalizer / wasm_finalizer_next_dead, which only use that TLS box and otherwise drop the operation. In wasm production, any GC-managed object that relies on the RPython finalizer queue will not be registered or later returned by next_dead; these hooks need the same with_wasm_active_gc_mut/gc_sync fallback as allocation, collection, and write-barrier hooks.
Useful? React with 👍 / 👎.
Forward the nested-inline abort outer-frame stash (FBW_ABORT_OUTER_STACK_OVERRIDES) and the active walk session's InlineParentFrame.call_stack_overrides slots as GC roots through the captured FbwStoreJournalRootArea, matching the quiesced-area model. Both stashes hold nursery-resident PyObjectRefs across residual-call allocations and are populated by default-on paths (PYRE_FBW_NESTED_RESID_ABORT, PYRE_FBW_INLINE_MULTIFRAME). Assisted-by: Claude
Forward the nested-inline abort outer-frame stash (FBW_ABORT_OUTER_STACK_OVERRIDES) and the active walk session's InlineParentFrame.call_stack_overrides slots as GC roots through the captured FbwStoreJournalRootArea, matching the quiesced-area model. Both stashes hold nursery-resident PyObjectRefs across residual-call allocations and are populated by default-on paths (PYRE_FBW_NESTED_RESID_ABORT, PYRE_FBW_INLINE_MULTIFRAME). Assisted-by: Claude
…eclines Abort override rooting: fbw_abort_outer_resume_take now returns only the resume pc. The outer-caller stack overrides stay in FBW_ABORT_OUTER_STACK_OVERRIDES (rooted by the #447 area walker) and are read in place via fbw_abort_outer_stack_overrides_with during the walk-end flush, then cleared with fbw_abort_outer_stack_overrides_clear. The flush boxes Int/Float locals, an allocation that can move the nursery-resident override refs; reading from the rooted cell lets a minor collection forward the very refs the flush writes. Residual vable token: move the tracing_before_residual_call arm from before the decline gates to after fbw_abort_nested_unjournaled_residual, so a residual declined by the inline-subwalk gate no longer strands TOKEN_TRACING_RESCALL on the active virtualizable. tracing_before now pairs with tracing_after only for residuals that proceed. Assisted-by: Claude
…eclines Abort override rooting: fbw_abort_outer_resume_take now returns only the resume pc. The outer-caller stack overrides stay in FBW_ABORT_OUTER_STACK_OVERRIDES (rooted by the #447 area walker) and are read in place via fbw_abort_outer_stack_overrides_with during the walk-end flush, then cleared with fbw_abort_outer_stack_overrides_clear. The flush boxes Int/Float locals, an allocation that can move the nursery-resident override refs; reading from the rooted cell lets a minor collection forward the very refs the flush writes. Residual vable token: move the tracing_before_residual_call arm from before the decline gates to after fbw_abort_nested_unjournaled_residual, so a residual declined by the inline-subwalk gate no longer strands TOKEN_TRACING_RESCALL on the active virtualizable. tracing_before now pairs with tracing_after only for residuals that proceed. Assisted-by: Claude
…ch-resume local binding fix (gh#498) (#513) * jit: decline unjournaled mutating residual executed inside an inline sub-walk Assisted-by: Claude * jit: decline any non-provably-pure residual inside an inline sub-walk Generalize the pre-call residual decline from write-tagged residuals to every residual that is not provably side-effect-free. The existing helper limits the gate to inline sub-walks, while writes_live_heap remains available for the FOR_ITER body-effect candidate path. Assisted-by: Claude * jit: latch executed nonpure FBW residuals Assisted-by: Claude * jit: stash const-int and float FBW returns Assisted-by: Claude * jit: flush nested executed-effect aborts Forward-commit nested inline residual-decline aborts after the walk has already executed a non-pure residual. Stash the top-level caller CALL pc and concrete call stack at inline-capture time, then use that outer coordinate for the abort flush instead of mapping the callee error pc through the outer jitcode tables. Assisted-by: Claude * jit: clear residual-call exception at entry Move the walk exception clear to residual-call entry before any dispatcher fold, symbolic decline, or concrete helper execution. This matches execute_varargs semantics and prevents a handled exception from surviving into a later linear catch_exception/L. Remove the catch_exception/L clear-and-continue workaround so an active exception there is again a strict invariant error. The success-arm clear was also removed because residual-call entry now performs the same WalkContext reset before every residual-call path; the existing success arm did not reset BH_LAST_EXC_VALUE, so there was no BH state to mirror. Assisted-by: Claude * check: add gh#495 double-apply regression tests to synth suite Add synth parity oracles for gh#495 covering inline subwalk mutation with abort/no-exception paths, property mutation, reflected-add consumption, user-iterator mutation, nested callee mutation, caught and uncaught mutating raises, and return-value side effects for float and constant-int callees. Assisted-by: Claude * Add closure branch resume regressions Add gh#498 synthetic parity cases for a closure freevar read, a nonlocal mutation, and a list-cell mutation feeding the same branch-resume local binding shape. Assisted-by: Claude * Add foriter exemption regression guards Assisted-by: Claude * Seed callee vstack mirror behind gate Assisted-by: Claude * jit: gate branch stack live-register snapshot Assisted-by: Claude * jit: enable branch stack live-register snapshot by default The preferred source is get_list_of_active_boxes's registers_r[index] (pyjitpl.py:222); the pcdep ownership proof bounds it to where the pyre register read means the same thing. PYRE_FBW_STACK_LIVEREG=0 restores the shadow-first order. Assisted-by: Claude * jit: flush BranchGuardKeptStackUnsupported FBW aborts A top-level FBW portal walk that concretely ran a FOR_ITER body's residual effects before aborting with BranchGuardKeptStackUnsupported had no forward-flush leg in run_perfn_walk: committed stayed false and legacy snapshot-replay re-ran the walked iteration, double-applying the executed __next__ effects. Extend the PYRE_FBW_BRANCH_FLUSH or-pattern to also match BranchGuardKeptStackUnsupported. That variant aborts after the consumed item's body ran, so the walk's symbolic shadow end state already counted the iteration; flush it with flush_walk_end_state_to_frame without re-delivering the in-flight item. BranchGuardUnrestorableKeptStackPermanent keeps its Shape-A in-flight item delivery. Assisted-by: Claude * jit: source portal-red active boxes from live bank Prefer a real color-indexed Ref-bank value when a force-alived portal red is present in liveness with no semantic frame slot. Fall back to the named red field, then CONST_NULL, so top-level finish snapshots do not panic on dead-or-unseeded red fields. Add a synth regression covering a three-term heap-loaded or chain with a freshly allocated loop argument. Re-applies the collect_outer_active_boxes portion of the pre-rebase change without its exception-stash match arm reflow (that stash was retired in favour of the upstream FinishConcrete::Raise disposition). Assisted-by: Claude * jit: root FBW stack override stashes through the #447 root-walker area Forward the nested-inline abort outer-frame stash (FBW_ABORT_OUTER_STACK_OVERRIDES) and the active walk session's InlineParentFrame.call_stack_overrides slots as GC roots through the captured FbwStoreJournalRootArea, matching the quiesced-area model. Both stashes hold nursery-resident PyObjectRefs across residual-call allocations and are populated by default-on paths (PYRE_FBW_NESTED_RESID_ABORT, PYRE_FBW_INLINE_MULTIFRAME). Assisted-by: Claude * jit: root FBW abort overrides across flush; arm residual token past declines Abort override rooting: fbw_abort_outer_resume_take now returns only the resume pc. The outer-caller stack overrides stay in FBW_ABORT_OUTER_STACK_OVERRIDES (rooted by the #447 area walker) and are read in place via fbw_abort_outer_stack_overrides_with during the walk-end flush, then cleared with fbw_abort_outer_stack_overrides_clear. The flush boxes Int/Float locals, an allocation that can move the nursery-resident override refs; reading from the rooted cell lets a minor collection forward the very refs the flush writes. Residual vable token: move the tracing_before_residual_call arm from before the decline gates to after fbw_abort_nested_unjournaled_residual, so a residual declined by the inline-subwalk gate no longer strands TOKEN_TRACING_RESCALL on the active virtualizable. tracing_before now pairs with tracing_after only for residuals that proceed. Assisted-by: Claude
Addresses #396 (R1+R2+R3, plus R4 slices 1–4: reentrancy-safe
gc_sync+ backend box demotion on all three backends — dynasm, cranelift, wasm).After the GC became a process-global singleton (
gc_sync, #388), the ~47 per-threadthread_local! Cell<Option<fn>>GC hook cells installed identical values on every thread. A collector running on an arbitrary thread would see an unset hook on any thread that never ran the install path — a free-threading correctness gap (P0), not just cleanup.Changes
FnPtrCell(AtomicPtr-backed, null =None, Acquire/Release, pointer-sizeconst-assert) +global_hook!macro (kept in majit-gc so pyre-object need not depend on it). Convert theACTIVE_*GC-guard / alloc / collect / root / write-barrier / heap-stats fn-ptr cells to process-global statics;ACTIVE_SUPPORTS_GUARD_GC_TYPE→AtomicBool. Addoverride_gc_guard_hooks_for_test(RAII guard under a process-global lock) for the metainterp mock-guard-hook test. De-thread-local the hook doc comments.gc_hookfn-ptr cells toglobal_hook!. Delete the write-only-deadGC_HEAP_STATS_HOOKpath (the interpreter safepoint dropped itsnursery_used == 0gate at thecollect_oldgenswitch). AddHOOK_TEST_LOCKserializing the hook-installer tests now that the cells are shared.init_gc_subsystem— per-threadgc_syncmutator registration + backend GC handle install stay per-thread; the process-global pyre-object hooks install once under aOnce, after the backendset_active_*install they route through (backend-first ordering). Remove the dead heap-stats trampoline.register_pyframe_root_walkerdoc — now a process-global cell.R4 — reentrancy-safe
gc_sync+ backend box demotionBackend
*_ACTIVE_GCTLS is not a pure ZST vestige: it is the test GC-ownership mechanism (set_gc_allocator, ~30 tests install a realBox<MiniMarkGC>), and the_RAWmirror answered reentrant collection-time ownership queries becausegc_syncwas not reentrant. R4 makes production stop relying on the box.gc_syncreentrant read-only query. A thread-localGC_OP_DEPTH+OpGuardwraps every&mutsingleton access (gc_opfast/slow,request_stw);gc_query_reentranttakes a fresh&*GC_STOREshared read when already inside agc_op(the collection-time root-walk case), avoiding the double-&mutalias / non-recursive-mutex deadlock.debug_assert!(!in_gc_op())catches an accidental reentrant&mut.GcHandle's&selfmethods now route throughgc_query_reentrant.install_gc_standaloneregisters theset_active_*hooks WITHOUT a box, so production reaches the GC throughgc_sync; tests still install a real box. Because aNonebox no longer means "no GC", every site that treated box-absent as GC-absent (alloc/write-barrier slowpaths called from JIT code, codegen-time guard-table / write-barrier-descr reads, theis_none()gates) now consultsgc_syncbefore its non-GC fallback. Missing that on dynasm first surfaced as omitted write barriers → remembered-set corruption → SIGSEGV/SIGBUS infib_recursive/nbody/calls_closures; all three backends are now clean under repeated GC-stress. (wasm is single-threaded, so its box was never a free-threading hazard — it is demoted for consistency.)The P1 free-threading work (STW safepoint + TLAB atop the now-reentrant
gc_sync) is tracked separately in #452.Verification
fib_recursive/nbody/calls_closurespass 5×5 with no crash under productiongc_syncrouting + a debug GC-stress run (debug_assert live) with all root-walkers registered.cargo testjob exercises the pre-existing GC TLS hooks → global static: 47 per-thread fn-ptr cells are redundant #396 parallel-GC-test race (many libtest threads share the onegc_syncGC → nondeterministic SIGABRT); that is the condition this epic exists to remove (P1, Free-threading P1: STW-safepoint + TLAB + reentrancy-safe gc_sync (#396 R4 blocker) #452), not a regression from these commits — the same suite passes--test-threads=1and passes locally.🤖 Generated with Claude Code
Summary by CodeRabbit