diff --git a/majit/majit-backend-wasm/src/codegen.rs b/majit/majit-backend-wasm/src/codegen.rs index c54c4505c4e..b5fc88951c5 100644 --- a/majit/majit-backend-wasm/src/codegen.rs +++ b/majit/majit-backend-wasm/src/codegen.rs @@ -2376,7 +2376,10 @@ fn value_id_end(inputargs: &[InputArg], ops: &[Op]) -> u32 { /// /// `TempVar` ids live in a reserved high strip and constants in their own /// namespace; neither indexes a value local, so both pass through unchanged. -fn rebase_region_value_ids(bridge: &InlinedBridge, offset: u32) -> (InlinedBridge, u32) { +fn rebase_region_value_ids( + bridge: &InlinedBridge, + offset: u32, +) -> Result<(InlinedBridge, u32), BackendError> { use majit_ir::operand::Operand; let shift = |r: OpRef| -> OpRef { @@ -2388,6 +2391,20 @@ fn rebase_region_value_ids(bridge: &InlinedBridge, offset: u32) -> (InlinedBridg }; let width = value_id_end(&bridge.inputargs, &bridge.ops); + // `with_raw` keeps the variant, but the emitters classify by raw payload + // (`OpRef::raw_is_constant`), so an id shifted to or past the limit reads + // as a constant and its result is skipped. Decline instead: the merged + // stream is an optimization, and no renumbering is correct once the + // region's range no longer fits below the limit. + if offset + .checked_add(width) + .is_none_or(|end| end > OpRef::VALUE_ID_LIMIT) + { + return Err(BackendError::Unsupported(format!( + "wasm backend: inlined bridge value ids exceed the value-id space \ + (offset {offset}, width {width})" + ))); + } let inputargs: Vec = bridge .inputargs .iter() @@ -2423,7 +2440,7 @@ fn rebase_region_value_ids(bridge: &InlinedBridge, offset: u32) -> (InlinedBridg } } - ( + Ok(( InlinedBridge { source_fail_index: bridge.source_fail_index, trace_id: bridge.trace_id, @@ -2433,7 +2450,7 @@ fn rebase_region_value_ids(bridge: &InlinedBridge, offset: u32) -> (InlinedBridg constants: bridge.constants.clone(), }, width, - ) + )) } /// Build a wasm module from majit IR. @@ -2486,7 +2503,7 @@ pub fn build_wasm_module( rebased_constants = constants.clone(); let mut next_value_id = value_id_end(inputargs, ops); for bridge in inlined_bridges { - let (bridge, width) = rebase_region_value_ids(bridge, next_value_id); + let (bridge, width) = rebase_region_value_ids(bridge, next_value_id)?; // The pool is keyed by value position for a folded value with no // producing op, so rebasing the region's ids moved its reads off // its own entries. Replay that window at the offset, and drop a @@ -2545,6 +2562,33 @@ pub fn build_wasm_module( "wasm backend: inlined bridge stream has an empty region".into(), )); } + // A region can carry a CALL_ASSEMBLER this build has no arm for. The + // dedicated arm is selected by `ca.emit_ca`, which is decided when the + // OWNER is compiled, and it reads the callee's geometry out of + // `ca.targets`; a region merged in later brings its own callee. An op + // that misses that arm does not fail — it falls through to the ordinary + // residual-call arm, which lowers arg 0 as an + // `__indirect_function_table` slot, and a CALL_ASSEMBLER's arg 0 is the + // callee's first frame slot. That calls whatever the slot happens to + // index and returns its result as the callee's, which is a silent wrong + // answer rather than a trap. `wasm_unsupported_trace_reason` asks this + // question of every trace's own ops; the merged stream is the one place + // it is never re-asked, so ask it here. + for op in &bridge.ops { + if !op.opcode.is_call_assembler() { + continue; + } + let target = op + .getdescr() + .and_then(|descr| descr.as_call_descr().and_then(|d| d.call_target_token())); + if !ca.emit_ca || target.is_none_or(|token| !ca.targets.contains_key(&token)) { + return Err(BackendError::Unsupported(format!( + "wasm backend: inlined bridge carries {:?}, which the owner \ + build has no CALL_ASSEMBLER arm for", + op.opcode + ))); + } + } let source_guard = guards .get(bridge.source_fail_index as usize) .ok_or_else(|| { diff --git a/majit/majit-backend-wasm/src/lib.rs b/majit/majit-backend-wasm/src/lib.rs index 60a09a7559c..a55ea18b8ed 100644 --- a/majit/majit-backend-wasm/src/lib.rs +++ b/majit/majit-backend-wasm/src/lib.rs @@ -83,8 +83,9 @@ use std::sync::{Arc, Mutex}; /// because the source module has frame-only dispatch; 46 = parameter entry /// declined because the source guard and bridge input arities disagree; 47 = /// LABEL publication suppressed because the bridge entry has nonzero parameters. -/// 48 = an inline trial's LABEL-resume storage exceeds the frozen frame. -pub static BRIDGE_DIAG: [AtomicU64; 49] = [const { AtomicU64::new(0) }; 49]; +/// 48 = an inline trial's LABEL-resume storage exceeds the frozen frame; 49 = +/// the region carries a CALL_ASSEMBLER the owner build emits no arm for. +pub static BRIDGE_DIAG: [AtomicU64; 50] = [const { AtomicU64::new(0) }; 50]; #[repr(u8)] #[derive(Clone, Copy)] @@ -117,6 +118,10 @@ impl FrameShortage { /// shortage without changing the compile result. static INLINE_GEOMETRY: [AtomicU64; 3] = [const { AtomicU64::new(0) }; 3]; static INLINE_GEOMETRY_COUNT: AtomicU64 = AtomicU64::new(0); +/// The first three reasons an inline-bridge install was refused, verbatim. +/// The names carry "trial" because they are a guest export the runner looks up +/// by string; the errors themselves come from the install itself, which is the +/// only build there is. static INLINE_TRIAL_ERRORS: Mutex> = Mutex::new(Vec::new()); pub(crate) fn record_inline_geometry(kind: FrameShortageKind, needed: usize, available: usize) { @@ -157,6 +162,39 @@ fn record_inline_trial_error(error: &BackendError) { } } +/// Sort a refused inline install into the decline tallies the host prints. +/// `replace_module` rejecting the bytes, or a build with no host binding to +/// replace them through, is a re-emission outcome and stays on its own counter; +/// every other reason is the merged module declining to emit, which is what the +/// per-shortage buckets are for. +fn classify_inline_install_error(error: &BackendError) { + let BackendError::Unsupported(reason) = error else { + diag_bump(37); + diag_bump(43); + return; + }; + if reason.contains("wasm host rejected the re-emitted trace module") + || reason.contains("no host replacement binding") + { + diag_bump(30); + return; + } + diag_bump(37); + if reason.contains("frame value slots exceed frozen frame layout") { + diag_bump(40); + } else if reason.contains("ordinary ref homes") { + diag_bump(41); + } else if reason.contains("label resume layout") { + diag_bump(48); + } else if reason.contains("no CALL_ASSEMBLER arm for") { + diag_bump(49); + } else if reason.contains("inlined bridge stream has no local loop LABEL") { + diag_bump(42); + } else { + diag_bump(43); + } +} + static REEMIT_ENABLED: AtomicBool = AtomicBool::new(false); static INLINE_BRIDGE_ENABLED: AtomicBool = AtomicBool::new(false); static BRIDGE_PARAMS_ENABLED: AtomicBool = AtomicBool::new(true); @@ -3349,84 +3387,62 @@ impl majit_backend::Backend for WasmBackend { candidate.classptr_to_typeid = self.collect_classptr_typeid_table(&merged_ops); candidate.guard_gc_type_info = self.collect_guard_gc_type_info(&merged_ops); candidate.nursery = nursery_alloc_params(&merged_ops); - match codegen::build_wasm_module(&candidate) { - Err(ref error @ BackendError::Unsupported(ref reason)) => { - record_inline_trial_error(error); - diag_bump(37); - if reason.contains("frame value slots exceed frozen frame layout") { - diag_bump(40); - } else if reason.contains("ordinary ref homes") { - diag_bump(41); - } else if reason.contains("label resume layout") { - diag_bump(48); - } else if reason - .contains("inlined bridge stream has no local loop LABEL") - { - diag_bump(42); - } else { - diag_bump(43); + let source_loop = original_token + .compiled + .get() + .and_then(|c| c.downcast_ref::()) + .expect("source loop disappeared before inline install"); + // The local branch supersedes any previous direct-cell + // dispatch for this guard. Remove it before reemit so + // the fresh array cannot replay a contradictory slot. + let old_bridge_slot = source_loop + .bridge_slots + .borrow_mut() + .remove(&source_fail_index); + #[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))] + if source_cells_base != 0 { + let cell = (source_cells_base as usize + source_fail_index as usize * 4) + as *mut u32; + unsafe { core::ptr::write(cell, 0) }; + } + // Eligibility IS the emission: `reemit_loop` runs the same + // `build_wasm_module` over the same candidate, and nothing + // it does before that call mutates state a failure would + // have to unwind — it reads the fail-index base and + // allocates a cell array that is dropped on the error path. + // So install directly and let the build answer, instead of + // asking it once as a trial and once for real. + let old_inputs = source_loop.reemit.replace(Some(candidate)); + match self.reemit_loop(original_token) { + Ok(()) => { + self.trace_counter += 1; + if let Some(table) = gc_table { + Self::register_gc_table(original_token, table); } + diag_bump(31); + diag_bump(32); + return Ok(AsmInfo { + code_addr: 0, + code_size: 0, + }); } - Err(ref error @ BackendError::CompilationFailed(_)) => { - record_inline_trial_error(error); - diag_bump(37); - diag_bump(43); - } - Ok(_) => { - let source_loop = original_token - .compiled - .get() - .and_then(|c| c.downcast_ref::()) - .expect("source loop disappeared before inline install"); - // The local branch supersedes any previous direct-cell - // dispatch for this guard. Remove it before reemit so - // the fresh array cannot replay a contradictory slot. - let old_bridge_slot = source_loop - .bridge_slots - .borrow_mut() - .remove(&source_fail_index); - #[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))] - if source_cells_base != 0 { - let cell = (source_cells_base as usize - + source_fail_index as usize * 4) - as *mut u32; - unsafe { core::ptr::write(cell, 0) }; - } - let old_inputs = source_loop.reemit.replace(Some(candidate)); - match self.reemit_loop(original_token) { - Ok(()) => { - self.trace_counter += 1; - if let Some(table) = gc_table { - Self::register_gc_table(original_token, table); - } - diag_bump(31); - diag_bump(32); - return Ok(AsmInfo { - code_addr: 0, - code_size: 0, - }); - } - Err(_) => { - source_loop.reemit.replace(old_inputs); - if let Some(slot) = old_bridge_slot { - source_loop - .bridge_slots - .borrow_mut() - .insert(source_fail_index, slot); - #[cfg(all( - target_arch = "wasm32", - not(target_os = "wasi") - ))] - if source_cells_base != 0 { - let cell = (source_cells_base as usize - + source_fail_index as usize * 4) - as *mut u32; - unsafe { core::ptr::write(cell, slot) }; - } - } - diag_bump(30); + Err(error) => { + source_loop.reemit.replace(old_inputs); + if let Some(slot) = old_bridge_slot { + source_loop + .bridge_slots + .borrow_mut() + .insert(source_fail_index, slot); + #[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))] + if source_cells_base != 0 { + let cell = (source_cells_base as usize + + source_fail_index as usize * 4) + as *mut u32; + unsafe { core::ptr::write(cell, slot) }; } } + record_inline_trial_error(&error); + classify_inline_install_error(&error); } } } diff --git a/majit/majit-backend-wasm/tests/codegen_test.rs b/majit/majit-backend-wasm/tests/codegen_test.rs index afaa2daac31..472fe4701c5 100644 --- a/majit/majit-backend-wasm/tests/codegen_test.rs +++ b/majit/majit-backend-wasm/tests/codegen_test.rs @@ -1305,6 +1305,134 @@ fn inlined_bridge_without_owner_loop_label_declines() { assert!(error.to_string().contains("no local loop LABEL")); } +/// A region merged into an owner brings its own CALL_ASSEMBLER callee, but the +/// dedicated CA arm is selected by `ca.emit_ca` and bakes the callee geometry +/// out of `ca.targets` — both decided when the OWNER was compiled. An op that +/// misses that arm does not fail: it falls through to the ordinary +/// residual-call arm, which lowers arg 0 as an `__indirect_function_table` +/// slot, while a CALL_ASSEMBLER's arg 0 is the callee's first frame slot. That +/// calls whatever the slot happens to index and hands the answer back as the +/// callee's — a silent wrong result rather than a trap. Every trace's own ops +/// are screened for unsupported opcodes before compilation; the merged stream +/// is the one place that question is never re-asked, so the merge asks it. +#[test] +fn inlined_bridge_carrying_an_unarmed_call_assembler_declines() { + fn build( + region_ops: Vec, + ca: codegen::CaParams, + ) -> Result, majit_backend::BackendError> { + let inputargs = vec![ + InputArg::from_type(Type::Int, 0), + InputArg::from_type(Type::Int, 1), + ]; + let owner_ops = vec![ + Op::new( + OpCode::Label, + &[rb(OpRef::input_arg_int(0)), rb(OpRef::input_arg_int(1))], + ), + make_op( + OpCode::IntAdd, + &[OpRef::input_arg_int(0), OpRef::const_int(1)], + OpRef::int_op(2), + ), + make_guard( + OpCode::GuardTrue, + &[OpRef::int_op(2)], + &[OpRef::int_op(2), OpRef::input_arg_int(1)], + ), + Op::new( + OpCode::Jump, + &[rb(OpRef::int_op(2)), rb(OpRef::input_arg_int(1))], + ), + ]; + let inputs = codegen::ModuleBuildInputs { + inputargs: inputargs.iter().map(InputArg::fresh_value_copy).collect(), + ops: owner_ops, + inlined_bridges: vec![codegen::InlinedBridge { + source_fail_index: 0, + trace_id: 7, + inputargs: vec![ + InputArg::from_type(Type::Int, 40), + InputArg::from_type(Type::Int, 41), + ], + ops: region_ops, + 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::build_wasm_module(&inputs).map(|(bytes, _, _)| bytes) + } + + /// The region the decline arms use, with `opcode` producing its one value. + /// The loop-closing JUMP carries the region's own inputs, so the result + /// type never reaches the owner's label and only the opcode varies. + fn region_ops(opcode: OpCode, result: OpRef) -> Vec { + vec![ + make_op( + opcode, + &[OpRef::input_arg_int(40), OpRef::input_arg_int(41)], + result, + ), + Op::new( + OpCode::Jump, + &[rb(OpRef::input_arg_int(40)), rb(OpRef::input_arg_int(41))], + ), + ] + } + + // The same region under an ordinary opcode, to pin that what declines + // below is the CALL_ASSEMBLER and not the fixture. + let plain = build( + region_ops(OpCode::IntAdd, OpRef::int_op(42)), + codegen::CaParams::default(), + ) + .expect("a loop-closing region with no CALL_ASSEMBLER merges into its owner"); + validate_wasm(&plain); + + for (opcode, result) in [ + (OpCode::CallAssemblerI, OpRef::int_op(42)), + (OpCode::CallAssemblerR, OpRef::ref_op(42)), + ] { + for ca in [ + // The owner emitted no CA arm at all. + codegen::CaParams::default(), + // The owner has the arm, but not for THIS callee: the region's + // target is absent from the table the arm bakes its geometry from. + codegen::CaParams { + emit_ca: true, + ..codegen::CaParams::default() + }, + ] { + let error = match build(region_ops(opcode, result), ca) { + Ok(_) => panic!("{opcode:?} has no arm in this build, so the merge must decline"), + Err(error) => error, + }; + assert!( + error.to_string().contains("no CALL_ASSEMBLER arm for"), + "declined for the wrong reason: {error}" + ); + } + } +} + /// A region's value ids are its own trace's, so they collide with the owner's. /// The merged stream has one local namespace, so an unrebased collision makes /// the region's entry moves land in locals the owner still holds live across @@ -1363,20 +1491,46 @@ fn inlined_bridge_emission_is_independent_of_the_regions_own_numbering() { // its first input arg share an id with the owner's loop-invariant // `int_op(2)`; `base = 40` clears every owner id. fn build(base: u32) -> Vec { + let region_pool = indexmap::IndexMap::from([(base + 3, 0x5a5a_007)]); + let first = build_with(base, region_pool.clone(), indexmap::IndexMap::new()) + .expect("a loop-closing region merges into its owner"); + // A region is RETAINED for the next re-emission, and `reemit_loop` + // rebases the same retained copy every time, so the rebase must leave + // it untouched. Building twice is what catches a rebase that wrote + // through to the region it read. + let second = build_with(base, region_pool, indexmap::IndexMap::new()) + .expect("a retained region rebases identically on re-emission"); + assert_eq!(first, second, "rebasing mutated the retained region"); + first + } + + fn build_with( + base: u32, + region_constants: indexmap::IndexMap, + owner_constants: indexmap::IndexMap, + ) -> Result, majit_backend::BackendError> { let inputargs = vec![ InputArg::from_type(Type::Int, 0), InputArg::from_type(Type::Int, 1), ]; + // `int_op(base + 3)` has no producing op: it is a folded value that + // only the region's own constant pool binds, so the merge has to move + // its pool key by the same offset it moves the read by. let region_ops = vec![ make_op( OpCode::IntAdd, &[OpRef::input_arg_int(base), OpRef::input_arg_int(base + 1)], OpRef::int_op(base + 2), ), + make_op( + OpCode::IntAdd, + &[OpRef::int_op(base + 2), OpRef::int_op(base + 3)], + OpRef::int_op(base + 4), + ), Op::new( OpCode::Jump, &[ - rb(OpRef::int_op(base + 2)), + rb(OpRef::int_op(base + 4)), rb(OpRef::input_arg_int(base + 1)), ], ), @@ -1393,9 +1547,9 @@ fn inlined_bridge_emission_is_independent_of_the_regions_own_numbering() { ], ops: region_ops, gc_table_base: 0, - constants: indexmap::IndexMap::new(), + constants: region_constants, }], - constants: indexmap::IndexMap::new(), + constants: owner_constants, vtable_offset: Some(0), classptr_to_typeid: HashMap::new(), guard_gc_type_info: codegen::GuardGcTypeInfo::default(), @@ -1414,9 +1568,7 @@ fn inlined_bridge_emission_is_independent_of_the_regions_own_numbering() { frame: codegen::FrameGeometry::fixed(), ca: codegen::CaParams::default(), }; - codegen::build_wasm_module(&inputs) - .expect("a loop-closing region merges into its owner") - .0 + codegen::build_wasm_module(&inputs).map(|(bytes, _, _)| bytes) } let colliding = build(2); @@ -1424,6 +1576,28 @@ fn inlined_bridge_emission_is_independent_of_the_regions_own_numbering() { validate_wasm(&colliding); validate_wasm(&disjoint); assert_eq!(colliding, disjoint); + + // A key the owner pool carries inside the window the region is rebased + // into names one of the REGION's ids once the merge is done, not the + // owner value it was recorded for. Left in place it answers a read the + // region's own pool declines, so the module builds on unrelated bits + // instead of declining. Dropping the region's seed must therefore reach + // the decline even though the owner pool has an entry at that position. + let stale = build_with( + 2, + indexmap::IndexMap::new(), + indexmap::IndexMap::from([(12, 0x1234)]), + ); + let error = match stale { + Ok(_) => panic!("a stale owner key inside the region window answered the region's read"), + Err(error) => error, + }; + assert!( + error + .to_string() + .contains("read with no producing op and no"), + "unexpected decline: {error}" + ); } #[test] diff --git a/majit/majit-ir/src/resoperation.rs b/majit/majit-ir/src/resoperation.rs index 5f692c963eb..f05487f3d9e 100644 --- a/majit/majit-ir/src/resoperation.rs +++ b/majit/majit-ir/src/resoperation.rs @@ -301,6 +301,13 @@ impl OpRef { raw & Self::CONST_BIT != 0 && raw < Self::SENTINEL_BASE } + /// One past the highest raw an ordinary value id may carry. Everything + /// from here up is either the constant namespace or the `TempVar` + /// sentinel strip, so a value id renumbered to or past this stops naming + /// a value and starts reading as a constant. A pass that shifts ids has + /// to check its range against this before it moves anything. + pub const VALUE_ID_LIMIT: u32 = Self::CONST_BIT; + /// Bit-helper variant of `const_index()` for callers that hold a raw /// u32 known to be a constant-namespace key. See `raw_is_constant` /// for context. diff --git a/pyre/pyre-wasm-runner/src/main.rs b/pyre/pyre-wasm-runner/src/main.rs index 850fe40838e..bf08ac14ece 100644 --- a/pyre/pyre-wasm-runner/src/main.rs +++ b/pyre/pyre-wasm-runner/src/main.rs @@ -738,6 +738,7 @@ fn run(module_path: &Path, source: &str, script: &Path) -> Result { "bridge_param_decl_arity", "bridge_param_label_suppressed", "inline_decl_label_resume_layout", + "inline_decl_call_assembler", ]; let mut parts = Vec::new(); for (i, lbl) in labels.iter().enumerate() { @@ -1755,6 +1756,30 @@ fn jit_execute(caller: &mut Caller<'_, Host>, func_id: u32, frame_ptr: u32) -> R Ok(ret) } +/// Name a call-area FUNC field that indexes no live function, once per value. +/// +/// The zero sentinel above is the only slot that legitimately has no function +/// behind it. Any other index that misses leaves nothing to call, and the 0 +/// written in its place is indistinguishable from a null Ref the callee +/// returned -- so a call lowered against the wrong operand is answered with a +/// plausible value instead of a trap, and only surfaces much later as a wrong +/// result. Say the slot out loud here so it surfaces at the call instead. +fn report_dead_call_slot(func_ptr: u32) { + static SEEN: std::sync::Mutex>> = + std::sync::Mutex::new(None); + if SEEN + .lock() + .unwrap() + .get_or_insert_with(Default::default) + .insert(func_ptr) + { + eprintln!( + "[warn] residual call area names function table slot {func_ptr}, \ + which holds no function; the call answers 0" + ); + } +} + /// Dispatch a residual call requested by a running trace. // PROBE(PYRE_WASM_CALL_HIST): temporary per-callee crossing histogram. static PROBE_CALL_HIST: std::sync::Mutex>> = @@ -1833,6 +1858,7 @@ fn jit_call_trampoline_inner( let func = match table.get(&mut *caller, func_ptr as u64) { Some(Ref::Func(Some(f))) => f, _ => { + report_dead_call_slot(func_ptr); write_i64(&memory, &mut *caller, call_area, 0)?; return Ok(()); }