Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions majit/majit-gc/src/shadow_stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1260,16 +1260,19 @@ pub fn extra_root_walk_kind() -> ExtraRootWalkKind {
EXTRA_ROOT_WALK_KIND.with(|k| k.get())
}

// Walkers registered today: eval.rs registers twelve (rd_consts, partial
// trace, active trace, compile snapshot, jitcode constants, fbw store
// journal, fbw finish concrete, pyre interpreter side tables, signal
// handlers, the _io autoflusher handle list, jit callee frames, and pyre
// objects), plus gcreftracer.rs registers the per-loop gc_table walker —
// thirteen total.
// Leave headroom above that so a future root source does not overflow the
// array and poison the registry lock with a "capacity exceeded" panic on
// first use.
const MAX_EXTRA_ROOT_WALKERS: usize = 16;
// One registration per *kind* of root storage, as framework.py registers
// GcRootMap sources: the interpreter's process-global off-GC slots
// (`walk_interpreter_global_roots`), the exceptions parked outside GC
// discipline (`walk_parked_exception_roots`), the immortal process-global
// stores (`walk_immortal_store_roots`), and the per-loop gc_table walker
// registered from gcreftracer.rs — four in a native build, three on wasm and
// under `sandbox`, where the faulthandler source is compiled out.
//
// Anything that enumerates a population belongs inside one of those, not in a
// slot of its own; the cap is what stops the array from drifting back into a
// per-defect callback registry, so raising it is the wrong response to running
// out of room.
const MAX_EXTRA_ROOT_WALKERS: usize = 8;

