From d5b810806453620c0997b6e7e2f427d64052717f Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 5 Aug 2026 13:22:34 +0900 Subject: [PATCH] interp: correct four comments about why the frame virtualizable forces exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four doc comments around the hand-placed `force_frame` calls attributed upstream's freedom from them to escape analysis, and attributed pyre's own materialization to the frame-chain walk. Both are wrong, and they are the statements that read as a licence to delete the calls. Upstream injects the force: `rvirtualizable.py:49-53 hook_access_field` genops `jit_force_virtualizable` on every redirected field access, `virtualizable.py:288-292` rewrites those into a `force_virtualizable_if_necessary` call across `translator.graphs`, and `jtransform.py:2164-2172 rewrite_op_jit_force_virtualizable` drops it again only in the graphs the codewriter looks inside. `executioncontext.rs:13-26` already said this; `:36-43` a few lines below said escape analysis instead. Pyre has no such injection: `rclass.rs buildinstancerepr` returns an error for any `_virtualizable_` class, naming `FieldListAccessor` / `_parse_field_list` parity as the blocker, and `rvirtualizable.rs should_force_field` has no callers. So the hand-placed calls are that mechanism relocated to a hand-picked consumer set. `gettopframe_nohidden` does not substitute for them. It forces the vref of the frame it starts from and then walks `f_backref` unforced, so it materializes the top frame only. That is why `locals()` works — it reports on the top frame — and `builtins.rs topframe_for_locals` claimed a per-frame `force_frame` instead, citing `executioncontext.rs:409-421`, which is now `clone_for_thread` internals. `typedef.rs` f_locals claimed `sys._getframe` gets the materialization "for free" from the same walk; it comes from `getframe`'s own `force_frame`. Comments only. Assisted-by: Claude --- pyre/pyre-interpreter/src/builtins.rs | 15 +++++---- pyre/pyre-interpreter/src/executioncontext.rs | 32 ++++++++++++++----- pyre/pyre-interpreter/src/module/sys/vm.rs | 12 ++++--- pyre/pyre-interpreter/src/typedef.rs | 8 +++-- 4 files changed, 46 insertions(+), 21 deletions(-) diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index d8ab48e5641..8abe0c86854 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -11154,12 +11154,15 @@ fn builtin_globals(args: &[PyObjectRef]) -> Result /// (`interp_inspect.py:7-11 locals` — `ec.gettopframe_nohidden()`). /// /// Going through `gettopframe_nohidden` is what makes the frame's fastlocals -/// readable: it runs `force_frame` on every frame it walks -/// (`executioncontext.rs:409-421`), and `fast2locals` reads -/// `locals_cells_stack_w` directly, so an unforced virtualizable hands back an -/// array of nulls — which `fast2locals` renders as an EMPTY mapping rather -/// than a stale one. Reading `CURRENT_FRAME` instead skips that force, and -/// under the JIT the caller then sees no locals at all. +/// readable, but the mechanism is the VREF force on its first line, not a +/// per-frame `force_frame`: the walk itself only follows `f_backref` and tests +/// `hide()`. That materializes the top frame and nothing below it, which is +/// exactly what this consumer needs — `locals()` reports on the top frame. +/// `getdictscope` reads `locals_cells_stack_w` directly, so an unforced +/// virtualizable hands back an array of nulls, which `fast2locals` renders as +/// an EMPTY mapping rather than a stale one. Reading `CURRENT_FRAME` instead +/// skips the vref force, and under the JIT the caller then sees no locals at +/// all. fn topframe_for_locals() -> *mut crate::PyFrame { let ec = crate::call::getexecutioncontext() as *mut crate::PyExecutionContext; if ec.is_null() { diff --git a/pyre/pyre-interpreter/src/executioncontext.rs b/pyre/pyre-interpreter/src/executioncontext.rs index bb7080db6cc..735439abc81 100644 --- a/pyre/pyre-interpreter/src/executioncontext.rs +++ b/pyre/pyre-interpreter/src/executioncontext.rs @@ -33,14 +33,30 @@ pub fn force_frame(frame: *mut PyFrame) { /// Force a frame whose fastlocals application code is about to read. /// -/// RPython needs no such call: storing the frame pointer anywhere the JIT -/// cannot see forces the virtualizable by escape analysis, so -/// `pyframe.py:539 fast2locals` always finds a materialized -/// `locals_cells_stack_w`. Pyre forces through explicit hooks instead, and -/// [`PyExecutionContext::gettopframe_nohidden`] only covers the frames IT -/// walks — a frame handed out some other way (a traceback's `tb_frame`) -/// reaches `fast2locals` unforced, whose null slots render as an EMPTY -/// mapping rather than a stale one. +/// RPython needs no such call because the force is injected for it, not +/// because none happens: `rvirtualizable.py:49-53 hook_access_field` genops +/// `jit_force_virtualizable` on every redirected FIELD access, +/// `virtualizable.py:288-292` rewrites those into a +/// `force_virtualizable_if_necessary` call across `translator.graphs`, and +/// `jtransform.py:2164-2172 rewrite_op_jit_force_virtualizable` drops it again +/// only in the graphs the codewriter looks inside. So `pyframe.py:539 +/// fast2locals` always finds a materialized `locals_cells_stack_w` with no +/// hand-placed hook anywhere. +/// +/// Pyre's rtyper declines to build a virtualizable `InstanceRepr` at all +/// (`rclass.rs buildinstancerepr`, blocked on `FieldListAccessor` / +/// `_parse_field_list` parity), so that injection does not exist here and +/// explicit calls like this one stand in for it — load-bearing, not redundant. +/// Deleting them was measured: `f_lineno` starts reporting the `def` line, +/// `f_locals` grows an entry, and the two read together segfault, with the +/// whole synthetic corpus green throughout. +/// +/// [`PyExecutionContext::gettopframe_nohidden`] does not substitute. It forces +/// the VREF of the frame it starts from and then walks `f_backref` unforced, so +/// it materializes the top frame only — enough for `locals()`, which reports on +/// that frame, and nothing for a frame handed out some other way (a traceback's +/// `tb_frame`), which reaches `fast2locals` unforced; its null slots render as +/// an EMPTY mapping rather than a stale one. /// /// # Safety /// `frame` must be a live `PyFrame` (or null). diff --git a/pyre/pyre-interpreter/src/module/sys/vm.rs b/pyre/pyre-interpreter/src/module/sys/vm.rs index 097b50f8c89..fcaadcb9212 100644 --- a/pyre/pyre-interpreter/src/module/sys/vm.rs +++ b/pyre/pyre-interpreter/src/module/sys/vm.rs @@ -566,10 +566,14 @@ fn simple_namespace_replace(args: &[PyObjectRef]) -> crate::PyResult { /// (`ExecutionContext::gettopframe_raw`), so `gettopframe_nohidden` is not /// "force-free" — it is free of the *virtualizable* force that /// `gettopframe` adds. Upstream takes neither of those, because -/// `look_inside_iff(isconstant(depth))` traces a constant `depth` THROUGH: the -/// walk never becomes a residual call, and the frame it hands to app level -/// materialises by escape analysis instead -/// (`executioncontext::force_frame_before_locals_read` states that contract). +/// `look_inside_iff(isconstant(depth))` traces a constant `depth` THROUGH, so +/// the walk never becomes a residual call — and the frame it hands to app level +/// materialises through the force `rvirtualizable.py:49-53 hook_access_field` +/// injects at every redirected FIELD access, which +/// `jtransform.py:2164-2172 rewrite_op_jit_force_virtualizable` deletes only in +/// the graphs the codewriter looks inside. Pyre has no such injection — +/// `rclass.rs buildinstancerepr` declines a virtualizable `InstanceRepr` +/// outright — so these two calls ARE that mechanism, relocated to one consumer. /// Pyre calls this residually, so both forces are load-bearing here: /// `gettopframe()` because a JIT-inlined callee has no frame of its own until a /// force materialises one — an unforced walk would start at the caller and then diff --git a/pyre/pyre-interpreter/src/typedef.rs b/pyre/pyre-interpreter/src/typedef.rs index b6d72f5d138..d3504be81e4 100644 --- a/pyre/pyre-interpreter/src/typedef.rs +++ b/pyre/pyre-interpreter/src/typedef.rs @@ -7211,9 +7211,11 @@ fn init_frame_type(ns: PyObjectRef) { // Both arms end at `fast2locals` (the proxy routes its reads // back through the frame), which reads `locals_cells_stack_w` // directly — so the virtualizable has to be materialized first or - // the mapping comes back EMPTY. `sys._getframe` gets that for - // free from `gettopframe_nohidden`; a frame reached through a - // traceback's `tb_frame` does not. + // the mapping comes back EMPTY. `sys._getframe` materializes it + // with its own explicit `force_frame` (`module/sys/vm.rs + // getframe`), not as a side effect of the walk: `gettopframe_nohidden` + // forces only the VREF of the frame it starts from. A frame + // reached through a traceback's `tb_frame` gets neither. crate::executioncontext::force_frame_before_locals_read(f); let frame = unsafe { &mut *f }; if frame.code().flags.contains(crate::CodeFlags::OPTIMIZED) {