Skip to content

Commit 89192db

Browse files
committed
jit: publish the vable promote snapshots in emission order, and fix the four callers the enum broke
`capture_vable_promote_guard` walked `0..minted`, newest guard first. `publish_last_guard_resume_snapshot` is not side-effect free -- it leaves the root frame's in-flight result slot cleared (`root_inflight_int_result`) -- so the order decides what the earlier guard's snapshot sees. `generate_guard` (pyjitpl.py:2582-2603) captures each guard synchronously as it is emitted, so the loop runs oldest-first. The `bool -> VableArrayStore` return also broke four `assert!` call sites in `pyre-jit-trace`'s test module, which a release binary build and `cargo test -p majit-metainterp` both compile past; `cargo check --workspace --all-targets` is what reports them. `VableArrayStore` is re-exported from the crate root so the external test can name it. The doc comment claimed pyre's two snapshots match what upstream would produce. They do not: `MetaInterp.replace_box` walks the framestack (`frame.replace_active_box_in_frame`) and `TraceCtx::replace_box` does not, so upstream's second capture would see the standard box. Stated as the pre-existing gap it is. Assisted-by: Claude
1 parent a2fa306 commit 89192db

3 files changed

Lines changed: 54 additions & 39 deletions

File tree

majit/majit-metainterp/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ pub use trace_ctx::GreenBox;
171171
pub use trace_ctx::MergePoint;
172172
pub use trace_ctx::ReconstructRecipe;
173173
pub use trace_ctx::TraceCtx;
174+
pub use trace_ctx::VableArrayStore;
174175

175176
/// Compute green key from code pointer and PC.
176177
/// Must use the same hash as the front-end's make_green_key — the full

majit/majit-metainterp/src/pyjitpl/dispatch.rs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1487,10 +1487,14 @@ where
14871487
/// One call can emit TWO: a vable array access whose symbolic frame box
14881488
/// differs from the standard box but shares its pointer promotes the
14891489
/// `isstandard` PTR_EQ and then the index, so every guard the call added is
1490-
/// stamped, not just the last. Both take the same snapshot content, which
1491-
/// is what the two upstream `capture_resumedata` calls would produce here:
1492-
/// what runs between them is `replace_box`, and pyre's does not carry the
1493-
/// framestack walk (`TraceCtx::replace_box`).
1490+
/// stamped, not just the last. In pyre the two snapshots currently come
1491+
/// out identical because `TraceCtx::replace_box` updates the side tables
1492+
/// but not the live `MIFrame`s; this is a pre-existing gap, not parity:
1493+
/// upstream `MetaInterp.replace_box` walks the framestack via
1494+
/// `frame.replace_active_box_in_frame`, so its second capture sees the
1495+
/// standard box where the first saw the old one. The loop runs in emission
1496+
/// order because each capture leaves the root frame's in-flight result slot
1497+
/// cleared.
14941498
///
14951499
/// `write` is the shadow slot a `vable_set*` overwrote. Upstream reaches
14961500
/// `virtualizable_boxes[index] = valuebox` only after both promotes have
@@ -1524,7 +1528,7 @@ where
15241528
ctx.swap_virtualizable_entry(w.index, w.prev_box, w.prev_value)
15251529
.map(|current| (w.index, current))
15261530
});
1527-
for from_end in 0..minted {
1531+
for from_end in (0..minted).rev() {
15281532
self.publish_last_guard_resume_snapshot(
15291533
ctx,
15301534
sym,

pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs

Lines changed: 44 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use super::*;
22
use crate::jitcode_runtime::{insns_opname_to_byte, named_jitcode};
33
use majit_ir::Type;
4-
use majit_metainterp::make_fail_descr;
4+
use majit_metainterp::{VableArrayStore, make_fail_descr};
55

66
#[test]
77
fn propagated_subwalk_abort_cannot_rebind_its_pc_to_a_caller_frame() {
@@ -570,16 +570,19 @@ fn vable_store_tracks_live_null_without_changing_the_recorded_trace() {
570570
let index0 = tc.const_int(0);
571571
let const_null = tc.const_null();
572572
let ops_before = tc.num_ops();
573-
assert!(tc.vable_setarrayitem_indexed(
574-
0,
575-
vable,
576-
index0,
577-
0,
578-
fdescr.clone(),
579-
adescr.clone(),
580-
const_null,
581-
null,
582-
true,
573+
assert!(matches!(
574+
tc.vable_setarrayitem_indexed(
575+
0,
576+
vable,
577+
index0,
578+
0,
579+
fdescr.clone(),
580+
adescr.clone(),
581+
const_null,
582+
null,
583+
true,
584+
),
585+
VableArrayStore::Stored(_)
583586
));
584587
assert!(tc.virtualizable_slot_stored_live_null(flat_base));
585588
assert!(
@@ -598,37 +601,44 @@ fn vable_store_tracks_live_null_without_changing_the_recorded_trace() {
598601
"the side-table marker records no op"
599602
);
600603

601-
assert!(tc.vable_setarrayitem_indexed(
602-
0,
603-
vable,
604-
index0,
605-
0,
606-
fdescr.clone(),
607-
adescr.clone(),
608-
const_null,
609-
null,
610-
false,
604+
assert!(matches!(
605+
tc.vable_setarrayitem_indexed(
606+
0,
607+
vable,
608+
index0,
609+
0,
610+
fdescr.clone(),
611+
adescr.clone(),
612+
const_null,
613+
null,
614+
false,
615+
),
616+
VableArrayStore::Stored(_)
611617
));
612618
assert!(!tc.virtualizable_slot_stored_live_null(flat_base));
613619

614620
let index1 = tc.const_int(1);
615621
let non_null = tc.const_ref(2);
616-
assert!(tc.vable_setarrayitem_indexed(
617-
0,
618-
vable,
619-
index1,
620-
1,
621-
fdescr.clone(),
622-
adescr.clone(),
623-
non_null,
624-
Value::Ref(majit_ir::GcRef(2)),
625-
true,
622+
assert!(matches!(
623+
tc.vable_setarrayitem_indexed(
624+
0,
625+
vable,
626+
index1,
627+
1,
628+
fdescr.clone(),
629+
adescr.clone(),
630+
non_null,
631+
Value::Ref(majit_ir::GcRef(2)),
632+
true,
633+
),
634+
VableArrayStore::Stored(_)
626635
));
627636
assert!(!tc.virtualizable_slot_stored_live_null(flat_base + 1));
628637

629-
assert!(
630-
tc.vable_setarrayitem_indexed(0, vable, index0, 0, fdescr, adescr, const_null, null, true,)
631-
);
638+
assert!(matches!(
639+
tc.vable_setarrayitem_indexed(0, vable, index0, 0, fdescr, adescr, const_null, null, true,),
640+
VableArrayStore::Stored(_)
641+
));
632642
assert!(tc.virtualizable_slot_stored_live_null(flat_base));
633643
tc.set_virtualizable_entry_at(flat_base, const_null, null);
634644
assert!(!tc.virtualizable_slot_stored_live_null(flat_base));

0 commit comments

Comments
 (0)