diff --git a/majit/majit-backend-wasm/src/codegen.rs b/majit/majit-backend-wasm/src/codegen.rs index 28c041f7bfc..5153906a494 100644 --- a/majit/majit-backend-wasm/src/codegen.rs +++ b/majit/majit-backend-wasm/src/codegen.rs @@ -2285,6 +2285,38 @@ pub fn merged_stream_has_loop_label(inputs: &ModuleBuildInputs) -> bool { find_loop_label_index(&ops).is_some_and(|label_idx| label_idx < inputs.ops.len()) } +/// Whether the guard a region would attach to sits in the owner's peeled +/// preamble, ahead of the loop header LABEL. +/// +/// `InlineGuard::branch_depth` is a depth at loop-body statement level, where +/// the per-region blocks are the innermost ones open. The preamble has not +/// entered the `loop` those blocks are opened in; its innermost blocks are the +/// LABEL resume pairs, so the same depth names a resume loader and the region +/// body stays unreachable. Such a bridge must keep the out-of-line path. +/// +/// `fail_index` is the exit ordinal within the owner's own stream — the +/// numbering `collect_guards_and_vars` assigns and `InlinedBridge` +/// records as `source_fail_index`. +pub fn inline_source_guard_precedes_loop_label( + inputs: &ModuleBuildInputs, + fail_index: u32, +) -> bool { + let Some(label_idx) = find_loop_label_index(&inputs.ops) else { + return false; + }; + let mut exit_ordinal = 0u32; + for (pos, op) in inputs.ops.iter().enumerate() { + if !op.opcode.is_guard() && op.opcode != OpCode::Finish { + continue; + } + if exit_ordinal == fail_index { + return pos < label_idx; + } + exit_ordinal += 1; + } + false +} + impl Clone for InlinedBridge { fn clone(&self) -> Self { Self { diff --git a/majit/majit-backend-wasm/src/lib.rs b/majit/majit-backend-wasm/src/lib.rs index 1de53563056..20c2210d2e3 100644 --- a/majit/majit-backend-wasm/src/lib.rs +++ b/majit/majit-backend-wasm/src/lib.rs @@ -87,8 +87,10 @@ use std::sync::{Arc, Mutex}; /// the region carries a CALL_ASSEMBLER the owner build emits no arm for; 50 = /// the owner is already invalidated, so a merged region would inherit its set /// flag instead of starting valid; 51 = the region's closing JUMP names a LABEL -/// published by another module, which no in-module `br` can reach. -pub static BRIDGE_DIAG: [AtomicU64; 52] = [const { AtomicU64::new(0) }; 52]; +/// published by another module, which no in-module `br` can reach; 52 = the +/// region's source guard is in the peeled preamble, outside the `loop` its +/// block is opened in. +pub static BRIDGE_DIAG: [AtomicU64; 53] = [const { AtomicU64::new(0) }; 53]; #[repr(u8)] #[derive(Clone, Copy)] @@ -3432,9 +3434,9 @@ impl majit_backend::Backend for WasmBackend { decline("foreign_label"); } else if !resumes_at_loop_header && !inline_nonheader_enabled() { // Resuming at the header lets the region `br` straight to the - // `loop`. Resuming at an earlier LABEL needs the - // `loop`-wrapped dispatch, which is opt-in until its - // miscompile is root-caused (`inline_nonheader_enable`). + // `loop`. Resuming at an earlier LABEL goes through the + // `loop`-wrapped dispatch, still opt-in + // (`inline_nonheader_enable`) while its cost is measured. diag_bump(38); decline("not_header"); } else if let Some(mut candidate) = original_token @@ -3453,6 +3455,16 @@ impl majit_backend::Backend for WasmBackend { } else if !codegen::merged_stream_has_loop_label(&candidate) { diag_bump(39); decline("no_loop_label"); + } else if codegen::inline_source_guard_precedes_loop_label( + &candidate, + source_fail_index, + ) { + // The guard is in the peeled preamble, which the `loop` + // holding the region blocks has not been entered from, so + // its branch would land in a LABEL resume loader and the + // region body would be unreachable. + diag_bump(52); + decline("source_in_preamble"); } else { self.collect_constants_from_ops(ops); candidate.inlined_bridges.push(codegen::InlinedBridge { diff --git a/majit/majit-backend-wasm/tests/codegen_test.rs b/majit/majit-backend-wasm/tests/codegen_test.rs index 6dbcca9cc66..5db613e31c4 100644 --- a/majit/majit-backend-wasm/tests/codegen_test.rs +++ b/majit/majit-backend-wasm/tests/codegen_test.rs @@ -1280,26 +1280,19 @@ fn test_empty_trace() { assert!(guards[0].is_finish); } -#[test] -fn inlined_bridge_without_owner_loop_label_declines() { - let inputargs = vec![InputArg::from_type(Type::Int, 0)]; - let guard = make_guard( - OpCode::GuardTrue, - &[OpRef::input_arg_int(0)], - &[OpRef::input_arg_int(0)], - ); - let finish = Op::new(OpCode::Finish, &[rb(OpRef::input_arg_int(0))]); - let inputs = codegen::ModuleBuildInputs { +/// The `ModuleBuildInputs` shape every inline-region test shares: a fixed +/// frame, no nursery, no census, and every base at zero. Only the owner trace +/// and the regions merged into it vary between them, so a new field lands here +/// once instead of at each call site. +fn inline_region_inputs( + inputargs: &[InputArg], + ops: Vec, + inlined_bridges: Vec, +) -> codegen::ModuleBuildInputs { + codegen::ModuleBuildInputs { inputargs: inputargs.iter().map(InputArg::fresh_value_copy).collect(), - ops: vec![guard, finish], - inlined_bridges: vec![codegen::InlinedBridge { - source_fail_index: 0, - trace_id: 1, - inputargs: vec![InputArg::from_type(Type::Int, 1)], - ops: vec![Op::new(OpCode::Finish, &[])], - gc_table_base: 0, - constants: indexmap::IndexMap::new(), - }], + ops, + inlined_bridges, constants: indexmap::IndexMap::new(), vtable_offset: Some(0), classptr_to_typeid: HashMap::new(), @@ -1318,7 +1311,30 @@ fn inlined_bridge_without_owner_loop_label_declines() { external_jump_key: 0, frame: codegen::FrameGeometry::fixed(), ca: codegen::CaParams::default(), - }; + } +} + +#[test] +fn inlined_bridge_without_owner_loop_label_declines() { + let inputargs = vec![InputArg::from_type(Type::Int, 0)]; + let guard = make_guard( + OpCode::GuardTrue, + &[OpRef::input_arg_int(0)], + &[OpRef::input_arg_int(0)], + ); + let finish = Op::new(OpCode::Finish, &[rb(OpRef::input_arg_int(0))]); + let inputs = inline_region_inputs( + &inputargs, + vec![guard, finish], + vec![codegen::InlinedBridge { + source_fail_index: 0, + trace_id: 1, + inputargs: vec![InputArg::from_type(Type::Int, 1)], + ops: vec![Op::new(OpCode::Finish, &[])], + gc_table_base: 0, + constants: indexmap::IndexMap::new(), + }], + ); let error = match codegen::build_wasm_module(&inputs) { Ok(_) => panic!("a label-less owner cannot accept an inlined bridge"), @@ -3008,10 +3024,10 @@ fn build_owner_with_region_closing_at( ]; let inputargs = vec![InputArg::from_type(Type::Int, 0)]; - let inputs = codegen::ModuleBuildInputs { - inputargs: inputargs.iter().map(InputArg::fresh_value_copy).collect(), + let inputs = inline_region_inputs( + &inputargs, ops, - inlined_bridges: vec![codegen::InlinedBridge { + vec![codegen::InlinedBridge { source_fail_index: 0, trace_id: 1, inputargs: vec![InputArg::from_type(Type::Int, 10)], @@ -3019,25 +3035,7 @@ fn build_owner_with_region_closing_at( gc_table_base: 0, constants: indexmap::IndexMap::new(), }], - constants: indexmap::IndexMap::new(), - vtable_offset: Some(0), - classptr_to_typeid: HashMap::new(), - guard_gc_type_info: codegen::GuardGcTypeInfo::default(), - alloc: codegen::AllocHelpers::default(), - wb_fn_ptr: 0, - nursery: None, - invalidated_flag_addr: 0, - gc_table_base: 0, - fail_index_base: 0, - bridge_cells_base: 0, - bridge_entry_arity: None, - bridge_param_dispatch: false, - trace_entry_census: None, - external_jump_slot: 0, - external_jump_key: 0, - frame: codegen::FrameGeometry::fixed(), - ca: codegen::CaParams::default(), - }; + ); codegen::build_wasm_module(&inputs).map(|(bytes, _, _)| bytes) } @@ -3200,10 +3198,10 @@ fn run_non_header_region_repro(with_ref: bool) -> (i64, i64, i64) { if with_ref { inputargs.push(InputArg::from_type(Type::Ref, 6)); } - let inputs = codegen::ModuleBuildInputs { - inputargs: inputargs.iter().map(InputArg::fresh_value_copy).collect(), + let inputs = inline_region_inputs( + &inputargs, ops, - inlined_bridges: vec![codegen::InlinedBridge { + vec![codegen::InlinedBridge { source_fail_index: 0, trace_id: 1, inputargs: if with_ref { @@ -3218,25 +3216,7 @@ fn run_non_header_region_repro(with_ref: bool) -> (i64, i64, i64) { gc_table_base: 0, constants: indexmap::IndexMap::new(), }], - constants: indexmap::IndexMap::new(), - vtable_offset: Some(0), - classptr_to_typeid: HashMap::new(), - guard_gc_type_info: codegen::GuardGcTypeInfo::default(), - alloc: codegen::AllocHelpers::default(), - wb_fn_ptr: 0, - nursery: None, - invalidated_flag_addr: 0, - gc_table_base: 0, - fail_index_base: 0, - bridge_cells_base: 0, - bridge_entry_arity: None, - bridge_param_dispatch: false, - trace_entry_census: None, - external_jump_slot: 0, - external_jump_key: 0, - frame: codegen::FrameGeometry::fixed(), - ca: codegen::CaParams::default(), - }; + ); let (bytes, _, _) = codegen::build_wasm_module(&inputs).expect("non-header region merges"); validate_wasm(&bytes); @@ -3583,3 +3563,355 @@ fn a_bridge_compiled_after_the_owner_was_invalidated_starts_valid() { "a bridge compiled after invalidate_loop starts valid" ); } + +/// The same two-label shape, but with a backend-only live-in captured at the +/// label the region resumes at: `v7` is produced before LABEL0, is not one of +/// its args, and is read in the loop body. The region's back edge must restore +/// it from its capture slot exactly as the entry `br_table`'s resume loader +/// does, so the body's `v3 + v7` reports 5005 + 100. +#[test] +fn region_closing_at_a_non_header_label_restores_that_labels_captures() { + assert_eq!(run_non_header_capture_repro(), (5005, 5004, 5105)); +} + +fn run_non_header_capture_repro() -> (i64, i64, i64) { + let descr0 = majit_ir::make_loop_target_descr(30, false); + let descr1 = majit_ir::make_loop_target_descr(31, false); + + let label0 = Op::new(OpCode::Label, &[rb(OpRef::int_op(1))]); + label0.setdescr(descr0.clone()); + let label1 = Op::new(OpCode::Label, &[rb(OpRef::int_op(2))]); + label1.setdescr(descr1.clone()); + let jump = Op::new(OpCode::Jump, &[rb(OpRef::int_op(3))]); + jump.setdescr(descr1); + + let ops = vec![ + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(0), OpRef::const_int(1)], + OpRef::int_op(1), + ), + // Produced before LABEL0, not one of its args, read after the header: + // the capture plan must hold it across both resume paths. + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(0), OpRef::const_int(100)], + OpRef::int_op(7), + ), + label0, + make_op( + OpCode::IntAdd, + &[OpRef::int_op(1), OpRef::const_int(1)], + OpRef::int_op(2), + ), + label1, + make_op( + OpCode::IntAdd, + &[OpRef::int_op(2), OpRef::const_int(1)], + OpRef::int_op(3), + ), + make_op( + OpCode::IntAdd, + &[OpRef::int_op(3), OpRef::int_op(7)], + OpRef::int_op(8), + ), + make_op( + OpCode::IntGt, + &[OpRef::int_op(3), OpRef::const_int(10)], + OpRef::int_op(4), + ), + make_guard(OpCode::GuardTrue, &[OpRef::int_op(4)], &[OpRef::int_op(3)]), + make_op( + OpCode::IntLt, + &[OpRef::int_op(3), OpRef::const_int(1000)], + OpRef::int_op(5), + ), + make_guard( + OpCode::GuardTrue, + &[OpRef::int_op(5)], + &[OpRef::int_op(3), OpRef::int_op(2), OpRef::int_op(8)], + ), + jump, + ]; + assert_eq!(codegen::resumable_label_count(&ops), 2); + + let region_jump = Op::new(OpCode::Jump, &[rb(OpRef::int_op(11))]); + region_jump.setdescr(descr0); + let region_ops = vec![ + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(10), OpRef::const_int(5000)], + OpRef::int_op(11), + ), + region_jump, + ]; + + let inputargs = vec![InputArg::from_type(Type::Int, 0)]; + let inputs = inline_region_inputs( + &inputargs, + ops, + vec![codegen::InlinedBridge { + source_fail_index: 0, + trace_id: 1, + inputargs: vec![InputArg::from_type(Type::Int, 10)], + ops: region_ops, + gc_table_base: 0, + constants: indexmap::IndexMap::new(), + }], + ); + let (bytes, _, _) = codegen::build_wasm_module(&inputs).expect("non-header region merges"); + validate_wasm(&bytes); + + let engine = Engine::default(); + let module = Module::new(&engine, &bytes).expect("generated trace should compile"); + let mut store = Store::new(&engine, ()); + let memory = + Memory::new(&mut store, MemoryType::new(2, None)).expect("test memory should allocate"); + memory + .write( + &mut store, + codegen::FRAME_SLOT_BASE as usize, + &0i64.to_le_bytes(), + ) + .unwrap(); + let mut linker = Linker::new(&engine); + linker.define("env", "memory", memory).unwrap(); + let instance = linker + .instantiate_and_start(&mut store, &module) + .expect("generated trace should instantiate"); + instance + .get_typed_func::(&store, "trace") + .unwrap() + .call(&mut store, 0) + .expect("generated trace should execute"); + + let read = |off: u64| { + let mut buf = [0u8; 8]; + memory.read(&store, off as usize, &mut buf).unwrap(); + i64::from_le_bytes(buf) + }; + assert_eq!(read(0), 1, "the second guard is the one that exits"); + ( + read(codegen::FRAME_SLOT_BASE), + read(codegen::FRAME_SLOT_BASE + 8), + read(codegen::FRAME_SLOT_BASE + 16), + ) +} + +/// Two regions attached to the SAME owner, both closing at the NON-header +/// LABEL. Each region owns one of the blocks opened at the loop header, so the +/// guard that reaches it and the back edge it takes must both name that +/// region's own depth — region 0 innermost. +/// +/// preamble v1 = v0 + 1 +/// LABEL0 [v1] +/// segment v2 = v1 + 1 +/// LABEL1 [v2] <- header, the `loop` +/// body v3 = v2 + 1 +/// guard v3 > 10 <- fail 0, region A +/// guard v3 > 6000 <- fail 1, region B +/// guard v3 < 100000 <- fail 2, exits with [v3, v2, v1] +/// JUMP -> LABEL1 [v3] +/// region A v11 = v10 + 5000; JUMP -> LABEL0 [v11] +/// region B v21 = v20 + 50000; JUMP -> LABEL0 [v21] +#[test] +fn two_regions_closing_at_a_non_header_label_each_reenter_at_their_own_depth() { + assert_eq!(run_two_non_header_regions_repro(), (100000, 99999, 55005)); +} + +fn run_two_non_header_regions_repro() -> (i64, i64, i64) { + let descr0 = majit_ir::make_loop_target_descr(40, false); + let descr1 = majit_ir::make_loop_target_descr(41, false); + + let label0 = Op::new(OpCode::Label, &[rb(OpRef::int_op(1))]); + label0.setdescr(descr0.clone()); + let label1 = Op::new(OpCode::Label, &[rb(OpRef::int_op(2))]); + label1.setdescr(descr1.clone()); + let jump = Op::new(OpCode::Jump, &[rb(OpRef::int_op(3))]); + jump.setdescr(descr1); + + let ops = vec![ + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(0), OpRef::const_int(1)], + OpRef::int_op(1), + ), + label0, + make_op( + OpCode::IntAdd, + &[OpRef::int_op(1), OpRef::const_int(1)], + OpRef::int_op(2), + ), + label1, + make_op( + OpCode::IntAdd, + &[OpRef::int_op(2), OpRef::const_int(1)], + OpRef::int_op(3), + ), + make_op( + OpCode::IntGt, + &[OpRef::int_op(3), OpRef::const_int(10)], + OpRef::int_op(4), + ), + make_guard(OpCode::GuardTrue, &[OpRef::int_op(4)], &[OpRef::int_op(3)]), + make_op( + OpCode::IntGt, + &[OpRef::int_op(3), OpRef::const_int(6000)], + OpRef::int_op(5), + ), + make_guard(OpCode::GuardTrue, &[OpRef::int_op(5)], &[OpRef::int_op(3)]), + make_op( + OpCode::IntLt, + &[OpRef::int_op(3), OpRef::const_int(100000)], + OpRef::int_op(6), + ), + make_guard( + OpCode::GuardTrue, + &[OpRef::int_op(6)], + &[OpRef::int_op(3), OpRef::int_op(2), OpRef::int_op(1)], + ), + jump, + ]; + assert_eq!(codegen::resumable_label_count(&ops), 2); + + let region_a_jump = Op::new(OpCode::Jump, &[rb(OpRef::int_op(11))]); + region_a_jump.setdescr(descr0.clone()); + let region_a = vec![ + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(10), OpRef::const_int(5000)], + OpRef::int_op(11), + ), + region_a_jump, + ]; + let region_b_jump = Op::new(OpCode::Jump, &[rb(OpRef::int_op(21))]); + region_b_jump.setdescr(descr0); + let region_b = vec![ + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(20), OpRef::const_int(50000)], + OpRef::int_op(21), + ), + region_b_jump, + ]; + + let inputargs = vec![InputArg::from_type(Type::Int, 0)]; + let inputs = inline_region_inputs( + &inputargs, + ops, + vec![ + codegen::InlinedBridge { + source_fail_index: 0, + trace_id: 1, + inputargs: vec![InputArg::from_type(Type::Int, 10)], + ops: region_a, + gc_table_base: 0, + constants: indexmap::IndexMap::new(), + }, + codegen::InlinedBridge { + source_fail_index: 1, + trace_id: 2, + inputargs: vec![InputArg::from_type(Type::Int, 20)], + ops: region_b, + gc_table_base: 0, + constants: indexmap::IndexMap::new(), + }, + ], + ); + let (bytes, _, _) = codegen::build_wasm_module(&inputs).expect("two non-header regions merge"); + validate_wasm(&bytes); + + let engine = Engine::default(); + let module = Module::new(&engine, &bytes).expect("generated trace should compile"); + let mut store = Store::new(&engine, ()); + let memory = + Memory::new(&mut store, MemoryType::new(2, None)).expect("test memory should allocate"); + memory + .write( + &mut store, + codegen::FRAME_SLOT_BASE as usize, + &0i64.to_le_bytes(), + ) + .unwrap(); + let mut linker = Linker::new(&engine); + linker.define("env", "memory", memory).unwrap(); + let instance = linker + .instantiate_and_start(&mut store, &module) + .expect("generated trace should instantiate"); + instance + .get_typed_func::(&store, "trace") + .unwrap() + .call(&mut store, 0) + .expect("generated trace should execute"); + + let read = |off: u64| { + let mut buf = [0u8; 8]; + memory.read(&store, off as usize, &mut buf).unwrap(); + i64::from_le_bytes(buf) + }; + assert_eq!(read(0), 2, "the third guard is the one that exits"); + ( + read(codegen::FRAME_SLOT_BASE), + read(codegen::FRAME_SLOT_BASE + 8), + read(codegen::FRAME_SLOT_BASE + 16), + ) +} + +/// A region's guard must sit inside the `loop` whose blocks it branches to. +/// `InlineGuard::branch_depth` counts those blocks from loop-body statement +/// level; in the peeled preamble the same depth names a LABEL resume loader, +/// so a bridge sourced there has to keep the out-of-line path. +#[test] +fn a_preamble_guard_is_reported_as_unreachable_from_the_region_blocks() { + let descr0 = majit_ir::make_loop_target_descr(50, false); + let descr1 = majit_ir::make_loop_target_descr(51, false); + + let label0 = Op::new(OpCode::Label, &[rb(OpRef::int_op(1))]); + label0.setdescr(descr0); + let label1 = Op::new(OpCode::Label, &[rb(OpRef::int_op(2))]); + label1.setdescr(descr1.clone()); + let jump = Op::new(OpCode::Jump, &[rb(OpRef::int_op(3))]); + jump.setdescr(descr1); + + let ops = vec![ + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(0), OpRef::const_int(1)], + OpRef::int_op(1), + ), + label0, + make_op( + OpCode::IntLt, + &[OpRef::int_op(1), OpRef::const_int(50)], + OpRef::int_op(6), + ), + // exit 0: in the preamble, between the two LABELs. + make_guard(OpCode::GuardTrue, &[OpRef::int_op(6)], &[OpRef::int_op(1)]), + make_op( + OpCode::IntAdd, + &[OpRef::int_op(1), OpRef::const_int(1)], + OpRef::int_op(2), + ), + label1, + make_op( + OpCode::IntAdd, + &[OpRef::int_op(2), OpRef::const_int(1)], + OpRef::int_op(3), + ), + make_op( + OpCode::IntLt, + &[OpRef::int_op(3), OpRef::const_int(100)], + OpRef::int_op(4), + ), + // exit 1: in the loop body. + make_guard(OpCode::GuardTrue, &[OpRef::int_op(4)], &[OpRef::int_op(3)]), + jump, + ]; + + let inputargs = vec![InputArg::from_type(Type::Int, 0)]; + let inputs = inline_region_inputs(&inputargs, ops, vec![]); + assert!(codegen::inline_source_guard_precedes_loop_label(&inputs, 0)); + assert!(!codegen::inline_source_guard_precedes_loop_label( + &inputs, 1 + )); +} diff --git a/majit/majit-metainterp/src/optimizeopt/virtualstate.rs b/majit/majit-metainterp/src/optimizeopt/virtualstate.rs index a3c91129ba7..bf3cf43b70e 100644 --- a/majit/majit-metainterp/src/optimizeopt/virtualstate.rs +++ b/majit/majit-metainterp/src/optimizeopt/virtualstate.rs @@ -1365,8 +1365,14 @@ impl VirtualState { .get_box_replacement_operand_opt(rb) .and_then(|b| state.ctx.get_constant_box(&b)) }); + // The constant-folded reading above is not what the + // Constant arm decides on: that one calls `runtime_value_of`, + // which reads the box's OBSERVED value. Print both, or a + // refusal that had an observed value all along reads as a + // box with no value at all. + let observed = runtime_box.and_then(|rb| state.ctx.runtime_value_of(rb)); eprintln!( - "[jit][jte] virtualstate mismatch index={i} box={box_opref:?} runtime={runtime_box:?} runtime_value={runtime_value:?} expected={expected:?} incoming={incoming:?}" + "[jit][jte] virtualstate mismatch index={i} box={box_opref:?} runtime={runtime_box:?} runtime_value={runtime_value:?} observed={observed:?} expected={expected:?} incoming={incoming:?}" ); } return Err(VirtualStatesCantMatch::default()); diff --git a/pyre/extra_tests/snippets/stdlib_abc.py b/pyre/extra_tests/snippets/stdlib_abc.py index 30b4ef37068..9db3f0e07ea 100644 --- a/pyre/extra_tests/snippets/stdlib_abc.py +++ b/pyre/extra_tests/snippets/stdlib_abc.py @@ -1,3 +1,4 @@ +import _abc import abc from testutils import assert_raises @@ -36,3 +37,122 @@ class SubConcrete(Concrete): assert isinstance(Concrete(), CustomInterface) assert isinstance(SubConcrete(), CustomInterface) assert not isinstance((), CustomInterface) + + +# `__abc_tpflags__` in a class body is consumed by `_abc_init`, not by class +# creation: only a class going through `ABCMeta` may take the structural-match +# marker, or `case [...]` would start accepting a plain object. +def matches_sequence_pattern(value): + match value: + case [*_]: + return True + case _: + return False + + +def matches_mapping_pattern(value): + match value: + case {}: + return True + case _: + return False + + +def abc_with_tpflags(flags): + return abc.ABCMeta("Tagged", (), {"__abc_tpflags__": flags}) + + +PY_TPFLAGS_SEQUENCE = 1 << 5 +PY_TPFLAGS_MAPPING = 1 << 6 + + +class PlainWithTpflags: + __abc_tpflags__ = PY_TPFLAGS_SEQUENCE + + +assert not matches_sequence_pattern(PlainWithTpflags()) +assert PlainWithTpflags.__abc_tpflags__ == PY_TPFLAGS_SEQUENCE + +assert matches_sequence_pattern(abc_with_tpflags(PY_TPFLAGS_SEQUENCE)()) +assert matches_mapping_pattern(abc_with_tpflags(PY_TPFLAGS_MAPPING)()) +# The value is masked, so a bit outside the two collection flags is ignored +# rather than rejected. +assert matches_sequence_pattern(abc_with_tpflags(PY_TPFLAGS_SEQUENCE | 1)()) +assert not matches_sequence_pattern(abc_with_tpflags(0)()) + +# Whatever it holds, the attribute is consumed -- a leftover would be inherited +# by every subclass and read again. +assert "__abc_tpflags__" not in abc_with_tpflags(PY_TPFLAGS_SEQUENCE).__dict__ +assert "__abc_tpflags__" not in abc_with_tpflags("not an int").__dict__ +assert not matches_sequence_pattern(abc_with_tpflags("not an int")()) + +# A value past the machine word is an error, not a silently skipped one, and a +# collection bit inside such a value does not survive the conversion. +with assert_raises(OverflowError): + abc_with_tpflags(1 << 100) +with assert_raises(OverflowError): + abc_with_tpflags((1 << 130) | PY_TPFLAGS_SEQUENCE) +with assert_raises(OverflowError): + abc_with_tpflags(-(1 << 130)) + +# `-1` carries both collection bits. +with assert_raises(TypeError): + abc_with_tpflags(-1) +with assert_raises(TypeError): + abc_with_tpflags(PY_TPFLAGS_SEQUENCE | PY_TPFLAGS_MAPPING) + +# `PyLong_CheckExact`: a bool and an `int` subclass are consumed and ignored. +assert not matches_sequence_pattern(abc_with_tpflags(True)()) + + +class TpflagsInt(int): + pass + + +assert not matches_sequence_pattern(abc_with_tpflags(TpflagsInt(PY_TPFLAGS_SEQUENCE))()) + +# The attribute is taken out of the type dict itself, so a rejected value is +# consumed all the same and a metaclass `__delattr__` never sees it. +deleted = [] + + +class WatchingMeta(abc.ABCMeta): + def __delattr__(cls, name): + deleted.append(name) + super().__delattr__(name) + + +rejected = WatchingMeta("Rejected", (), {}) +rejected.__abc_tpflags__ = PY_TPFLAGS_SEQUENCE | PY_TPFLAGS_MAPPING +with assert_raises(TypeError): + _abc._abc_init(rejected) +assert "__abc_tpflags__" not in rejected.__dict__ +assert deleted == [] + +WatchingMeta("Tagged", (), {"__abc_tpflags__": PY_TPFLAGS_SEQUENCE}) +assert deleted == [] + +# Registering under a marked ABC hands the marker to the registered class and +# its descendants, but never to an immutable type: a `str` matching `case [...]` +# is the one thing a sequence pattern must not accept. +assert not matches_sequence_pattern("ab") +assert not matches_sequence_pattern(b"ab") + + +class Marked(abc.ABCMeta("SeqBase", (), {"__abc_tpflags__": PY_TPFLAGS_SEQUENCE})): + pass + + +class Unrelated: + pass + + +class UnrelatedChild(Unrelated): + pass + + +Marked.register(Unrelated) +assert matches_sequence_pattern(Unrelated()) +assert matches_sequence_pattern(UnrelatedChild()) +Marked.register(str) +assert not matches_sequence_pattern("ab") diff --git a/pyre/pyre-interpreter/src/call.rs b/pyre/pyre-interpreter/src/call.rs index e2fcae55d77..d21a3651b3b 100644 --- a/pyre/pyre-interpreter/src/call.rs +++ b/pyre/pyre-interpreter/src/call.rs @@ -5498,29 +5498,6 @@ pub unsafe fn create_all_slots( // typeobject.py:1507-1508: inherit flag_map_or_seq from bases pyre_object::typeobject::inherit_flag_map_or_seq(w_type, w_bases); - // typeobject.c type_new: a class body carrying `__abc_tpflags__` - // (`collections.abc` Mapping = `1<<6`, Sequence = `1<<5`) folds its - // COLLECTION_FLAGS into the structural-match marker, so subclasses of - // `abc.Mapping` / `abc.Sequence` match `case {..}` / `case [..]`. The - // bit lives only in the defining body's namespace, so subclasses pick - // it up through `inherit_flag_map_or_seq` above rather than re-reading. - if let Some(w_flags) = crate::type_dict_lookup(w_type, "__abc_tpflags__") - && pyre_object::is_int(w_flags) - { - let flags = pyre_object::w_int_get_value(w_flags); - let collection_flags = flags & ((1 << 6) | (1 << 5)); - if collection_flags == ((1 << 6) | (1 << 5)) { - return Err(crate::PyError::type_error( - "__abc_tpflags__ cannot be both Py_TPFLAGS_SEQUENCE and Py_TPFLAGS_MAPPING", - )); - } - if flags & (1 << 6) != 0 { - pyre_object::typeobject::w_type_set_flag_map_or_seq(w_type, b'M'); - } else if flags & (1 << 5) != 0 { - pyre_object::typeobject::w_type_set_flag_map_or_seq(w_type, b'S'); - } - } - // typeobject.py: copy_flags_from_bases — inherit hasdict/weakrefable/hasuserdel copy_flags_from_bases(w_type, w_bases); diff --git a/pyre/pyre-interpreter/src/module/_abc/mod.rs b/pyre/pyre-interpreter/src/module/_abc/mod.rs index d2bb0fccd17..e62fe90af9e 100644 --- a/pyre/pyre-interpreter/src/module/_abc/mod.rs +++ b/pyre/pyre-interpreter/src/module/_abc/mod.rs @@ -307,6 +307,51 @@ fn abc_init(args: &[PyObjectRef]) -> Result { } let methods = w_frozenset_from_items(&abstract_names); crate::baseobjspace::setattr_str(cls, "__abstractmethods__", methods)?; + + // `app_abc.py _abc_init` — fold a `__abc_tpflags__` in the class body + // into the structural-match marker, then drop the attribute. + // + // This runs here rather than in `type_new` because the marker is only + // meant for a class going through `ABCMeta`: consuming it at class + // creation gives a plain class the marker too, and `case [...]` would + // then accept an object that is not a sequence. + // + // `_abc.c _abc__abc_init_impl` masks the value and applies whatever + // collection bits survive, where `interp_abc.py set_collection_flag` + // compares the whole value against one flag and raises `ValueError` + // otherwise. The mask is the observable behaviour -- `__abc_tpflags__ = + // (1 << 5) | 1` sets the sequence marker -- so mask here and hand the + // primitive the single bit its own contract is written against. + if unsafe { is_type(cls) } + && let Some(w_flags) = crate::type_dict_lookup(cls, "__abc_tpflags__") + { + // `PyDict_Pop(dict, &_Py_ID(__abc_tpflags__), &flags)` -- take the + // entry out of the type dict itself, and take it before validating. + // A metaclass `__delattr__` never sees this, and a value the checks + // below reject is consumed all the same, so a subclass re-running + // `_abc_init` does not inherit the same rejection. + let roots = pyre_object::gc_roots::push_roots(); + let flags_slot = roots.publish(&[w_flags]); + crate::type_dict_delete(cls, "__abc_tpflags__"); + unsafe { crate::baseobjspace::mutated(cls, Some("__abc_tpflags__")) }; + let w_flags = roots.get(flags_slot); + // `PyLong_CheckExact` -- an `int` subclass, `bool` included, is + // consumed and ignored, as is anything that is not an int at all. + // Both int representations spell `int`, so both are exact. + if unsafe { is_exact_type(w_flags, &INT_TYPE) || is_exact_type(w_flags, &LONG_TYPE) } { + // `PyLong_AsLong` -- a value past the machine word raises + // `OverflowError` rather than being skipped like a non-int. + let flags = crate::baseobjspace::int_w(w_flags)?; + if flags & COLLECTION_FLAGS == COLLECTION_FLAGS { + return Err(crate::PyError::type_error( + "__abc_tpflags__ cannot be both Py_TPFLAGS_SEQUENCE and Py_TPFLAGS_MAPPING", + )); + } + if flags & COLLECTION_FLAGS != 0 { + set_collection_flag_of(cls, flags & COLLECTION_FLAGS)?; + } + } + } } Ok(w_none()) } @@ -364,20 +409,91 @@ fn register(args: &[PyObjectRef]) -> Result { Ok(subclass) } -// `interp_abc.py set_collection_flag_recursive` — stamp the marker on -// `w_type` and every class already deriving from it. +/// `typeobject.py PATMA_SEQUENCE` — `Py_TPFLAGS_SEQUENCE`. +const PATMA_SEQUENCE: i64 = 1 << 5; +/// `typeobject.py PATMA_MAPPING` — `Py_TPFLAGS_MAPPING`. +const PATMA_MAPPING: i64 = 1 << 6; +/// `app_abc.py COLLECTION_FLAGS`. +const COLLECTION_FLAGS: i64 = PATMA_SEQUENCE | PATMA_MAPPING; + +/// `interp_abc.py set_collection_flag` — stamp one structural-match marker on +/// `w_type`, without touching its subclasses. +/// +/// `_abc_init` masks `__abc_tpflags__` down to a single bit first, so the +/// strict test below only ever rejects a caller that spells the flag itself. +fn set_collection_flag_of(w_type: PyObjectRef, flag: i64) -> Result<(), crate::PyError> { + let marker = collection_marker(w_type, flag, "_internal_set_collection_flag")?; + unsafe { typeobject::w_type_set_flag_map_or_seq(w_type, marker) }; + Ok(()) +} + +/// The `flag_patma_collection` byte one collection flag stands for, with the +/// two rejections `set_collection_flag` makes before it stamps anything: +/// `space.interp_w(W_TypeObject, w_self)` on the receiver, and the strict +/// one-bit test on the flag. +fn collection_marker(w_type: PyObjectRef, flag: i64, who: &str) -> Result { + if !unsafe { is_type(w_type) } { + return Err(crate::PyError::type_error(format!( + "{who}() argument 1 must be a type" + ))); + } + match flag { + PATMA_SEQUENCE => Ok(b'S'), + PATMA_MAPPING => Ok(b'M'), + _ => Err(crate::PyError::value_error(format!( + "invalid value for __abc_tpflags__: {flag}" + ))), + } +} + +/// `_abc._internal_set_collection_flag(cls, flag)`. +fn internal_set_collection_flag(args: &[PyObjectRef]) -> Result { + let [w_type, w_flag] = args else { + return Err(crate::PyError::type_error( + "_internal_set_collection_flag() requires (cls, flag)", + )); + }; + set_collection_flag_of(*w_type, crate::baseobjspace::int_w(*w_flag)?)?; + Ok(w_none()) +} + +/// `_abc._internal_set_collection_flag_recursive(cls, flag)`. +fn internal_set_collection_flag_recursive( + args: &[PyObjectRef], +) -> Result { + let [w_type, w_flag] = args else { + return Err(crate::PyError::type_error( + "_internal_set_collection_flag_recursive() requires (cls, flag)", + )); + }; + let marker = collection_marker( + *w_type, + crate::baseobjspace::int_w(*w_flag)?, + "_internal_set_collection_flag_recursive", + )?; + // `_PyType_SetFlagsRecursive` starts the guarded walk at the argument + // itself, not at its children: `set_collection_flag` stamps whatever it is + // handed, so entering through that one would let this primitive mark an + // immutable type -- `str` among them. + set_collection_flag_recursive(*w_type, marker); + Ok(w_none()) +} + +// `typeobject.c set_flags_recursive` — stamp the marker on `w_type` and every +// class already deriving from it. `interp_abc.py set_collection_flag_recursive` +// carries neither of the two stops below and would mark `str`. fn set_collection_flag_recursive(w_type: PyObjectRef, flag: u8) { unsafe { - // A non-heap type's marker is fixed at registration - // (`objspace.py:104-108` marks exactly dict / dictproxy / list / - // tuple), and `Py_TPFLAGS_IMMUTABLETYPE` stops the recursion there. - // `_collections_abc` runs `Sequence.register(str)` and - // `ByteString.register(bytes)`, so without this stop `str` / `bytes` / - // `bytearray` would start matching `case [...]` — the one thing a - // sequence pattern must never accept. + // `Py_TPFLAGS_IMMUTABLETYPE`: a non-heap type's marker is fixed at + // registration (`objspace.py StdObjSpace.initialize` marks exactly + // dict / dictproxy / list / tuple). `_collections_abc` runs + // `Sequence.register(str)` and `ByteString.register(bytes)`, so without + // this stop `str` / `bytes` / `bytearray` would start matching + // `case [...]` — the one thing a sequence pattern must never accept. // - // A class already carrying the marker passed it to its descendants at - // creation (`inherit_flag_map_or_seq`), so that subtree is done. + // `(tp_flags & mask) == flags`: a class already carrying the marker + // passed it to its descendants at creation + // (`inherit_flag_map_or_seq`), so that subtree is done. if !typeobject::w_type_is_heaptype(w_type) || typeobject::w_type_get_flag_map_or_seq(w_type) == flag { @@ -479,7 +595,7 @@ fn subclass_of(cls: PyObjectRef, subclass: PyObjectRef) -> Result Result { let cls_slot = roots.publish(&[args[0]]); let instance_slot = roots.publish(&[args[1]]); - // `app_abc.py subclass = instance.__class__`. + // `app_abc.py _abc_instancecheck` `subclass = instance.__class__`. let subclass = crate::baseobjspace::getattr_str(roots.get(instance_slot), "__class__")?; let subclass_slot = roots.publish(&[subclass]); if weak_cache_contains(roots.get(cls_slot), "_abc_cache", roots.get(subclass_slot))? { return Ok(w_bool_from(true)); } - // `app_abc.py subtype = type(instance)` — the instance's real class. + // `app_abc.py _abc_instancecheck` `subtype = type(instance)` — the instance's real class. // User-defined instances carry the generic layout marker in `ob_type` and // the real class in `w_class`, so reading `ob_type` directly would resolve // to `object`; `r#type` returns the class for both builtin and user @@ -609,7 +725,7 @@ fn instancecheck(args: &[PyObjectRef]) -> Result { roots.get(subclass_slot), )?)); } - // `app_abc.py any(cls.__subclasscheck__(c) for c in (subclass, subtype))`. + // `app_abc.py _abc_instancecheck` `any(cls.__subclasscheck__(c) for c in (subclass, subtype))`. for slot in [subclass_slot, subtype_slot] { if subclasscheck_of(roots.get(cls_slot), roots.get(slot))? { return Ok(w_bool_from(true)); @@ -711,6 +827,8 @@ crate::py_module! { "_get_dump" / 1 = get_dump, "_reset_registry" / 1 = reset_registry, "_reset_caches" / 1 = reset_caches, + "_internal_set_collection_flag" / 2 = internal_set_collection_flag, + "_internal_set_collection_flag_recursive" / 2 = internal_set_collection_flag_recursive, }, extra_init: |ns| { crate::importing::appleveldef_install_seeded( diff --git a/pyre/pyre-wasm-runner/src/main.rs b/pyre/pyre-wasm-runner/src/main.rs index 20206a257ef..80d328708ba 100644 --- a/pyre/pyre-wasm-runner/src/main.rs +++ b/pyre/pyre-wasm-runner/src/main.rs @@ -755,6 +755,7 @@ fn run(module_path: &Path, source: &str, script: &Path) -> Result { "inline_decl_call_assembler", "inline_decl_owner_invalidated", "inline_decl_foreign_label", + "inline_decl_source_in_preamble", ]; let mut parts = Vec::new(); for (i, lbl) in labels.iter().enumerate() {