From e8985049c60f52e4b6889276ba628939b9429f8d Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Thu, 20 Aug 2026 03:08:27 +0900 Subject: [PATCH 1/3] majit-translate: read a deref-last projection as a value, not an address `build_rvalue`'s `Rvalue::Ref` and `Rvalue::RawPtr` arms called every `PlaceKind::Projection` an address-of. `&(*p).f` names the field's address; `&*(*p).f` reads the pointer the field holds and dereferences it. Both are projections, so both were marked. `mark_place_address_of` sets `taken_by_address` on the descriptor of the last op the projection emitted, which is the `getfield`. `rewrite_op_getfield` folds `suppresses_virtualizable()` into `fresh_virtualizable`, and the `vable_array_vars` insert is gated on `!fresh_virtualizable`, so a marked read is never registered: it stays an ordinary `getarrayitem_gc` against the heap array, which is written back only at `sync_virtualizable_before_jit`, `sync_virtualizable_after_jit` and `sync_virtualizable_after_guard_failure`. `locals_w!` expands to `&*$frame.locals_cells_stack_w`, the deref-last shape, and it has 44 call sites across `pyframe.rs`, `eval.rs` and `builtins.rs`. `place_ref_is_address_of` spells the `Deref` test as `resolve_place` and `emit_projection_write` already spell it, applied one level out. `address_of_the_vable_array_slot_is_marked_not_a_read` keeps the `addr_of_mut!` shape, whose outermost step is a field, on the marked side. Assisted-by: Claude --- majit/majit-translate/src/front/mir.rs | 83 +++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 2 deletions(-) diff --git a/majit/majit-translate/src/front/mir.rs b/majit/majit-translate/src/front/mir.rs index f4c3ae97711..519d87d2ccf 100644 --- a/majit/majit-translate/src/front/mir.rs +++ b/majit/majit-translate/src/front/mir.rs @@ -4664,7 +4664,7 @@ impl<'a> Lowering<'a> { // itself. Aliasing the dest local to the referent Variable // keeps the IR small, treating `&x` as a same-Variable copy. Rvalue::Ref { place, .. } => { - let projection = matches!(&place.kind, PlaceKind::Projection(..)); + let projection = Self::place_ref_is_address_of(&place); let before = self.graph.block(self.block_id[mir_bb]).operations.len(); let v = self.resolve_place(mir_bb, place)?; self.mark_place_address_of(mir_bb, projection, before, &v); @@ -4675,7 +4675,7 @@ impl<'a> Lowering<'a> { // and references identically at the IR level (lifetime // tracking lives outside the JIT). Rvalue::RawPtr { place, .. } => { - let projection = matches!(&place.kind, PlaceKind::Projection(..)); + let projection = Self::place_ref_is_address_of(&place); let before = self.graph.block(self.block_id[mir_bb]).operations.len(); let v = self.resolve_place(mir_bb, place)?; self.mark_place_address_of(mir_bb, projection, before, &v); @@ -5375,6 +5375,24 @@ impl<'a> Lowering<'a> { Ok(var) } + /// Whether `&` / `&raw [mut] ` takes the address of a + /// place, as opposed to reading the value one holds. + /// + /// `&*(*p).f` — a projection whose outermost element is `Deref` — yields + /// the value the pointer in `(*p).f` holds, so the underlying read stays + /// a value read and keeps its lowering. `&(*p).f`, Field-last, is the + /// real place-address and stays marked. + /// + /// The `Deref` test is spelled as `resolve_place` and + /// `emit_projection_write` already spell it, applied one level out. + fn place_ref_is_address_of(place: &Place) -> bool { + match &place.kind { + PlaceKind::Projection(_, ProjectionElem::Atom(s)) if s == "Deref" => false, + PlaceKind::Projection(..) => true, + _ => false, + } + } + /// Record that the `Variable` just resolved for a place is the /// **address** of a field, not its value. /// @@ -21726,6 +21744,67 @@ mod tests { assert_eq!(resolve_to_producer_op(&graph, &Variable::new()), None); } + /// `&*(*p).f` reads the value the field holds; `&(*p).f` names the + /// field's address. + /// + /// Both reach `build_rvalue`'s `Ref` / `RawPtr` arms as a + /// `PlaceKind::Projection`, so a bare `matches!(.., Projection(..))` + /// calls them both an address-of. On `locals_cells_stack_w` the false + /// mark is not inert: `rewrite_op_getfield` reads + /// `suppresses_virtualizable` and skips the `vable_array_vars` + /// registration, so the read leaves the protocol and becomes a plain + /// `getarrayitem_gc` against a heap array that is only synchronised at + /// the three `sync_virtualizable_*` points — a stale read, not a slow + /// equivalent. `locals_w!` expands to the deref-then-ref spelling, so + /// every one of its readers took that path. + #[test] + fn a_deref_last_projection_reads_a_value_rather_than_naming_an_address() { + use super::Lowering; + use majit_charon_reader::ullbc::{Place, PlaceKind, ProjectionElem}; + + fn ty() -> TyRef { + TyRef::Other(serde_json::Value::Null) + } + fn place(kind: PlaceKind) -> Place { + Place { kind, ty: ty() } + } + fn field(inner: Place, name: &str) -> Place { + place(PlaceKind::Projection( + Box::new(inner), + ProjectionElem::Tagged(serde_json::json!({ "Field": name })), + )) + } + fn deref(inner: Place) -> Place { + place(PlaceKind::Projection( + Box::new(inner), + ProjectionElem::Atom("Deref".to_string()), + )) + } + + let local = || place(PlaceKind::Local(0)); + let is_addr = Lowering::place_ref_is_address_of; + + // `&(*p).f` — the outermost step names a field, so the reference + // stands for that field's address and the mark is owed. + assert!(is_addr(&field(deref(local()), "locals_cells_stack_w"))); + + // `&*(*p).f` — the outermost step dereferences what the field + // holds, so the reference stands for the pointee. The field read + // underneath is an ordinary value read. + assert!(!is_addr(&deref(field( + deref(local()), + "locals_cells_stack_w" + )))); + + // `&*p`, the same shape one level in, and a bare local: neither + // names a field address either. + assert!(!is_addr(&deref(local()))); + assert!(!is_addr(&local())); + + // A nested field is still an address-of at its outermost step. + assert!(is_addr(&field(field(deref(local()), "a"), "b"))); + } + #[test] fn items_block_base_accessor_gate_excludes_deref_in_place() { use super::graph_is_items_block_base_accessor; From 25e7f8a040f8b6daf5b7c9fc7a1dafdb1dfa9b3b Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Thu, 20 Aug 2026 03:08:45 +0900 Subject: [PATCH 2/3] pyre: compute a stack index before subscripting the frame array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subscript evaluates its receiver before its index expression, so `locals_w!(self)[self.valuestackdepth - 1 - depth]` emits the `locals_cells_stack_w` read first and the subtraction's overflow check after it. That check branches, so the array is defined in one block and consumed in another. Lowering `peek_at` shows it directly: the `FieldRead` is in bb0, the two `sub`s split bb0 from bb4 from bb5, and the `ArrayRead` is in bb5, with the array carried as a link argument the whole way. `_check_no_vable_array` rejects that — `Escaped via: link argument` — and the rejection is a `panic!` inside `generate_into`, which `pyre-jit-trace`'s `build.rs` calls with no `catch_unwind`, so it is a build failure rather than a degraded trace. The panic message names this cause first: "indexing with an index not known non-negative". `peekvalue_maybe_none` and `settopvalue` already compute the index into a local before the subscript, which is also how `pyframe.py:479-484` spells it, and lowering the former puts its `FieldRead` and `ArrayRead` in one block. Six sites did not: `peek`, `peek_at`, `peekvalues` (`base + idx`) and the three reads in `with_except_start`. The slice bounds check does not split a block — the front folds it into the `ArrayRead` — only the arithmetic does. Assisted-by: Claude --- pyre/pyre-interpreter/src/eval.rs | 10 +++++++--- pyre/pyre-interpreter/src/pyframe.rs | 17 ++++++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/pyre/pyre-interpreter/src/eval.rs b/pyre/pyre-interpreter/src/eval.rs index 67bab1e7af0..0515fac1a05 100644 --- a/pyre/pyre-interpreter/src/eval.rs +++ b/pyre/pyre-interpreter/src/eval.rs @@ -3485,9 +3485,13 @@ impl OpcodeStepExecutor for PyFrame { "WITH_EXCEPT_START requires five stack values", )); } - let val = locals_w!(self)[depth - 1]; - let exit_self = locals_w!(self)[depth - 4]; - let exit_func = locals_w!(self)[depth - 5]; + // Indices first: a subscript evaluates its receiver before its index + // expression, so arithmetic inside the brackets puts the subtraction's + // overflow check between the `locals_cells_stack_w` read and its use. + let (i_val, i_self, i_func) = (depth - 1, depth - 4, depth - 5); + let val = locals_w!(self)[i_val]; + let exit_self = locals_w!(self)[i_self]; + let exit_func = locals_w!(self)[i_func]; let res = with_except_start_values(exit_func, exit_self, val); if res.is_null() { return Err(crate::call::take_call_error() diff --git a/pyre/pyre-interpreter/src/pyframe.rs b/pyre/pyre-interpreter/src/pyframe.rs index ae0f8960127..eca1ba3197b 100644 --- a/pyre/pyre-interpreter/src/pyframe.rs +++ b/pyre/pyre-interpreter/src/pyframe.rs @@ -3025,13 +3025,23 @@ impl PyFrame { #[inline] pub fn peek(&self) -> PyObjectRef { - locals_w!(self)[self.valuestackdepth - 1] + // The index is computed into a local first, as + // `pyframe.py:479-484 peekvalue_maybe_none` computes `index` before + // its subscript. A subscript evaluates its receiver before its index + // expression, so spelling the arithmetic inside the brackets emits the + // `locals_cells_stack_w` read ahead of the subtraction's overflow + // check, and that check's branch then carries the array out of the + // block on a link — which `_check_no_vable_array` rejects. + let index = self.valuestackdepth - 1; + locals_w!(self)[index] } #[inline] #[allow(dead_code)] pub fn peek_at(&self, depth: usize) -> PyObjectRef { - locals_w!(self)[self.valuestackdepth - 1 - depth] + // Hoisted for the reason given on [`Self::peek`]. + let index = self.valuestackdepth - 1 - depth; + locals_w!(self)[index] } /// Null the locals_cells_stack slots at and above `depth`, the @@ -3143,7 +3153,8 @@ impl PyFrame { let mut idx = n; while idx > 0 { idx -= 1; - values_w[idx] = locals_w!(self)[base + idx]; + let slot = base + idx; + values_w[idx] = locals_w!(self)[slot]; } values_w } From 11925bcec74c369ef383b1203947118221fe6abc Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Thu, 20 Aug 2026 03:08:51 +0900 Subject: [PATCH 3/3] pyre: carry unroll_safe onto fast2locals `pyframe.py:572` decorates `fast2locals` `@jit.unroll_safe`. `look_inside_graph` cancels `contains_loop` for a graph carrying the hint (`policy.py:61-62`), so the slot loop no longer keeps the codewriter out. Assisted-by: Claude --- pyre/pyre-interpreter/src/pyframe.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pyre/pyre-interpreter/src/pyframe.rs b/pyre/pyre-interpreter/src/pyframe.rs index eca1ba3197b..be045eb7089 100644 --- a/pyre/pyre-interpreter/src/pyframe.rs +++ b/pyre/pyre-interpreter/src/pyframe.rs @@ -4155,6 +4155,13 @@ impl PyFrame { /// silently dropped). A function frame with no locals bound yet lazily /// allocates a fresh dict (pyframe.py:557 `self.space.newdict(instance=True)`) /// and caches it, so `locals() is locals()` holds. Errors propagate. + /// + /// `@jit.unroll_safe` (`pyframe.py:572`) cancels `contains_loop` in the + /// policy (`codewriter/policy.rs look_inside_graph`), so the slot loop + /// below does not keep the codewriter out. Without it the whole function + /// is one residual call, and the `f_locals` read behind it forces the + /// virtualizable for the length of that call. + #[majit_macros::unroll_safe] pub fn fast2locals(&mut self) -> Result<(), crate::PyError> { // `space.setitem_str` / `space.delitem` allocate one key per slot and // can collect. RPython's GC transform keeps both the frame and its