/// Registered root walkers. Each slot is either `None` or a function
/// pointer. We cap the count to keep the set stack-allocatable and
Expand Down
19 changes: 12 additions & 7 deletions majit/majit-metainterp/src/recorder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,21 @@ pub struct Snapshot {
pub struct SnapshotFrame {
/// Index of the jitcode (or 0 for the root portal).
pub jitcode_index: u32,
/// Program counter within the jitcode. In RPython, the MIFrame's
/// `pc` field IS the JitCode byte offset (`pyjitpl.py:185
/// setposition`). Pyre's tracer populates this slot with the Python
/// bytecode PC because pyre traces Python bytecode rather than JitCode
/// — see `[[project-issue73-phase5-design]]` for the broader deviation
/// context; the runtime translates `py_pc` through `pc_map` at resume
/// time until pyre's walker-as-tracer epic lands.
/// Program counter within the jitcode: the JitCode byte offset, as the
/// MIFrame's `pc` field is upstream (`pyjitpl.py:185 setposition`). Both
/// writers stamp a JitCode offset — `build_state_field_snapshot` reads
/// `MIFrame::pc` directly, and `capture_snapshot_for_last_guard_multi_frame`
/// takes it from the walker's own `build_framestack_snapshot`. There is no
/// longer a second, Python-keyed coordinate here and no translation table
/// between the two.
pub pc: u32,
/// Forward-carried Python instruction PC for this JitCode position.
/// `u32::MAX` is the no-snapshot sentinel paired with `pc == -1`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 4 \
  'SnapshotFrame|build_state_field_snapshot|build_framestack_snapshot|capture_snapshot_for_last_guard_multi_frame|py_pc|u32::MAX|pc\s*==\s*-1' \
  --glob '*.rs' .

Repository: youknowone/pyre

Length of output: 50372


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="majit/majit-metainterp/src/recorder.rs"
if [ -f "$FILE" ]; then
  echo "== recorder.rs outline =="
  ast-grep outline "$FILE" --view compact || true
  echo "== recorder.rs relevant lines =="
  sed -n '1,180p' "$FILE" | cat -n
else
  echo "missing $FILE"
  fd -a recorder.rs .
fi

echo "== exact matches focused =="
rg -n -C 3 'SnapshotFrame|build_state_field_snapshot|build_framestack_snapshot|capture_snapshot_for_last_guard_multi_frame|u32::MAX|pc == -1|pc == u32::MAX' majit/majit-metainterp/src/recorder.rs majit/majit-metainterp/src majit/majit-metainterp || true

Repository: youknowone/pyre

Length of output: 50372


Clarify the no-snapshot py_pc sentinel.

SnapshotFrame::pc is u32, so pc == -1 is not a Rust-side value. The -1 reference matches the resumed/framestack form; note the upstream frame PC sentinel if intentional, or document the stored condition as pc == u32::MAX. Ensure snapshot writers/readers keep the same code path.

🤖 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-metainterp/src/recorder.rs` at line 84, Clarify the no-snapshot
sentinel documentation near SnapshotFrame::pc: distinguish the upstream or
resumed framestack `pc == -1` representation from the stored Rust-side condition
`pc == u32::MAX`. Preserve the existing snapshot writer and reader code paths
and document whichever sentinel is intentionally used consistently.

///
/// Upstream derives the Python-level position where it needs one; this
/// carries it, because the resume decoder that reads it back for
/// `f_lasti` and traceback reconstruction holds no jitcode metadata and
/// so has no jitcode→Python inverse available at that point.
pub py_pc: u32,
/// Tagged references to the live boxes in this frame.
pub boxes: Vec<SnapshotTagged>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
fbw_blackhole_adopted_multi_frame=0
fbw_blackhole_adopted_single_frame=3
fbw_blackhole_adopted_single_frame=0
fbw_rolled_back_with_effects=0
fbw_store_journal_rollback_failed=0
field_pos_attached_misplaced=0
Expand Down
67 changes: 67 additions & 0 deletions pyre/gate-triage.md
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,73 @@ Kept as-is; listed for completeness.
`PYRE_SHARED_BUILD`, `PYRE_SYNTH_PYPY`, `_PYRE`, `_PYTHON`.
- **Test harness (1)**: `PYRE_MIR_STRESS_LLBC`.

## §6 — The 66 gates the audits never listed (2026-08-07)

The hand audits above enumerated what they were looking at. Measured against the
tree instead, **66 of the 105 live gates had no entry anywhere in this file** —
the table was 63% empty, because nothing failed when a new gate skipped it.
`pyre/pyrex/tests/gate_triage_complete.rs` is now that failure: a `PYRE_*` read
with no entry here fails `cargo test`. The counts to quote, distinguished:

| count | value |
|---|---|
| distinct names read from the environment | **105** |
| (file, name) read pairs | 127 |
| **live gates that were absent from this file** | **66** |
| names here with no read site left (retire) | 51 |

```
git ls-files '*.rs' | xargs rg --no-filename -o 'env::var[_a-z]*\("(PYRE_[A-Z0-9_]+)"' -r '$1' | sort -u
```
Comment on lines +853 to +855

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Specify the shell language for this fenced block.

Add sh to the opening fence. This fixes markdownlint rule MD040.

Proposed fix
-```
+```sh
 git ls-files '*.rs' | xargs rg --no-filename -o 'env::var[_a-z]*\("(PYRE_[A-Z0-9_]+)"' -r '$1' | sort -u
📝 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.

Suggested change
```
git ls-files '*.rs' | xargs rg --no-filename -o 'env::var[_a-z]*\("(PYRE_[A-Z0-9_]+)"' -r '$1' | sort -u
```
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)

[warning] 853-853: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/gate-triage.md` around lines 853 - 855, Specify the shell language on
the fenced block containing the git/rg command by changing its opening fence to
use sh, while leaving the command unchanged.

Source: Linters/SAST tools


Polarity below follows this file's rule, with one correction it needed: an
`is_none()` whose value *is* the enable flag means default **ON**, but an
`if …is_none() { return; }` early-return guard means the thing is default
**OFF**. Three diagnostics (`PYRE_DESCR_SPELLING_GATE`, `PYRE_GC_DIAG`,
`PYRE_MC_DIAG`) read as ON under the unqualified rule and are OFF in fact.

### §6a — Live default-ON (4): the removal targets

| gate | what is ON by default | retire when |
|---|---|---|
| PYRE_JD1 | the jd1 compiled-loop experiment (`eval.rs jd1_experiment_enabled`); `PYRE_NO_JD1` or `PYRE_JD1=0` turns it off, and no-JIT implies off | the jd1 experiment concludes |
| PYRE_JD1_NO_ENTER | entering the compiled jd1 loop directly rather than leaving the drain to the interpreter caller | with `PYRE_JD1` |
| PYRE_WALKABORT_OFF | the non-carrier walk-abort leg (`trace.rs walk_abort_leg_enabled`) | kept deliberately: the leg commits irrevocably once the blackhole runs, so it is the one-binary A/B for the bug class it sits in |
| PYRE_WASM_FULL_TEARDOWN | skipping the ~0.2s wasm engine teardown at exit; setting it restores the drops for leak diagnostics | when teardown stops being the dominant fixed startup tax |

### §6b — VALUE knobs (11): config, not gates

`PYRE_DYN_INDIRECT`, `PYRE_FBW_MULTIFRAME_DEPTH`, `PYRE_JD1_THRESHOLD`,
`PYRE_OPTION_RESIDUAL_NARROW`, `PYRE_PCMAP_RECIPE_RESULTCOLOR_AUDIT_PROBE`,
`PYRE_TRACE_CALL_DIAG`, `PYRE_TRACE_OPS_DIAG`,
`PYRE_WASM_FORCE_CA_TERMINAL_DECLINE`, `PYRE_WASM_FUEL`,
`PYRE_WASM_GUEST_PROFILE`, `PYRE_WASM_MODULE`.

### §6c — Default-OFF diagnostics, censuses and probes (51): keep, cost nothing

Each is inert unless set, so none is a removal target by this file's
already-ON criterion. They are listed so they cannot be missed again.

`PYRE_BH_NULL_ARG`, `PYRE_CALLEE_RCA`, `PYRE_DESCR_SPELLING_GATE`,
`PYRE_DIAG_51C`, `PYRE_DIAG_GIN`, `PYRE_DIAG_INLINE_RECOG`,
`PYRE_DYNASM_EXEC_DIAG`, `PYRE_FBW_CENSUS`, `PYRE_FBW_INLINE_DIAG`,
`PYRE_FBW_LOOPBODY_SCAN_FULL`, `PYRE_FBW_LOOPBODY_SCAN_LOOP_ONLY`,
`PYRE_FBW_MF_DIAG`, `PYRE_FBW_STRICT_DIAG`, `PYRE_FIELD_IDENTITY_CENSUS`,
`PYRE_GC_DIAG`, `PYRE_GC_FREELIST_DIAG`, `PYRE_JD1_DEBUG`, `PYRE_JD1_DUMP`,
`PYRE_LB_SITE`, `PYRE_LLBC_SKIP_FINGERPRINT_CHECK`, `PYRE_LLBC_STRICT`,
`PYRE_M73_BACKXLAT_TWIN_AUDIT`, `PYRE_M73_EMPTYTWIN_CENSUS`,
`PYRE_M73_LASTINSTR_AUDIT`, `PYRE_M73_MIDBODY_CARRY_AUDIT`,
`PYRE_MAJIT_STATS_ANCESTOR`, `PYRE_MAJIT_STATS_ROOT_ONLY`, `PYRE_MC_DIAG`,
`PYRE_MIR_FRAMESTATE_STRICT`, `PYRE_NO_JD1`, `PYRE_NO_UNROLL`,
`PYRE_PCMAP_AFTERRESIDUAL_AUDIT`, `PYRE_PCMAP_CONTAINING_AUDIT`,
`PYRE_PCMAP_RECIPE_RESULTCOLOR_AUDIT`, `PYRE_PCMAP_RESIDUAL_CENSUS`,
`PYRE_PORTAL_RCA`, `PYRE_PROBE_BH_STARTUP`, `PYRE_PROBE_SNAPSHOT`,
`PYRE_PROBE_SUBSCR`, `PYRE_PROFILE_PIPELINE`, `PYRE_QMUT_MAPDICT_FORCE`,
`PYRE_RERAISE_DIAG`, `PYRE_SIZE_SHELL_OWNERS`, `PYRE_SNAPSHOT_DIAG`,
`PYRE_WASM_DUMP_BAD_TRACE`, `PYRE_WASM_EXEC_TRACE`, `PYRE_WASM_FBW_CENSUS`,
`PYRE_WASM_GUARD_CENSUS`, `PYRE_WASM_JIT_STATS`, `PYRE_WASM_NO_CACHE`,
`PYRE_WASM_STARTUP_TRACE`.

## Summary

| bucket | count |
Expand Down
42 changes: 19 additions & 23 deletions pyre/pyre-interpreter/src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1164,18 +1164,29 @@ pub unsafe fn walk_pyframe_roots_area(
}
}

/// Install the PyFrame GC root walker with the majit-gc collector.
/// The interpreter's process-global off-GC slots, as one root source.
///
/// Frame roots are not here: `PyFrame.locals_cells_stack_w` and the
/// thread-local exception carriers ride the per-mutator `PyFrameRootArea`
/// (`walk_pyframe_root_area`), which reaches every registered thread rather
/// than only the collecting one. What is left is genuinely process-global —
/// app-level interphook handles, the `threading` module's own roots, and the
/// faulthandler's — so it registers once for the process.
fn walk_interpreter_global_roots(visitor: &mut dyn FnMut(&mut majit_ir::GcRef)) {
walk_global_prebuilt_roots(visitor);
crate::module::thread::walk_thread_roots(visitor);
#[cfg(all(not(target_arch = "wasm32"), not(feature = "sandbox")))]
crate::module::faulthandler::handler::walk_faulthandler_roots(visitor);
}

/// Install the interpreter's process-global GC root walker with the majit-gc
/// collector.
///
/// Called once at process startup from the JIT driver / pyrex main.
/// Stored in a process-global fn-pointer cell (#396); calling again with
/// the same fn pointer is idempotent.
pub fn register_pyframe_root_walker() {
majit_gc::shadow_stack::register_extra_root_walker(walk_global_prebuilt_roots);
majit_gc::shadow_stack::register_extra_root_walker(crate::module::thread::walk_thread_roots);
#[cfg(all(not(target_arch = "wasm32"), not(feature = "sandbox")))]
majit_gc::shadow_stack::register_extra_root_walker(
crate::module::faulthandler::handler::walk_faulthandler_roots,
);
pub fn register_interpreter_global_root_walker() {
majit_gc::shadow_stack::register_extra_root_walker(walk_interpreter_global_roots);
}

thread_local! {
Expand Down Expand Up @@ -1232,21 +1243,6 @@ unsafe fn walk_in_flight_exception_area(
unsafe { walk_raw_exception_roots(exc, visitor) };
}

/// Root the residual-call raise carried in the blackhole `BH_LAST_EXC_VALUE`
/// cell. A residual `bh_call` that raised publishes the exception's raw pointer
/// here (`publish_residual_call_exception`); between that write and the frame
/// that catches it (`route_to_catch` / the eval-loop walker-skip path) the
/// exception is held only in this raw `i64` cell, invisible to the precise
/// collector — a safepoint major would sweep its oldgen traceback chain. Mark
/// it (and its children) so the whole graph survives. Only `0` (no pending
/// raise) is skipped; production never stores a non-pointer value here (the
/// `0xDEAD` sentinel exists solely in a blackhole unit-test helper).
fn walk_bh_last_exception(visitor: &mut dyn FnMut(&mut majit_ir::GcRef)) {
majit_metainterp::blackhole::BH_LAST_EXC_VALUE.with(|c| {
unsafe { walk_raw_exception_cell_area(c as *const _, visitor) };
});
}

/// Forward one raw `i64` exception carrier cell and trace the exception's
/// GC-managed children. Shared by every such carrier
/// (`BH_LAST_EXC_VALUE`, `GUARD_EXC_VALUE`, `TL_JIT_PENDING_EXCEPTION`) so they
Expand Down
6 changes: 3 additions & 3 deletions pyre/pyre-jit/src/call_jit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,9 +473,9 @@ extern "C" fn jit_exc_raise_shim(value: i64) {
/// helper's NULL result flows to the consumer — keep both states in sync.
///
/// Both cells this writes — `BH_LAST_EXC_VALUE` and the backend `JIT_EXC_VALUE`
/// (via `store_jit_exception`) — are GC-rooted by their respective extra-root
/// walkers (`walk_bh_last_exception`, `walk_jit_exc_value`), so this writer
/// needs no rooting of its own.
/// (via `store_jit_exception`) — are GC-rooted by `walk_parked_exception_roots`
/// and by the per-mutator `PyFrameRootArea`, so this writer needs no rooting of
/// its own.
fn publish_residual_call_exception(exc_obj: i64) {
// Every residual raise enters the two exception channels here, so this is
// the one place that can hold the line on what they may carry — and the
Expand Down
76 changes: 44 additions & 32 deletions pyre/pyre-jit/src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3909,34 +3909,61 @@ fn walk_rbigint_parts_cache(visitor: &mut dyn FnMut(&mut majit_ir::GcRef)) {
});
}

/// Phase B: root walkers that reference interpreter state (immortal dicts,
/// mapdict side table, etc.). Called on first eval entry, after the
/// interpreter is initialized.
fn install_gc_root_walkers() {
pyre_interpreter::eval::register_pyframe_root_walker();
majit_gc::shadow_stack::register_extra_root_walker(walk_jit_exc_value);
majit_gc::shadow_stack::register_extra_root_walker(walk_bh_last_exc_value);
majit_gc::shadow_stack::register_extra_root_walker(walk_guard_exc_value);
majit_gc::shadow_stack::register_extra_root_walker(walk_rbigint_parts_cache);
/// Every exception parked outside GC discipline, as one root source.
///
/// Each carrier below holds a `W_BaseException` in a cell the precise collector
/// cannot reach — a backend static, a raw TLS `i64`, a `PyError` in flight, the
/// active `PyreSym` — for the window between the store and whatever drains it.
/// They share one registration rather than taking a registry slot apiece
/// because they are one *kind* of root storage, which is the granularity
/// `framework.py root_walker.walk_roots` registers at: a fixed set of sources,
/// each enumerating its own population.
fn walk_parked_exception_roots(visitor: &mut dyn FnMut(&mut majit_ir::GcRef)) {
walk_jit_exc_value(visitor);
walk_bh_last_exc_value(visitor);
walk_guard_exc_value(visitor);
// Children of the off-GC exception singletons, which no carrier and no
// collection phase reaches on its own.
majit_gc::shadow_stack::register_extra_root_walker(walk_immortal_exception_singleton_roots);
walk_immortal_exception_singleton_roots(visitor);
// Stored `PyError` carrier whose GC refs the precise collector cannot
// reach through its raw TLS cell. Mirrors `walk_pending_call_error`.
majit_gc::shadow_stack::register_extra_root_walker(crate::call_jit::walk_last_ca_exception);
crate::call_jit::walk_last_ca_exception(visitor);
// Exception parked for the next interpreter call boundary (JIT prologue
// overflow, or a jd1 drain error handed back to the caller loop): live in
// a raw TLS cell across the collecting code that runs before the drain.
majit_gc::shadow_stack::register_extra_root_walker(
pyre_interpreter::stack_check::walk_jit_pending_exception,
);
pyre_interpreter::stack_check::walk_jit_pending_exception(visitor);
// Trace-time exception carriers held only in the active `PyreSym`
// (`trace_built_exc` / `last_exc_value` / `current_exc_value`): a
// trace-built exception is unreachable to the precise collector between its
// construction and the RAISE_VARARGS lift-out.
majit_gc::shadow_stack::register_extra_root_walker(
pyre_jit_trace::trace::walk_active_sym_exc_roots,
);
pyre_jit_trace::trace::walk_active_sym_exc_roots(visitor);
}

/// The immortal, process-global stores whose GC-heap slots nothing else
/// forwards, as one root source.
///
/// Every holder here is `malloc_typed` or a plain static, so the marker skips
/// it and this walk is the only path to its contents. All four are
/// process-global rather than per-mutator for the same reason: each outlives
/// the thread that filled it. `w_globals` (`pycode.py:159-165
/// frame_stores_global`) is first-store-wins, `_mapdict_caches[i].w_method`
/// (mapdict.py:1418) is filled once, and a compiled `W_SRE_Pattern` outlives
/// its compiling thread — as per-mutator areas their slots would lose their
/// root at that thread's `unregister_mutator` while the holder stayed live.
fn walk_immortal_store_roots(visitor: &mut dyn FnMut(&mut majit_ir::GcRef)) {
walk_rbigint_parts_cache(visitor);
sre_pattern_root_walker(visitor);
w_globals_stamped_code_root_walker(visitor);
mapdict_method_cache_root_walker(visitor);
}

/// Phase B: root walkers that reference interpreter state (immortal dicts,
/// mapdict side table, etc.). Called on first eval entry, after the
/// interpreter is initialized.
fn install_gc_root_walkers() {
pyre_interpreter::eval::register_interpreter_global_root_walker();
majit_gc::shadow_stack::register_extra_root_walker(walk_parked_exception_roots);
majit_gc::shadow_stack::register_extra_root_walker(walk_immortal_store_roots);
// The mapdict side tables are keyed by owner address and root their
// values, so a major collection has to drop the entries whose owner it is
// about to sweep.
Expand All @@ -3949,21 +3976,6 @@ fn install_gc_root_walkers() {
// `MetaInterp::forced_virtuals` is the same shape but lives in one mutator's
// `JIT_DRIVER` rather than a global table, so it registers per mutator
// instead — see `forced_virtuals_pruner_area`.

// GC-heap slots of the immortal, process-global `W_SRE_Pattern` store. The
// patterns are `malloc_typed`, so nothing traces into them; a compiled
// pattern outlives the thread that compiled it, so its owner is a global
// walker rather than a per-mutator area.
majit_gc::shadow_stack::register_extra_root_walker(sre_pattern_root_walker);

// Same ownership argument for the two immortal-code-object registries.
// `w_globals` (`pycode.py:159-165 frame_stores_global`) is first-store-wins
// and `_mapdict_caches[i].w_method` (mapdict.py:1418) is filled once, so
// both slots outlive whichever thread stamped them; as per-mutator areas
// they lost their root at that thread's `unregister_mutator` while the
// code object stayed live and callable.
majit_gc::shadow_stack::register_extra_root_walker(w_globals_stamped_code_root_walker);
majit_gc::shadow_stack::register_extra_root_walker(mapdict_method_cache_root_walker);
}

fn register_thread_root_areas() {
Expand Down
Loading
Loading