diff --git a/pyre/bench/synth/getframe_inline_subwalk_multiframe.py b/pyre/bench/synth/getframe_inline_subwalk_multiframe.py index 59ad5a46bef..e0e106a27bb 100644 --- a/pyre/bench/synth/getframe_inline_subwalk_multiframe.py +++ b/pyre/bench/synth/getframe_inline_subwalk_multiframe.py @@ -54,7 +54,7 @@ def leaf(x): - sys._getframe(0).f_locals + sys._getframe(0).f_locals # noqa: B018 — the read itself is the force under test return sys._getframe(2).f_locals["base"] + x diff --git a/pyre/bench/synth/getframe_while_escaping_read_frame_identity.py b/pyre/bench/synth/getframe_while_escaping_read_frame_identity.py index 50e62085f7c..1f9839e1cdf 100644 --- a/pyre/bench/synth/getframe_while_escaping_read_frame_identity.py +++ b/pyre/bench/synth/getframe_while_escaping_read_frame_identity.py @@ -34,8 +34,8 @@ # Both reads still escape — `_gf()` names `leaf_a`'s published frame and # `_gf(1)` names `part_b`'s portal — but only once per call now that # `sys._getframe` forces the frame it RETURNS and not also the top of the stack. -# The multi-frame adopts this file exists for are unmoved at 10; what the -# duplicate escape carried was the single-frame count, 5 -> 0, alongside +# The multi-frame adoption count this file exists for is unmoved at 10. What +# the duplicate escape carried was the single-frame count, 5 -> 0, alongside # `part_a`'s loop compiling. import sys diff --git a/pyre/bench/synth/sys_audit_hooks.py b/pyre/bench/synth/sys_audit_hooks.py index 04f62904192..203aebbc2a6 100644 --- a/pyre/bench/synth/sys_audit_hooks.py +++ b/pyre/bench/synth/sys_audit_hooks.py @@ -10,9 +10,18 @@ # * `sys.audit` is free when no hook is installed, so the count a hook sees # starts at the first `addaudithook`, not at interpreter start; # * installing a hook emits `sys.addaudithook` to the hooks already there, -# and a `RuntimeError` out of that event means those hooks REFUSED the new -# one: it is dropped and the refusal does not propagate. Any other -# exception does propagate. +# and an `Exception` out of that event means those hooks REFUSED the new +# one: it is dropped and the refusal does not propagate. A `BaseException` +# outside `Exception` does propagate -- not exercised here, and it cannot +# be: a hook is installed for the life of the interpreter and the first one +# that raises masks every hook behind it, so reaching the propagating branch +# needs the hook set cleared between cases. Upstream's own facility for +# that (`__pypy__._testing_clear_audithooks`, `interp_magic.py:292`) refuses +# to run once translated, so no app-level program on a built interpreter +# can get there. The refusal below raises from app code, so it carries a +# real exception object and takes `error_is_exception`'s isinstance arm; +# the `PyErrorKind` fallback behind it answers only for an error that never +# materialised one, which nothing app-level can hand this event. # * `__cantrace__` on a hook is honoured (the flag exists so a tracing hook # can opt back in); nothing here can observe the tracing state, so only the # attribute lookup path is exercised. diff --git a/pyre/pyre-interpreter/src/module/sys/vm.rs b/pyre/pyre-interpreter/src/module/sys/vm.rs index c932066ed5e..58f978c1ebe 100644 --- a/pyre/pyre-interpreter/src/module/sys/vm.rs +++ b/pyre/pyre-interpreter/src/module/sys/vm.rs @@ -726,7 +726,16 @@ pub fn getframe(depth: i64) -> crate::PyResult { // force is what makes those reads see the JIT's live virtualizable fields — // upstream gets that ordering from the `hook_access_field` injection at // each field read, which pyre has relocated to this one call site. - audit("sys._getframe", &[current as PyObjectRef])?; + if audit_hooks_armed() { + // The wrap of the event name and the hooks themselves are both + // collection points, and the frame this is about to return reaches them + // only as a copied pointer, which the collector does not rewrite. Root + // it across the emit and read the answer back out of its slot. + let _roots = pyre_object::gc_roots::push_roots(); + let frame_slot = pyre_object::gc_roots::pin_roots(&[current as PyObjectRef]); + audit("sys._getframe", &[current as PyObjectRef])?; + current = pyre_object::gc_roots::shadow_stack_get(frame_slot) as *mut crate::PyFrame; + } Ok(current as PyObjectRef) } @@ -1287,15 +1296,7 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { if ec.is_null() { return Ok(pyre_object::w_none()); } - // Force the frame `topframeref` names before walking. Kept where - // [`getframe`] no longer has it: this walk takes no force on the - // frame it ENDS at and then reads that frame's `w_globals`, so - // whether the force belongs here at all is a separate question from - // the one settled above. - let mut current = unsafe { - (*ec).gettopframe(); - (*ec).gettopframe_nohidden() - }; + let mut current = unsafe { (*ec).gettopframe_nohidden() }; // `while (f && (_PyFrame_IsIncomplete(f) || depth-- > 0))` — the // post-decrement test fails immediately for a negative depth, so a // negative walks zero frames and reports the current module rather @@ -1308,6 +1309,13 @@ pub fn register_module(ns: pyre_object::PyObjectRef) { if current.is_null() { return Ok(pyre_object::w_none()); } + // `w_globals` is one of the six fields `interp_jit.py:25-30` + // declares virtualizable, so the frame it is read off has to be + // materialized first. The force belongs HERE, at the consumer, and + // not at the walk that reached the frame — see [`force_frame`]: + // forcing a walk escapes the traced virtualizable and + // `vable_after_residual_call` aborts the trace with ABORT_ESCAPE. + crate::executioncontext::force_frame(current); let w_globals = unsafe { (*current).w_globals }; if w_globals.is_null() { return Ok(pyre_object::w_none()); @@ -2642,23 +2650,36 @@ fn trigger_audit_events( w_event: pyre_object::PyObjectRef, args_w: &[pyre_object::PyObjectRef], ) -> Result<(), crate::PyError> { - let _roots = pyre_object::gc_roots::push_roots(); - pyre_object::gc_roots::pin_root(w_event); - // Before the tuple is built, not after: its allocation can collect, and the - // caller's arguments are reachable only through the borrowed slice. - for &w_arg in args_w { - pyre_object::gc_roots::pin_root(w_arg); - } - let w_args = pyre_object::tupleobject::w_tuple_new(args_w.to_vec()); - pyre_object::gc_roots::pin_root(w_args); + // `pin_root` copies the pointer into a shadow-stack slot and the collector + // rewrites THAT slot, never the local it was copied from. So every value + // still needed after one of the app-level calls below is reached through + // its slot index, the way `baseobjspace::isinstance` does it. + // + // The event, the arguments and the hook set are one livevar set spanning + // three slices, so they are published together and normalized once + // (`gc_roots::pin_roots`): a per-value pin queries the collector after the + // first write, which would let a foreign collection run while the values + // behind it were still invisible to it. + // // A hook may install another hook, and upstream's list is replaced rather // than appended to, so an in-flight trigger keeps iterating the set it - // started with. Snapshotting into pinned roots reproduces that and keeps - // every callable forwarded across the calls below. - let hooks_w = holder.hooks_w.clone(); - for &w_hook in &hooks_w { - pyre_object::gc_roots::pin_root(w_hook); - } + // started with. The published slots ARE that snapshot — they keep every + // callable forwarded across the calls below and are unaffected by a + // replacement of `holder.hooks_w`. + let _roots = pyre_object::gc_roots::push_roots(); + let event_slot = pyre_object::gc_roots::publish_roots(&[w_event]); + let args_slot = pyre_object::gc_roots::publish_roots(args_w); + let hooks_slot = pyre_object::gc_roots::publish_roots(&holder.hooks_w); + let hook_count = holder.hooks_w.len(); + pyre_object::gc_roots::normalize_roots(event_slot, 1 + args_w.len() + hook_count); + + // From the published slots, not from the caller's slice: the tuple's own + // allocation can collect, and so can everything reached before it. + let items = (0..args_w.len()) + .map(|i| pyre_object::gc_roots::shadow_stack_get(args_slot + i)) + .collect(); + let args_tuple_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(pyre_object::tupleobject::w_tuple_new(items)); let ec = crate::call::getexecutioncontext() as *mut crate::executioncontext::ExecutionContext; // don't trace audithooks by default @@ -2666,8 +2687,23 @@ fn trigger_audit_events( unsafe { (*ec).is_tracing += 1 }; } let mut result = Ok(()); - for &w_hook in &hooks_w { - let cantrace = match crate::baseobjspace::findattr(w_hook, "__cantrace__") { + for i in 0..hook_count { + let w_hook = pyre_object::gc_roots::shadow_stack_get(hooks_slot + i); + // `space.findattr` (`baseobjspace.py:881-888`) answers `None` for ANY + // non-async error out of the lookup, so a hook whose `__cantrace__` + // descriptor raises is simply treated as not having one. The bare + // `findattr` panics on those instead, and this argument is app code, so + // it is the async arm alone (`error.py:62-65`; pyre carries SystemExit + // of that pair today) that may travel out of here. + let w_cantrace = match crate::baseobjspace::findattr_result(w_hook, "__cantrace__") { + Ok(found) => found, + Err(err) if err.kind == crate::PyErrorKind::SystemExit => { + result = Err(err); + break; + } + Err(_) => None, + }; + let cantrace = match w_cantrace { None => false, Some(w_cantrace) => match crate::baseobjspace::is_true(w_cantrace) { Ok(cantrace) => cantrace, @@ -2680,7 +2716,13 @@ fn trigger_audit_events( if cantrace && !ec.is_null() { unsafe { (*ec).is_tracing -= 1 }; } - let w_result = crate::baseobjspace::call_function(w_hook, &[w_event, w_args]); + let w_result = crate::baseobjspace::call_function( + pyre_object::gc_roots::shadow_stack_get(hooks_slot + i), + &[ + pyre_object::gc_roots::shadow_stack_get(event_slot), + pyre_object::gc_roots::shadow_stack_get(args_tuple_slot), + ], + ); if cantrace && !ec.is_null() { unsafe { (*ec).is_tracing += 1 }; } @@ -2716,7 +2758,17 @@ pub fn audit(event: &str, args_w: &[pyre_object::PyObjectRef]) -> Result<(), cra if !audit_hooks_armed() { return Ok(()); } - audit_w(w_str_new(event), args_w) + // The wrap is a collection point and the arguments reach it only as copied + // pointers — `call_function_impl_result` reloads its own from the shadow + // stack for exactly that reason — so they are rooted in front of it and the + // emit runs off the reloaded values. + let _roots = pyre_object::gc_roots::push_roots(); + let args_slot = pyre_object::gc_roots::pin_roots(args_w); + let w_event = w_str_new(event); + let args_w: Vec = (0..args_w.len()) + .map(|i| pyre_object::gc_roots::shadow_stack_get(args_slot + i)) + .collect(); + audit_w(w_event, &args_w) } /// `vm.py:481 holder.hooks_w is None`, negated. A JIT fold that answers a call @@ -2764,9 +2816,18 @@ fn sys_audit(args: &[pyre_object::PyObjectRef]) -> crate::PyResult { } // The `@unwrap_spec` round trip is observable: the hooks are handed the // `str` the unwrapped name is re-wrapped as, so a `str` subclass reaches - // them flattened to a plain one. - let event = crate::baseobjspace::str_utf8_w(w_event)?; - audit_w(w_str_new(event), &positional[1..])?; + // them flattened to a plain one. The owned copy comes first so no borrow + // of `w_event` is live across the rooting below, and the arguments behind + // the event are rooted in front of the re-wrap because they reach it only + // as copied pointers — the same bracket [`audit`] takes around its own. + let event = crate::baseobjspace::str_utf8_w(w_event)?.to_string(); + let _roots = pyre_object::gc_roots::push_roots(); + let args_slot = pyre_object::gc_roots::pin_roots(&positional[1..]); + let w_text = w_str_new(&event); + let args_w: Vec = (0..positional.len() - 1) + .map(|i| pyre_object::gc_roots::shadow_stack_get(args_slot + i)) + .collect(); + audit_w(w_text, &args_w)?; Ok(w_none()) } @@ -2781,23 +2842,33 @@ fn sys_addaudithook(args: &[pyre_object::PyObjectRef]) -> crate::PyResult { "addaudithook() missing 1 required positional argument", )); }; + // The event below runs the already-installed hooks, which is app code and + // can collect, so the pin has to precede it — and the value stored after it + // has to come back out of the slot, since a relocation rewrites the slot + // and not this local. + let _roots = pyre_object::gc_roots::push_roots(); + let hook_slot = pyre_object::gc_roots::shadow_stack_len(); + pyre_object::gc_roots::pin_root(w_hook); if let Err(err) = audit("sys.addaudithook", &[]) { if !error_is_exception(&err) { return Err(err); } return Ok(w_none()); } - let _roots = pyre_object::gc_roots::push_roots(); - pyre_object::gc_roots::pin_root(w_hook); let holder = audit_holder(); unsafe { // `holder.hooks_w = holder.hooks_w + [w_hook]` — a fresh list, so a // `trigger_audit_events` already iterating the old one keeps the set it - // started with. - let old = &(*holder).hooks_w; - let mut next = Vec::with_capacity(old.len() + 1); - next.extend_from_slice(old); - next.push(w_hook); + // started with. The new slice is complete before the notification, so + // no borrow of `holder.hooks_w` is live across it. + let mut next = { + let old: &[pyre_object::PyObjectRef] = &(*holder).hooks_w; + let mut next = Vec::with_capacity(old.len() + 1); + next.extend_from_slice(old); + next + }; + next.push(pyre_object::gc_roots::shadow_stack_get(hook_slot)); + let next = next.into_boxed_slice(); // Notification precedes the store, as `rclass.py:1010-1012 // hook_setfield` emits `jit_force_quasi_immutable` before `setfield`. // The `is_installed()` fast path is `quasiimmut.py:38-41 invalidation`'s @@ -2805,7 +2876,7 @@ fn sys_addaudithook(args: &[pyre_object::PyObjectRef]) -> crate::PyResult { if (*holder).hooks_watchers.is_installed() { pyre_object::quasiimmut::sweep_quasi_immut_field(&(*holder).hooks_watchers); } - (*holder).hooks_w = next.into_boxed_slice(); + (*holder).hooks_w = next; (*holder).hooks_armed.set(true); } Ok(w_none()) diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs index 4d2f0d94767..4be3bbc18f1 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -1359,8 +1359,16 @@ pub fn flush_active_frame_escape(ctx: &TraceCtx, frame: *mut pyre_interpreter::P // A declined full flush still escaped the virtualizable, so the // locals region is written anyway (`virtualizable.py:101-138 // write_boxes` has no decline) — otherwise the callee reads an - // array of nulls. That write claims no resume pc, and the undo - // stays armed so the legacy replay re-enters the pre-flush frame. + // array of nulls. That write claims no resume pc, and NOTHING + // restores the pre-flush frame from here: the committed-pc + // walk-end leg is gated on a pc this arm never sets and the + // deferred leg on a flag it never arms, so the capture simply + // sits until [`capture_escape_flush_undo`] supersedes it or the + // walk-start reset drops it. The deferred arm this arm used to + // carry was withdrawn once the `value-stack underflow` it was + // added for stopped reproducing on artefacts that pass + // `PYRE_LLBC_STRICT=1` — 0/10 on cranelift without it, with the + // whole synthetic suite and every recorded counter unmoved. // // Upstream reports the escape from the vable token state alone, // independent of any resume-image write. See diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index ca37b02cc5b..b00e5dc79dd 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -3283,10 +3283,14 @@ pub(crate) fn note_inline_subwalk_end( jd_no: usize, pos: majit_metainterp::recorder::TracePosition, ) { - majit_metainterp::mc_diag_bump(59); + // Below the driver lookup, matching where `note_inline_subwalk_start` + // bumps 58: both counters then measure an APPENDED ENTRY, so an unclosed + // entry left by a driverless call shows up as `ptp_push != ptp_pop` rather + // than hiding behind equal counts. let Some((driver, _)) = crate::driver::try_driver_pair() else { return; }; + majit_metainterp::mc_diag_bump(59); driver .meta_interp_mut() .push_portal_trace_position(jd_no, None, pos);