diff --git a/majit/majit-gc/src/collector.rs b/majit/majit-gc/src/collector.rs index bdee84e6505..91641e82c77 100644 --- a/majit/majit-gc/src/collector.rs +++ b/majit/majit-gc/src/collector.rs @@ -2127,6 +2127,8 @@ impl MiniMarkGC { &mut self, obj_addr: usize, site: &str, + // The child site names the slot kind; the root path identifies the producer. + parent_site: &'static str, holder_addr: usize, slot_addr: usize, ) -> GcRef { @@ -2164,6 +2166,7 @@ impl MiniMarkGC { panic!( "GC BUG: invalid type_id={} at obj_addr={:#x} \ (header_addr={:#x}, nursery_start={:#x}, site={}, \ + parent_site={}, \ nursery_free={:#x}, nursery_top={:#x}, holder_addr={:#x}, \ holder_type_id={:?}, holder_offset={:?}, holder_words={:#x?})", type_id, @@ -2171,6 +2174,7 @@ impl MiniMarkGC { obj_addr - GcHeader::SIZE, self.nursery.start_ptr() as usize, site, + parent_site, self.nursery.free_ptr() as usize, self.nursery.top_ptr() as usize, holder_addr, @@ -2281,6 +2285,8 @@ impl MiniMarkGC { slot_addr: usize, holder_addr: usize, site: &str, + // The child site names the slot kind; the root path identifies the producer. + parent_site: &'static str, ) { const NURSERY_POISON_WORD: usize = (usize::MAX / 0xff) * 0xaa; if self.nursery.poison_enabled() && field_ref.0 == NURSERY_POISON_WORD { @@ -2291,8 +2297,8 @@ impl MiniMarkGC { }; let holder_offset = slot_addr.checked_sub(holder_addr); panic!( - "GC BUG: traced slot contains nursery poison at slot_addr={:#x} holder_addr={:#x} holder_type_id={:?} holder_offset={:?} site={}", - slot_addr, holder_addr, holder_type_id, holder_offset, site, + "GC BUG: traced slot contains nursery poison at slot_addr={:#x} holder_addr={:#x} holder_type_id={:?} holder_offset={:?} site={} parent_site={}", + slot_addr, holder_addr, holder_type_id, holder_offset, site, parent_site, ); } } @@ -2311,11 +2317,18 @@ impl MiniMarkGC { /// white old objects to `more_objects_to_trace`. #[inline] fn drag_out_root(&mut self, gcref: &mut GcRef) { - self.assert_traced_slot_initialized(*gcref, gcref as *mut GcRef as usize, 0, "minor_root"); + self.assert_traced_slot_initialized( + *gcref, + gcref as *mut GcRef as usize, + 0, + "minor_root", + "minor_root", + ); let pinned = self.pinned_objects.contains(&gcref.0); if self.is_nursery_object_start(gcref.0) && !pinned { let slot_addr = gcref as *mut GcRef as usize; - *gcref = self.copy_nursery_object(gcref.0, "minor_root_target", 0, slot_addr); + *gcref = + self.copy_nursery_object(gcref.0, "minor_root_target", "minor_root", 0, slot_addr); } // incminimark.py:2140-2143: append iff (VISITED | PINNED) == 0. pyre's // marking convention sets VISITED at push time (see `seed_major_root` @@ -2332,7 +2345,7 @@ impl MiniMarkGC { /// Trace an object's GC pointer fields and update any that point /// into the nursery by copying the target. - fn trace_and_update_object(&mut self, obj_addr: usize, site: &str) { + fn trace_and_update_object(&mut self, obj_addr: usize, site: &'static str) { let type_id = unsafe { (*header_of(obj_addr)).type_id() }; self.validate_type_id(type_id, obj_addr, site); let custom_trace = self.types.get(type_id).custom_trace; @@ -2347,11 +2360,13 @@ impl MiniMarkGC { slot_ptr as usize, obj_addr, "minor_custom_trace", + site, ); if self.is_nursery_object_start(field_ref.0) { let new_ref = self.copy_nursery_object( field_ref.0, "minor_custom_trace_target", + site, obj_addr, slot_ptr as usize, ); @@ -2378,11 +2393,13 @@ impl MiniMarkGC { slot as usize, obj_addr, "minor_fixed_field", + site, ); if self.is_nursery_object_start(field_ref.0) { let new_ref = self.copy_nursery_object( field_ref.0, "minor_fixed_field_target", + site, obj_addr, slot as usize, ); @@ -2404,11 +2421,13 @@ impl MiniMarkGC { slot as usize, obj_addr, "minor_varsize_item", + site, ); if self.is_nursery_object_start(field_ref.0) { let new_ref = self.copy_nursery_object( field_ref.0, "minor_varsize_item_target", + site, obj_addr, slot as usize, ); @@ -4257,11 +4276,13 @@ impl MiniMarkGC { slot as usize, obj, "minor_dirty_card_item", + "minor_dirty_card", ); if self.is_nursery_object_start(field_ref.0) { let new_ref = self.copy_nursery_object( field_ref.0, "minor_dirty_card_item_target", + "minor_dirty_card", obj, slot as usize, ); diff --git a/majit/majit-translate/src/codewriter/jitcode.rs b/majit/majit-translate/src/codewriter/jitcode.rs index 0cd26c16f25..b4b02771c51 100644 --- a/majit/majit-translate/src/codewriter/jitcode.rs +++ b/majit/majit-translate/src/codewriter/jitcode.rs @@ -1084,8 +1084,18 @@ impl BhFieldSpec { /// `BhDescr::Size.all_fielddescrs` matching `descr.py:188 /// init_size_descr` parity. pub fn from_field_descr(fd: &dyn majit_ir::descr::FieldDescr) -> Self { + // descr.py:241-254 `get_type_flag`: a `Ptr` to a GC struct is + // FLAG_POINTER, and only a `Ptr` to a raw struct degrades to + // FLAG_UNSIGNED. pyre models the raw case as `Type::Int`, so a + // pointer field always round-trips as `Pointer` — the same mapping the + // codewriter's own `value_type_to_field_flag` and + // `bh_field_flag_from_descr` already use. Emitting `Unsigned` here + // made the round trip lossy: `SimpleFieldDescr::is_pointer_field()` is + // `flag == Pointer` (descr.py:173), so the rebuilt descr denied being + // a pointer field and `handle_write_barrier_setfield` dropped the + // store's write barrier. let field_flag = if fd.is_pointer_field() { - majit_ir::descr::ArrayFlag::Unsigned + majit_ir::descr::ArrayFlag::Pointer } else if fd.is_float_field() { majit_ir::descr::ArrayFlag::Float } else if fd.field_type() == majit_ir::value::Type::Void { diff --git a/pyre/bench/synth/_pending/exception_tb_f_locals_vref_root_walk.py b/pyre/bench/synth/_pending/exception_tb_f_locals_vref_root_walk.py new file mode 100644 index 00000000000..e861330e37d --- /dev/null +++ b/pyre/bench/synth/_pending/exception_tb_f_locals_vref_root_walk.py @@ -0,0 +1,86 @@ +# pyre-check: skip-backends=wasm +# +# PARKED: its guard_failures is not the same on every host. macos-latest and +# ubuntu-24.04 both report 5980 on cranelift; windows-latest reported 5979 in +# the run that promoted this file. That is the band #1043 removed the +# closure_per_call overlay over — two counters there disagreed with themselves +# across jobs, one toward its shared value and one away from it — so a +# `.cranelift.win32.jitstats` overlay cannot hold this either, and a missing +# baseline is a hard fail rather than an opt-out. The walker guard therefore has +# no suite gate; this file is the reproduction, and it is correct on all three +# native backends and PYRE_NO_JIT=1 at every size of a 512K-32M PYPY_GC_NURSERY +# sweep. +# +# wasm is exempted above. It reads the catching frame mid-`except` as if the +# implicit `del e` had already run, so `f_locals` loses `e` on part of the loop +# and a second tuple reaches `seen`: +# 2 [(('drive', ('e', 'k', 'seen')), 'mid'), (('drive', ('k', 'seen')), 'mid')] +# Compiled-code only — clean at N=4000, wrong from N=8000 — and measured +# identical with the whole source tree reset to the merge base, so it predates +# the walker guard this file gates. Same stale-`f_locals`-from-compiled-code +# class as getframe_caller_locals_nested_compiled_callee, exempted the same way: +# not because wasm is right here. +# +# Reading `f_locals` off the OUTERMOST traceback node — the catching frame — +# leaves a JIT virtual ref in that frame's `locals_cells_stack_w`. A minor +# collection triggered by a nursery allocation from compiled code then walks the +# slot as a GC root. A vref's leading word is the `JIT_VIRTUAL_REF_VTABLE` magic +# rather than a PyObject `ob_type`, so a root walker that hands the slot to the +# raw exception walk dereferences the magic as a type pointer and takes a +# SIGSEGV. Reading the innermost node instead is clean. +# +# The collection has to land while the vref is on the stack, which is why the +# loop is long: on cranelift the crash is deterministic from roughly the 6000th +# iteration (2000 and 4000 stay clean). dynasm and the plain interpreter never +# reach that GC point on this shape, so this costs all three backends a run to +# gate one of them. +# +# The argument form is for narrowing by hand — ` thisfile.py 4000 tail` +# and the rest. The suite runs it with none, which takes the defaults below. +# +# Expected output: 1 [(('drive', ('e', 'k', 'seen')), 'mid')] + +import sys + +N = int(sys.argv[1]) if len(sys.argv) > 1 else 15000 +WHICH = sys.argv[2] if len(sys.argv) > 2 else "head" + + +def mid(i): + raise ValueError("boom") + + +def locs(tb): + out = [] + idx = 0 + while tb is not None: + f = tb.tb_frame + want = ( + WHICH == "all" + or (WHICH == "head" and idx == 0) + or (WHICH == "tail" and tb.tb_next is None) + ) + if want: + out.append((f.f_code.co_name, tuple(sorted(f.f_locals)))) + else: + out.append(f.f_code.co_name) + tb = tb.tb_next + idx += 1 + return tuple(out) + + +def drive(): + seen = set() + k = 0 + while k < N: + try: + mid(k) + except ValueError as e: + seen.add(locs(e.__traceback__)) + e.__traceback__ = None + k += 1 + return sorted(seen) + + +r = drive() +print(len(r), r) diff --git a/pyre/bench/synth/_pending/gc_varsize_item_const_shape_witness.py b/pyre/bench/synth/_pending/gc_varsize_item_const_shape_witness.py new file mode 100644 index 00000000000..549f8192d0a --- /dev/null +++ b/pyre/bench/synth/_pending/gc_varsize_item_const_shape_witness.py @@ -0,0 +1,78 @@ +"""Deterministic witness for `GC BUG ... site=minor_varsize_item_target`. + +Run with no arguments. cranelift aborts 3/3; dynasm, `PYRE_NO_JIT=1`, pypy and +cpython all print `1 [(('drive', ('e', 'k', 'seen')), 'mid')]`. + +OPEN, and independent of the traceback-journal and virtual-ref root-walk fixes: +it reproduces identically with either of those reverted. + + GC BUG: invalid type_id= at obj_addr=0x...fc28 + (header_addr=0x...fc20, nursery_start=0x...b0000, + site=minor_varsize_item_target, nursery_free=0x...fee0, + nursery_top=0x...b0000, holder_addr=0x..., + holder_type_id=Some(9), holder_offset=Some(8), + holder_words=[0xd, 0x...fc28, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0]) + +Reached from `gc_alloc_nursery_shim` -> `alloc_with_type_slow` -> +`do_collect_nursery` -> `trace_and_update_object` -> `copy_nursery_object`, so +it is a minor collection triggered by a nursery allocation out of compiled code. + +What the message says: the holder is a varsize object of type_id 9 carrying its +length (13) at offset 0 and items from offset 8, and only `items[0]` is set. The +holder address is above `nursery_top`, so the holder is old-gen and `items[0]` +points into the nursery. The reported `type_id` differs every run and is +pointer-shaped, so the target's header is recycled memory rather than a live +object — the shape of an unbarriered old-to-young store whose target an earlier +minor collection already moved. + +The trigger is allocation layout, not the loop bounds. The same body reading +`N`/`WHICH` from `sys.argv` is clean when `15000 head` is passed, and aborts when +it takes the identical values from `else` defaults; wrapping those defaults to +defeat constant folding (`int("15000")`, `len(sys.argv)` arithmetic) does not +change it, which is what rules the folding explanation out. +""" + +N = 15000 +WHICH = "head" + +ERR = ValueError("boom") + + +def mid(i): + raise ValueError("boom") + + +def locs(tb): + out = [] + idx = 0 + while tb is not None: + f = tb.tb_frame + want = ( + WHICH == "all" + or (WHICH == "head" and idx == 0) + or (WHICH == "tail" and tb.tb_next is None) + ) + if want: + out.append((f.f_code.co_name, tuple(sorted(f.f_locals)))) + else: + out.append(f.f_code.co_name) + tb = tb.tb_next + idx += 1 + return tuple(out) + + +def drive(): + seen = set() + k = 0 + while k < N: + try: + mid(k) + except ValueError as e: + seen.add(locs(e.__traceback__)) + e.__traceback__ = None + k += 1 + return sorted(seen) + + +r = drive() +print(len(r), r) diff --git a/pyre/bench/synth/exception_inline_callee_tb_frame_locals.cranelift.jitstats b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.cranelift.jitstats new file mode 100644 index 00000000000..4665b398e2f --- /dev/null +++ b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.cranelift.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=1 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=1 diff --git a/pyre/bench/synth/exception_inline_callee_tb_frame_locals.dynasm.jitstats b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.dynasm.jitstats new file mode 100644 index 00000000000..4665b398e2f --- /dev/null +++ b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.dynasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=1 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=1 diff --git a/pyre/bench/synth/exception_inline_callee_tb_frame_locals.py b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.py new file mode 100644 index 00000000000..83314942c1d --- /dev/null +++ b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.py @@ -0,0 +1,44 @@ +# pyre-check: max-pypy-ratio=24 +# Regression guard: the frame an inlined callee's traceback node names must +# report the callee's NON-PARAMETER locals, not just the arguments it was +# entered with. +# +# A `STORE_FAST` on the callee's own fresh frame is folded to an SSA register +# and emits no store into `locals_cells_stack_w`, so the array holds only the +# parameters seeded at the inline. Storing that frame into `PyTraceback.frame` +# lets it outlive the trace, and without replaying the fold first every +# `tb_frame.f_locals` consumer — `traceback` formatting and debuggers among +# them — silently loses `marker`. +# +# The loop has to pass the compile threshold: the interpreted iterations write +# the live frame directly and read correctly either way, so a short run cannot +# see this. Before the fix, dynasm and cranelift both lost `marker` from the +# moment the loop compiled — 1242 of 4000 iterations still answered +# `('i', 'marker')`, the rest `('i',)`. +# +# Expected output: 4000 ('i', 'marker') + +N = 4000 + + +def mid(i): + marker = i * 2 + raise ValueError(marker) + + +def drive(): + kinds = {} + k = 0 + while k < N: + try: + mid(k) + except ValueError as e: + tb = e.__traceback__.tb_next + names = tuple(sorted(tb.tb_frame.f_locals)) if tb is not None else None + kinds[names] = kinds.get(names, 0) + 1 + k += 1 + return kinds + + +for names, count in sorted(drive().items(), key=lambda kv: -kv[1]): + print(count, names) diff --git a/pyre/bench/synth/exception_inline_callee_tb_frame_locals.wasm.jitstats b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.wasm.jitstats new file mode 100644 index 00000000000..4665b398e2f --- /dev/null +++ b/pyre/bench/synth/exception_inline_callee_tb_frame_locals.wasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=1 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=1 diff --git a/pyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstats b/pyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstats index b4677131a35..a7184f7b1ba 100644 --- a/pyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstats +++ b/pyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=4 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=975 +guard_failures=1562 internal_compile_panics=0 loops_aborted=0 loops_compiled=6 diff --git a/pyre/bench/synth/exception_oserror_fields.cranelift.jitstats b/pyre/bench/synth/exception_oserror_fields.cranelift.jitstats index 7f27db25dfa..d6f5976a85d 100644 --- a/pyre/bench/synth/exception_oserror_fields.cranelift.jitstats +++ b/pyre/bench/synth/exception_oserror_fields.cranelift.jitstats @@ -2,7 +2,7 @@ bridges_compiled=1 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=201 +guard_failures=202 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/exception_oserror_fields.dynasm.jitstats b/pyre/bench/synth/exception_oserror_fields.dynasm.jitstats index 7f27db25dfa..d6f5976a85d 100644 --- a/pyre/bench/synth/exception_oserror_fields.dynasm.jitstats +++ b/pyre/bench/synth/exception_oserror_fields.dynasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=1 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=201 +guard_failures=202 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/exception_oserror_fields.wasm.jitstats b/pyre/bench/synth/exception_oserror_fields.wasm.jitstats index 7f27db25dfa..d6f5976a85d 100644 --- a/pyre/bench/synth/exception_oserror_fields.wasm.jitstats +++ b/pyre/bench/synth/exception_oserror_fields.wasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=1 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=201 +guard_failures=202 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/exception_reused_object_tb_not_doubled.cranelift.jitstats b/pyre/bench/synth/exception_reused_object_tb_not_doubled.cranelift.jitstats new file mode 100644 index 00000000000..e0432826d36 --- /dev/null +++ b/pyre/bench/synth/exception_reused_object_tb_not_doubled.cranelift.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=3 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=600 +internal_compile_panics=0 +loops_aborted=3 +loops_compiled=4 diff --git a/pyre/bench/synth/exception_reused_object_tb_not_doubled.dynasm.jitstats b/pyre/bench/synth/exception_reused_object_tb_not_doubled.dynasm.jitstats new file mode 100644 index 00000000000..e0432826d36 --- /dev/null +++ b/pyre/bench/synth/exception_reused_object_tb_not_doubled.dynasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=3 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=600 +internal_compile_panics=0 +loops_aborted=3 +loops_compiled=4 diff --git a/pyre/bench/synth/exception_reused_object_tb_not_doubled.py b/pyre/bench/synth/exception_reused_object_tb_not_doubled.py new file mode 100644 index 00000000000..1800bbe2ed8 --- /dev/null +++ b/pyre/bench/synth/exception_reused_object_tb_not_doubled.py @@ -0,0 +1,56 @@ +# One exception object raised over and over, caught in a hot loop whose handler +# forces the catching frame through the traceback (`tb_frame.f_locals`). The +# force aborts the recording walk, and a walk that does not commit must undo the +# traceback node it already attached to the live exception — otherwise the +# interpreter's replay records the same frames a second time and the reused +# object hands out a chain of four nodes instead of two. A freshly raised +# exception hides this: the discarded walk's object is unreachable, so only a +# reused one accumulates both deliveries. +# +# N is bounded rather than sized for throughput. The doubling reproduces from +# roughly the 1200th iteration on both native backends, while cranelift takes a +# GC BUG (`minor_varsize_item_target`) on this shape somewhere between 4000 and +# 8000 — a pre-existing crash this workload happens to reach, not something the +# traceback chain is party to. +# +# No `max-pypy-ratio`: this is a traceback-shape oracle, and its pypy arm runs +# well under a tenth of a second — below the point where a wall-clock ratio +# measures the workload rather than process startup. +# +# Expected output: [(('drive', ('e', 'k', 'seen')), 'mid')] +N = 4000 +ERR = ValueError("boom") + + +def mid(i): + raise ERR + + +def shape(tb): + out = [] + idx = 0 + while tb is not None: + f = tb.tb_frame + if idx == 0: + out.append((f.f_code.co_name, tuple(sorted(f.f_locals)))) + else: + out.append(f.f_code.co_name) + tb = tb.tb_next + idx += 1 + return tuple(out) + + +def drive(): + seen = set() + k = 0 + while k < N: + try: + mid(k) + except ValueError as e: + seen.add(shape(e.__traceback__)) + e.__traceback__ = None + k += 1 + return sorted(seen) + + +print(drive()) diff --git a/pyre/bench/synth/exception_reused_object_tb_not_doubled.wasm.jitstats b/pyre/bench/synth/exception_reused_object_tb_not_doubled.wasm.jitstats new file mode 100644 index 00000000000..e0432826d36 --- /dev/null +++ b/pyre/bench/synth/exception_reused_object_tb_not_doubled.wasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=3 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=600 +internal_compile_panics=0 +loops_aborted=3 +loops_compiled=4 diff --git a/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats b/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats index 029c4d1b755..75d560fbfa0 100644 --- a/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats +++ b/pyre/bench/synth/exception_traceback_frame_lineno.wasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=814 +guard_failures=813 internal_compile_panics=0 loops_aborted=0 loops_compiled=17 diff --git a/pyre/bench/synth/exception_traceback_loop_forms.cranelift.jitstats b/pyre/bench/synth/exception_traceback_loop_forms.cranelift.jitstats index e9e71515370..23809a7ff53 100644 --- a/pyre/bench/synth/exception_traceback_loop_forms.cranelift.jitstats +++ b/pyre/bench/synth/exception_traceback_loop_forms.cranelift.jitstats @@ -2,7 +2,7 @@ bridges_compiled=4 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=810 +guard_failures=811 internal_compile_panics=0 loops_aborted=0 loops_compiled=8 diff --git a/pyre/bench/synth/exception_traceback_loop_forms.wasm.jitstats b/pyre/bench/synth/exception_traceback_loop_forms.wasm.jitstats index e9e71515370..23809a7ff53 100644 --- a/pyre/bench/synth/exception_traceback_loop_forms.wasm.jitstats +++ b/pyre/bench/synth/exception_traceback_loop_forms.wasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=4 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=810 +guard_failures=811 internal_compile_panics=0 loops_aborted=0 loops_compiled=8 diff --git a/pyre/bench/synth/exception_value_op_caught.cranelift.jitstats b/pyre/bench/synth/exception_value_op_caught.cranelift.jitstats index 4665b398e2f..9d7d0e11958 100644 --- a/pyre/bench/synth/exception_value_op_caught.cranelift.jitstats +++ b/pyre/bench/synth/exception_value_op_caught.cranelift.jitstats @@ -2,7 +2,7 @@ bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=1 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/exception_value_op_caught.dynasm.jitstats b/pyre/bench/synth/exception_value_op_caught.dynasm.jitstats index 4665b398e2f..9d7d0e11958 100644 --- a/pyre/bench/synth/exception_value_op_caught.dynasm.jitstats +++ b/pyre/bench/synth/exception_value_op_caught.dynasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=1 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/exception_value_op_caught.wasm.jitstats b/pyre/bench/synth/exception_value_op_caught.wasm.jitstats index 4665b398e2f..9d7d0e11958 100644 --- a/pyre/bench/synth/exception_value_op_caught.wasm.jitstats +++ b/pyre/bench/synth/exception_value_op_caught.wasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=1 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/exec_fresh_globals_delete_name.cranelift.jitstats b/pyre/bench/synth/exec_fresh_globals_delete_name.cranelift.jitstats new file mode 100644 index 00000000000..54c5079b494 --- /dev/null +++ b/pyre/bench/synth/exec_fresh_globals_delete_name.cranelift.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=4 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=1 diff --git a/pyre/bench/synth/exec_fresh_globals_delete_name.dynasm.jitstats b/pyre/bench/synth/exec_fresh_globals_delete_name.dynasm.jitstats new file mode 100644 index 00000000000..54c5079b494 --- /dev/null +++ b/pyre/bench/synth/exec_fresh_globals_delete_name.dynasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=4 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=1 diff --git a/pyre/bench/synth/exec_fresh_globals_delete_name.py b/pyre/bench/synth/exec_fresh_globals_delete_name.py new file mode 100644 index 00000000000..a3fd5b49275 --- /dev/null +++ b/pyre/bench/synth/exec_fresh_globals_delete_name.py @@ -0,0 +1,13 @@ +# pyre-check: max-pypy-ratio=20 +# A compiled module loop may be reused by exec() with a fresh globals/locals +# dict. Guard-failure resume must carry this frame's debugdata and w_globals, +# not the recording frame's mappings, or DELETE_NAME targets the stale locals. +src = "acc = 1\nfor r in range(400):\n pass\ndel acc\n" +code = compile(src, "", "exec") + +checksum = 0 +for _ in range(6): + exec(code, {"range": range}) + checksum += 1 + +print(checksum) diff --git a/pyre/bench/synth/exec_fresh_globals_delete_name.wasm.jitstats b/pyre/bench/synth/exec_fresh_globals_delete_name.wasm.jitstats new file mode 100644 index 00000000000..54c5079b494 --- /dev/null +++ b/pyre/bench/synth/exec_fresh_globals_delete_name.wasm.jitstats @@ -0,0 +1,8 @@ +bridges_compiled=0 +descr_set_absent=0 +descr_set_ambiguous=0 +descr_set_stale_absent=0 +guard_failures=4 +internal_compile_panics=0 +loops_aborted=0 +loops_compiled=1 diff --git a/pyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstats b/pyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstats index 6b27a430b35..89605bf928f 100644 --- a/pyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstats +++ b/pyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=9 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=2036 +guard_failures=2038 internal_compile_panics=0 loops_aborted=0 loops_compiled=4 diff --git a/pyre/bench/synth/type_name_setter.cranelift.jitstats b/pyre/bench/synth/type_name_setter.cranelift.jitstats index 9cf2e63b4a5..9d7d0e11958 100644 --- a/pyre/bench/synth/type_name_setter.cranelift.jitstats +++ b/pyre/bench/synth/type_name_setter.cranelift.jitstats @@ -2,7 +2,7 @@ bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=3 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/bench/synth/type_name_setter.dynasm.jitstats b/pyre/bench/synth/type_name_setter.dynasm.jitstats index 9cf2e63b4a5..9d7d0e11958 100644 --- a/pyre/bench/synth/type_name_setter.dynasm.jitstats +++ b/pyre/bench/synth/type_name_setter.dynasm.jitstats @@ -2,7 +2,7 @@ bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 -guard_failures=3 +guard_failures=2 internal_compile_panics=0 loops_aborted=0 loops_compiled=1 diff --git a/pyre/pyre-interpreter/src/eval.rs b/pyre/pyre-interpreter/src/eval.rs index 83116740e11..96465b955b6 100644 --- a/pyre/pyre-interpreter/src/eval.rs +++ b/pyre/pyre-interpreter/src/eval.rs @@ -762,6 +762,29 @@ unsafe fn chain_next_frame(f_backref: *mut PyFrame) -> *mut PyFrame { crate::executioncontext::vref_referent(f_backref) } +/// Forward the frame named by a `JitVirtualRef` stored in a frame-shaped slot. +/// +/// Frame backrefs and `executioncontext.rs:88-101` both allow such a slot to +/// hold a vref in place of a `*mut PyFrame`. Its leading word is the +/// `JIT_VIRTUAL_REF_VTABLE` magic, not a PyObject `ob_type`, so callers must +/// skip PyObject-shaped walks when this returns true. The vref is old-gen and +/// non-moving, but its `forced` frame may be young. +/// `virtualref.py:94-98 is_virtual_ref(gcref)` supplies the predicate. +#[inline] +unsafe fn forward_virtual_ref_forced( + value: *mut u8, + visitor: &mut dyn FnMut(&mut majit_ir::GcRef), +) -> bool { + unsafe { + if !majit_metainterp::virtualref::ptr_is_virtual_ref(value as *const u8) { + return false; + } + let vref = value as *mut majit_metainterp::virtualref::JitVirtualRef; + visitor(&mut *(&mut (*vref).forced as *mut *mut u8 as *mut majit_ir::GcRef)); + true + } +} + /// How much of `locals_cells_stack_w` is reachable state. /// /// `valuestackdepth` is an absolute index that starts at `stack_base()` @@ -952,10 +975,7 @@ pub unsafe fn walk_pyframe_roots_area( // `f_backref` needs no such hop — the visitor already left it // naming the live copy. let f_backref = *f_back_slot; - if majit_metainterp::virtualref::ptr_is_virtual_ref(f_backref as *const u8) { - let vref = f_backref as *mut majit_metainterp::virtualref::JitVirtualRef; - visitor(&mut *(&mut (*vref).forced as *mut *mut u8 as *mut majit_ir::GcRef)); - } + forward_virtual_ref_forced(f_backref as *mut u8, visitor); // pyframe.py:102 `self.pycode` — the running code object. // Visited as a root so a code object reachable only via @@ -1084,6 +1104,9 @@ pub unsafe fn walk_pyframe_roots_area( // AFTER the visitor so a relocated value is the live one. unsafe { let value = (*slot_ptr).0 as PyObjectRef; + if forward_virtual_ref_forced(value as *mut u8, visitor) { + continue; + } walk_raw_exception_roots(value, visitor); walk_raw_immortal_roots(value, visitor); } diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 97ec8778de4..3e798f50d25 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -3228,7 +3228,7 @@ static PYTRACEBACK_DESCR_GROUP: LazyLock = LazyLock::new(| PYTRACEBACK_W_CODE_OFFSET, PYTRACEBACK_W_NEXT_OFFSET, }; - build_object_descr_group_with_def_path( + let group = build_object_descr_group_with_def_path( PYTRACEBACK_OBJECT_SIZE, PYTRACEBACK_GC_TYPE_ID, &PYTRACEBACK_TYPE as *const _ as usize, @@ -3290,7 +3290,13 @@ static PYTRACEBACK_DESCR_GROUP: LazyLock = LazyLock::new(| ], "", "", - ) + ); + // `w_pytraceback_new` allocates traceback nodes non-moving because raw + // `*mut PyTraceback` readers and the exception `w_traceback` chain keep + // bare pointers. A nursery allocation would move the node at the next + // minor collection while those copies retain its old address. + group.size_descr.set_non_moving(true); + group }); pub fn pytraceback_size_descr() -> DescrRef { @@ -3605,6 +3611,36 @@ mod tests { ); } + #[test] + fn jit_emitted_raw_pointer_objects_are_non_moving() { + let traceback_descr = pytraceback_size_descr(); + let traceback_size = traceback_descr + .as_size_descr() + .expect("PyTraceback SizeDescr"); + assert!( + traceback_size.non_moving(), + "raw traceback pointers are not rewritten when a minor collection moves an object" + ); + + let instance_descr = w_object_object_size_descr(); + let instance_size = instance_descr + .as_size_descr() + .expect("W_ObjectObject SizeDescr"); + assert!( + instance_size.non_moving(), + "raw instance pointers can survive across allocation without being rooted" + ); + + let storage_descr = crate::state::mapdict_storage_gcarray_descr(); + let storage_array = storage_descr + .as_array_descr() + .expect("mapdict storage ArrayDescr"); + assert!( + storage_array.non_moving(), + "the mapdict custom tracer marks raw storage pointers but cannot rewrite them" + ); + } + #[test] fn pyframe_size_descr_clears_the_vable_token_slot() { let descr = pyframe_size_descr(); diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs index ea743d95fcf..5de1afc9f15 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs @@ -40,25 +40,13 @@ fn record_bridge_handler_entry_traceback( // interpreter's `handle_exception` could record a node from — hence the // runtime emit when the IR-virtual prepend declines. // - // The concrete leg of the record mutates the live exception, and a walk - // that is later discarded would leave that mutation behind while the - // metainterp's own delivery attaches the node again. Journal the attach so - // a rollback restores `exception.w_traceback` to the head recorded here. - let traceback_exception = match exc_concrete { - ConcreteValue::Ref(exception) => Some(exception), - _ => None, - }; - let traceback_before = - traceback_exception.and_then(crate::jitcode_dispatch::fbw_traceback_journal_head); + // The concrete leg of each record mutates the live exception; both + // recorders journal their own attach, so a walk that is later discarded + // does not leave the node behind for the metainterp's own delivery to + // record on top of. let emit_runtime = !record_prepend_application_traceback(wc, exc, exc_concrete, position); record_inline_application_traceback(wc, exc, exc_concrete, position, true, emit_runtime); record_top_level_application_traceback(wc, exc, exc_concrete, position, true, emit_runtime); - if let Some(exception) = traceback_exception { - crate::jitcode_dispatch::fbw_traceback_journal_push_if_attached( - exception, - traceback_before, - ); - } } /// `executioncontext.py:91-107 leave` for a frame the bridge resumed into diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs index c2c20b294e6..a82eaef92b4 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs @@ -1671,7 +1671,9 @@ pub(crate) fn fbw_terminate_with_finish( /// merely redundant: `gen_store_back_in_vable` sets `forced_virtualizable`, and /// `store_token_in_vable` returns early on that (pyjitpl.py) — the two are /// alternatives, not a sequence — and its final store zeroes the token slot. -fn fbw_force_virtualizable_before_return(ctx: &mut WalkContext<'_, '_, Sym>) { +pub(crate) fn fbw_force_virtualizable_before_return( + ctx: &mut WalkContext<'_, '_, Sym>, +) { let Some(vbox) = ctx.trace_ctx.standard_virtualizable_box() else { return; }; diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index 4ac8e6f99a5..ffaac0f5c9d 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -535,6 +535,23 @@ impl WalkSession { } } +/// Apply a walk-time concrete `record_application_traceback` with the undo log +/// armed. +/// +/// The authoritative walk attaches this iteration's node to the LIVE exception +/// while it records, so a walk that does not commit has left a node the +/// interpreter's replay then records again. On a freshly raised exception that +/// is invisible — the replay builds a new object and the discarded one is +/// unreachable — but a raise that re-uses one exception object accumulates both +/// deliveries, and the handler reads the whole chain twice over. +/// `fbw_store_journal_rollback` splices the journaled link back out; the commit +/// path keeps it, because a committed walk's node IS this iteration's. +fn journaled_concrete_traceback_attach(exc_ptr: pyre_object::PyObjectRef, attach: impl FnOnce()) { + let previous_head = crate::jitcode_dispatch::fbw_traceback_journal_head(exc_ptr); + attach(); + crate::jitcode_dispatch::fbw_traceback_journal_push_if_attached(exc_ptr, previous_head); +} + fn record_top_level_application_traceback( ctx: &mut WalkContext<'_, '_, Sym>, exc: OpRef, @@ -557,12 +574,14 @@ fn record_top_level_application_traceback( (session.recording_frame_ptr, session.recording_jitcode_index) }; if execute_concrete { - majit_metainterp::record_application_traceback_for_recording( - exc_ptr as usize as i64, - frame_ptr as i64, - jitcode_index, - opcode_position as i32, - ); + journaled_concrete_traceback_attach(exc_ptr, || { + majit_metainterp::record_application_traceback_for_recording( + exc_ptr as usize as i64, + frame_ptr as i64, + jitcode_index, + opcode_position as i32, + ); + }); } let hook = majit_metainterp::record_application_traceback_hook_address(); let frame = crate::state::pyjitcode_for_jitcode_index(jitcode_index) @@ -620,11 +639,13 @@ fn record_exc_edge_discarded_tracebacks( } let hook = majit_metainterp::record_discarded_level_traceback_hook_address(); for &(w_code, py_pc) in levels.iter().rev() { - majit_metainterp::record_discarded_level_traceback_for_recording( - exc_ptr as usize as i64, - w_code as i64, - py_pc as i64, - ); + journaled_concrete_traceback_attach(exc_ptr, || { + majit_metainterp::record_discarded_level_traceback_for_recording( + exc_ptr as usize as i64, + w_code as i64, + py_pc as i64, + ); + }); if hook.is_null() || exc.is_none() { continue; } @@ -704,7 +725,7 @@ fn record_inline_application_traceback( .and_then(|py_pc| { concrete_portal_frame(ctx, consts.jitcode_index).map(|frame| (frame, py_pc)) }); - match node_frame { + journaled_concrete_traceback_attach(exc_ptr, || match node_frame { Some((frame_ptr, py_pc)) => { // `dispatch_bytecode` (pyopcode.py) writes `self.last_instr` // before every opcode so a frame read while it runs answers for @@ -736,7 +757,7 @@ fn record_inline_application_traceback( consts.jitcode_index, opcode_position as i32, ), - } + }); } let hook = majit_metainterp::record_inline_application_traceback_hook_address(); if emit_runtime && !hook.is_null() && !exc.is_none() { @@ -921,6 +942,23 @@ fn traceback_node_site( .get(jitcode.metadata.portal_frame_reg as usize) .copied() .unwrap_or(OpRef::NONE); + // Storing this box into the node's `frame` field lets the traceback + // outlive the frame, so folded inline-callee locals have to reach the + // array first. A `STORE_FAST` on the callee's own fresh frame is folded + // to an SSA register and emits no array store; without replaying that fold, + // the escaping frame carries only its seeded parameters and every + // `tb_frame.f_locals` consumer loses the callee's non-parameter locals. + // Upstream needs no replay because `STORE_FAST` writes + // `locals_cells_stack_w` itself and virtualization removes the store while + // the frame stays virtual (`pypy/interpreter/pyframe.py`, + // `rpython/jit/metainterp/virtualizable.py`). Declining on `Err` leaves + // the node to the opaque fabricating hook, the same disposition both + // callers already take for an unresolved frame. + if !frame.is_none() + && residual_call::disarm_folded_inline_callee_after_escape(ctx, opcode_position).is_err() + { + return None; + } Some(TracebackNodeSite { frame, w_code, @@ -2881,6 +2919,30 @@ pub fn walk( // raise coordinate out of `frame.last_instr`. Compiled // code never wrote that field, so publish it here. fbw_publish_exit_last_instr(ctx, recording_opcode_position); + // `pyjitpl.py:3261 compile_exit_frame_with_exception` opens + // with `store_token_in_vable()`, exactly as + // `compile_done_with_this_frame` does — both frame exits + // settle the token, and this one did not. Every residual + // call arms it (`walker_vable_and_vrefs_before_residual_call` + // records FORCE_TOKEN + SETFIELD_GC through + // `token_field_descr`), so an exit that leaves it armed + // leaves the frame naming a jitframe the backend is about to + // free in `execute_token`, and the next + // `is_force_token_armed` walks `jf_forward` off freed + // memory. + // + // Settle it the way the value/void arms do — store back, + // which zeroes the token slot — rather than by arming it + // like upstream. Upstream can leave the token live because + // its FORCE_TOKEN is the heap-allocated GC `JITFRAME` + // (`jitframe.py` `lltype.malloc(JITFRAME, ...)`); pyre's is + // the machine frame pointer (`mov Rq(r), rbp` / `mov X(r), + // x29`), which stops being addressable the moment compiled + // code returns. `gen_store_back_in_vable` sets + // `forced_virtualizable`, which is the same early-out + // `store_token_in_vable` takes on `vbox is + // self.forced_virtualizable`. + fbw_force_virtualizable_before_return(ctx); // RPython parity: framestack exhausted with no handler // match → `compile_exit_frame_with_exception(last_exc_box)`. // Stash the exception the same way the value-return arms 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 cab0f82e881..a444d4dad67 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -3656,7 +3656,7 @@ fn maybe_record_inline_callee_last_instr( .heapcache_setfield_cached(callee_frame, last_instr_idx, last_instr); } -fn disarm_folded_inline_callee_after_escape( +pub(crate) fn disarm_folded_inline_callee_after_escape( ctx: &mut WalkContext<'_, '_, Sym>, pc: usize, ) -> Result<(), DispatchError> { diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs index 827448fa5dc..d2778bcfd89 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs @@ -3930,6 +3930,96 @@ fn step_through_raise_records_outermost_finish_and_terminates() { ); } +#[test] +fn top_level_raise_settles_the_vable_token() { + // `pyjitpl.py:3261 compile_exit_frame_with_exception` opens with + // `store_token_in_vable()`, the same as `compile_done_with_this_frame`. + // Every residual call arms the token + // (`walker_vable_and_vrefs_before_residual_call`), so an exception exit + // that settles nothing leaves the frame naming a jitframe the backend + // frees on the way out of `execute_token`. pyre settles by storing back + // — which zeroes the token — because its FORCE_TOKEN is the machine frame + // pointer, not upstream's heap-allocated GC `JITFRAME`. + let raise_byte = *insns_opname_to_byte() + .get("raise/r") + .expect("`raise/r` must be in insns table"); + let code = [raise_byte, 0x02]; + let mut tc = fresh_trace_ctx(); + let mut vable_buf = vec![0u8; 65536]; + bind_fake_vable(&mut tc, &mut vable_buf); + let mut regs = distinct_const_refs(&mut tc, 4); + let ops_before = tc.num_ops(); + let session = std::cell::RefCell::new(WalkSession::default()); + let mut wc = WalkContext { + callee_shadow: None, + inline_callee_consts: None, + fbw_mode: test_fbw_mode(), + session: &session, + registers_r: &mut regs, + registers_i: &mut [], + registers_f: &mut [], + concrete_registers_r: &mut [], + concrete_registers_i: &mut [], + descr_refs: &[], + raw_descrs: RawDescrPool::Global, + is_authoritative_executor: false, + trace_ctx: &mut tc, + is_top_level: true, + sub_jitcode_lookup: &no_sub_jitcodes, + last_exc_value: None, + last_exc_value_concrete: ConcreteValue::Null, + entry_py_pc: EntryPyPc::Py(0), + outer_resume_marker_jit_pc: None, + outer_jitcode_index: 0, + outer_active_boxes: Vec::new(), + store_subscr_fn_addr: None, + pending_guard_snapshot_error: None, + vstack_boxes: Vec::new(), + vstack_depth: 0, + vstack_cur_pypc: 0, + vstack_valid: false, + vstack_last_ref: OpRef::NONE, + vstack_reorder_ceiling: u32::MAX, + live_before_jit_pc: usize::MAX, + live_after_jit_pc: usize::MAX, + }; + fbw_finish_payload_reset(); + let (outcome, _next_pc) = walk(&code, 0, &mut wc).expect("raise/r must dispatch"); + assert_eq!(outcome, DispatchOutcome::Terminate); + drop(wc); + let _ = fbw_finish_payload_take(); + assert!( + tc.num_ops() > ops_before, + "the exception exit must record the virtualizable store-back", + ); + let last = tc.ops().last().expect("recorded op must exist").clone(); + assert_eq!( + last.opcode, + majit_ir::OpCode::SetfieldGc, + "the store-back's tail op is the token store", + ); + let info = crate::frame_layout::build_pyframe_virtualizable_info(); + let recorded_descr = last.getdescr().expect("token store carries a field descr"); + let field = recorded_descr + .as_field_descr() + .expect("token store's descr is a FieldDescr"); + assert_eq!( + field.offset(), + info.token_offset, + "the tail store must target the vable_token slot", + ); + let value = last + .getarglist() + .get(1) + .expect("SetfieldGc args are [obj, value]") + .to_opref(); + assert_eq!( + tc.box_value(value), + Some(majit_ir::Value::Int(0)), + "the exception exit must leave vable_token cleared, not armed", + ); +} + #[test] fn raise_r_emits_guard_class_when_concrete_exc_pinned_in_shadow() { // The concrete shadow is mutable and @@ -10528,9 +10618,12 @@ fn dispatch_via_miframe_mirrors_last_exc_value_back_into_sym() { // parity: `metainterp.last_exc_value = ...` is metainterp-level // state that survives across opimpl invocations. use crate::state::PyreSym; + use pyre_object::interp_exceptions::{ExcKind, w_exception_new}; let mut tc = TraceCtx::for_test_types(&[majit_ir::Type::Ref]); - let exc_oprep = tc.const_ref(0xDEAD_BEEF); + let exc = w_exception_new(ExcKind::ValueError, "boom"); + // The walk now reads the concrete exception's traceback head. + let exc_oprep = tc.const_ref(exc as i64); let mut sym = PyreSym::new_uninit(OpRef::NONE); *sym.registers_r_mut() = vec![OpRef::NONE; 8]; sym.registers_r_mut()[3] = exc_oprep; @@ -11643,3 +11736,73 @@ fn mayforce_null_ref_arg_exempts_the_unread_load_global_namespace() { Err(DispatchError::MayForceNullRefArgUnsupported { pc: 186 }), )); } + +#[test] +fn traceback_journal_rollback_unwinds_every_walk_node() { + // A recording walk attaches one node per frame it delivers the exception + // through, so by the time the walk ends the exception carries SEVERAL + // journaled nodes — the bridge handler entry that introduced the log only + // ever pushed one. A non-commit exit has to splice all of them back out, or + // the interpreter's replay records the same frames again and a raise that + // re-uses one exception object hands out a doubled chain. + use pyre_interpreter::pytraceback::{ + w_pytraceback_get_lasti, w_pytraceback_get_w_next, w_pytraceback_new, + }; + use pyre_object::interp_exceptions::{ + ExcKind, w_exception_get_traceback, w_exception_new, w_exception_set_traceback, + }; + + // `frame` is only ever compared by identity here, and `w_code` only has to + // be a chain-terminating slot, so both stay NULL — the splice walks + // `w_next` and nothing dereferences either. + fn prepend(exc: pyre_object::PyObjectRef, lasti: i64) { + unsafe { + let head = w_exception_get_traceback(exc); + let node = + w_pytraceback_new(std::ptr::null_mut(), lasti, head, 1, pyre_object::PY_NULL); + w_exception_set_traceback(exc, node); + } + } + + fn chain(exc: pyre_object::PyObjectRef) -> Vec { + let mut out = Vec::new(); + let mut node = unsafe { w_exception_get_traceback(exc) }; + while !node.is_null() { + out.push(unsafe { w_pytraceback_get_lasti(node) }); + node = unsafe { w_pytraceback_get_w_next(node) }; + } + out + } + + super::fbw_store_journal_reset(); + let exc = w_exception_new(ExcKind::ValueError, "boom"); + // The node the raise itself already attached before the walk got here. + prepend(exc, 1); + assert_eq!(chain(exc), vec![1]); + + // Callee level first, catching level last — the order the walk records in, + // and the reverse of the order the rollback has to undo. + for lasti in [2i64, 3] { + super::journaled_concrete_traceback_attach(exc, || prepend(exc, lasti)); + } + assert_eq!(chain(exc), vec![3, 2, 1]); + + super::fbw_store_journal_rollback(); + assert_eq!( + chain(exc), + vec![1], + "a non-commit walk must unwind EVERY node it attached, not just the last" + ); + + // Commit keeps them: a committed walk's nodes ARE this delivery's. + for lasti in [4i64, 5] { + super::journaled_concrete_traceback_attach(exc, || prepend(exc, lasti)); + } + super::fbw_store_journal_commit(); + super::fbw_store_journal_rollback(); + assert_eq!( + chain(exc), + vec![5, 4, 1], + "a committed walk keeps its nodes" + ); +} diff --git a/pyre/pyre-jit-trace/src/trace_opcode.rs b/pyre/pyre-jit-trace/src/trace_opcode.rs index 40387998d14..47f8b8dcc80 100644 --- a/pyre/pyre-jit-trace/src/trace_opcode.rs +++ b/pyre/pyre-jit-trace/src/trace_opcode.rs @@ -1777,12 +1777,20 @@ impl MIFrame { let vsd = self .concrete_valuestackdepth() .unwrap_or_else(|| self.sym().valuestackdepth) as i64; - // virtualizable.py:86-93 read_boxes: ALL static fields from the heap. + // `debugdata` keeps the box `read_boxes` seeded instead of being + // re-published as a recording-time ConstPtr. DELETE_NAME resolves the + // name through `debugdata.w_locals`, so a constant pins the recording + // frame's mapping and the compiled loop then deletes through it once + // `exec(code, globals)` reuses the same code object with a fresh + // namespace. virtualizable.py:86-93 `read_boxes` carries every static + // as a loop-carried box for exactly this reason; the remaining pointer + // statics keep their promoted-constant representation because the + // bytecode path cannot rebind them mid-loop. let last_instr_value = resume_pc as i64 - 1; let last_instr_op = ctx.const_int(last_instr_value); let pycode_op = ctx.const_ref(code_ptr as i64); let vsd_op = ctx.const_int(vsd); - let debugdata_op = ctx.const_ref(debugdata as i64); + let debugdata_op = self.sym().vable_debugdata; let lastblock_op = ctx.const_ref(lastblock as i64); let w_globals_op = ctx.const_ref(ns_ptr); let owns = { @@ -1790,13 +1798,12 @@ impl MIFrame { s.vable_last_instr = last_instr_op; s.vable_pycode = pycode_op; s.vable_valuestackdepth = vsd_op; - s.vable_debugdata = debugdata_op; s.vable_lastblock = lastblock_op; s.vable_w_globals = w_globals_op; s.owns_virtualizable_shadow() }; // pyjitpl.py:1188-1199 `_opimpl_setfield_vable` parity: - // mirror the heap-read seed into the canonical + // mirror the republished statics into the canonical // `metainterp.virtualizable_boxes` shadow so subsequent readers // (snapshot, JUMP-arg dedup) see the same identity that // `s.vable_*` carries. diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 86a9583e245..18a4e986f1f 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -4340,6 +4340,19 @@ unsafe extern "C" fn force_pyframe(frame: *mut pyre_interpreter::PyFrame) { // materialization never writes the slot, so the token read already // excludes it. // + // Dereferencing the token is only sound because every exit from a + // compiled activation leaves the slot cleared. Upstream does not need + // that invariant: its FORCE_TOKEN is the heap-allocated GC `JITFRAME` + // the deadframe retains, so a surviving token still names live memory. + // pyre's is the machine frame pointer and the backend frees the whole + // `jf_forward` chain inside `execute_token`, so the slot has to be + // cleared on the way out instead — `gen_store_back_in_vable` on both + // portal exits (`fbw_terminate_with_finish` and the raise arm), + // `sync_after` on the loop back-edge, and + // `sync_virtualizable_after_guard_failure` on guard failure. When that + // invariant breaks, the fault lands in `JitFrame::resolve` rather than + // here. + // // Comparing against `MetaInterp::vable_ptr` as well named the WRONG // frame. That cell is rewritten by every `sync_before`, including the // entry a callee's own compiled loop takes from inside the caller's