diff --git a/.gitignore b/.gitignore index 9cad237bc54f..afb3ff15003f 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,7 @@ cranelift.dbg* docs/_build docs/book .vscode/ +.vs/ rusty-tags.* tags target diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index e5de3b40ee66..13aa97fd0011 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -848,7 +848,8 @@ impl MachBuffer { // fixup record referring to that last branch is removed. } - fn optimize_branches(&mut self, ctrl_plane: &mut ControlPlane) { + /// Performs various optimizations on branches pointing at the current label. + pub fn optimize_branches(&mut self, ctrl_plane: &mut ControlPlane) { if ctrl_plane.get_decision() { return; } @@ -1443,10 +1444,6 @@ impl MachBuffer { ) -> MachBufferFinalized { let _tt = timing::vcode_emit_finish(); - // Do any optimizations on branches at tail of buffer, as if we - // had bound one last label. - self.optimize_branches(ctrl_plane); - self.finish_emission_maybe_forcing_veneers(ForceVeneers::No, ctrl_plane); let alignment = self.finish_constants(constants); diff --git a/cranelift/codegen/src/machinst/vcode.rs b/cranelift/codegen/src/machinst/vcode.rs index 6b05b38c8bd3..4d2168f65617 100644 --- a/cranelift/codegen/src/machinst/vcode.rs +++ b/cranelift/codegen/src/machinst/vcode.rs @@ -189,10 +189,6 @@ pub struct EmitResult { /// bb-starts. Computed only if `debug_value_labels` is non-empty. pub bb_edges: Vec<(CodeOffset, CodeOffset)>, - /// Final instruction offsets, recorded during emission. Computed - /// only if `debug_value_labels` is non-empty. - pub inst_offsets: Vec, - /// Final length of function body. pub func_body_len: CodeOffset, @@ -620,6 +616,8 @@ fn is_reftype(ty: Type) -> bool { ty == types::R64 || ty == types::R32 } +const NO_INST_OFFSET: CodeOffset = u32::MAX; + impl VCode { /// New empty VCode. fn new( @@ -807,7 +805,7 @@ impl VCode { let mut disasm = String::new(); if !self.debug_value_labels.is_empty() { - inst_offsets.resize(self.insts.len(), 0); + inst_offsets.resize(self.insts.len(), NO_INST_OFFSET); } // Count edits per block ahead of time; this is needed for @@ -1071,6 +1069,10 @@ impl VCode { } } + // Do any optimizations on branches at tail of buffer, as if we had + // bound one last label. + buffer.optimize_branches(ctrl_plane); + // emission state is not needed anymore, move control plane back out *ctrl_plane = state.take_ctrl_plane(); @@ -1097,6 +1099,7 @@ impl VCode { } } + self.monotonize_inst_offsets(&mut inst_offsets[..], func_body_len); let value_labels_ranges = self.compute_value_labels_ranges(regalloc, &inst_offsets[..], func_body_len); let frame_size = self.abi.frame_size(); @@ -1105,7 +1108,6 @@ impl VCode { buffer: buffer.finish(&self.constants, ctrl_plane), bb_offsets, bb_edges, - inst_offsets, func_body_len, disasm: if want_disasm { Some(disasm) } else { None }, sized_stackslot_offsets: self.abi.sized_stackslot_offsets().clone(), @@ -1115,6 +1117,46 @@ impl VCode { } } + fn monotonize_inst_offsets(&self, inst_offsets: &mut [CodeOffset], func_body_len: u32) { + if self.debug_value_labels.is_empty() { + return; + } + + // During emission, branch removal can make offsets of instructions incorrect. + // Consider the following sequence: [insi][jmp0][jmp1][jmp2][insj] + // It will be recorded as (say): [30] [34] [38] [42] [] + // When the jumps get removed we are left with (in "inst_offsets"): + // [insi][jmp0][jmp1][jmp2][insj][...] + // [30] [34] [38] [42] [34] + // Which violates the monotonicity invariant. This method sets offsets of these + // removed instructions such as to make them appear zero-sized: + // [insi][jmp0][jmp1][jmp2][insj][...] + // [30] [34] [34] [34] [34] + // + let mut next_offset = func_body_len; + for inst_index in (0..(inst_offsets.len() - 1)).rev() { + let inst_offset = inst_offsets[inst_index]; + + // Not all instructions get their offsets recorded. + if inst_offset == NO_INST_OFFSET { + continue; + } + + if inst_offset > next_offset { + trace!( + "Fixing code offset of the removed Inst {}: {} -> {}", + inst_index, + inst_offset, + next_offset + ); + inst_offsets[inst_index] = next_offset; + continue; + } + + next_offset = inst_offset; + } + } + fn compute_value_labels_ranges( &self, regalloc: ®alloc2::Output, @@ -1137,9 +1179,12 @@ impl VCode { inst_offsets[to.inst().index()] }; - // Empty range or to-offset of zero can happen because of - // cold blocks (see above). - if to_offset == 0 || from_offset == to_offset { + // Empty ranges or unavailable offsets can happen + // due to cold blocks and branch removal (see above). + if from_offset == NO_INST_OFFSET + || to_offset == NO_INST_OFFSET + || from_offset == to_offset + { continue; } @@ -1158,24 +1203,34 @@ impl VCode { continue; }; - ranges.push(ValueLocRange { + // ValueLocRanges are recorded by *instruction-end + // offset*. `from_offset` is the *start* of the + // instruction; that is the same as the end of another + // instruction, so we only want to begin coverage once + // we are past the previous instruction's end. + let start = from_offset + 1; + + // Likewise, `end` is exclusive, but we want to + // *include* the end of the last + // instruction. `to_offset` is the start of the + // `to`-instruction, which is the exclusive end, i.e., + // the first instruction not covered. That + // instruction's start is the same as the end of the + // last instruction that is included, so we go one + // byte further to be sure to include it. + let end = to_offset + 1; + + trace!( + "Recording debug range for VL{} in {:?}: [Inst {}..Inst {}) [{}..{})", + label, loc, - // ValueLocRanges are recorded by *instruction-end - // offset*. `from_offset` is the *start* of the - // instruction; that is the same as the end of another - // instruction, so we only want to begin coverage once - // we are past the previous instruction's end. - start: from_offset + 1, - // Likewise, `end` is exclusive, but we want to - // *include* the end of the last - // instruction. `to_offset` is the start of the - // `to`-instruction, which is the exclusive end, i.e., - // the first instruction not covered. That - // instruction's start is the same as the end of the - // last instruction that is included, so we go one - // byte further to be sure to include it. - end: to_offset + 1, - }); + from.inst().index(), + to.inst().index(), + start, + end + ); + + ranges.push(ValueLocRange { loc, start, end }); } value_labels_ranges diff --git a/crates/cranelift/src/debug/transform/attr.rs b/crates/cranelift/src/debug/transform/attr.rs index 8ae62562d113..d4357f1db88d 100644 --- a/crates/cranelift/src/debug/transform/attr.rs +++ b/crates/cranelift/src/debug/transform/attr.rs @@ -98,6 +98,7 @@ where let addr = addr_tr.translate(u).unwrap_or(write::Address::Constant(0)); write::AttributeValue::Address(addr) } + AttributeValue::Block(d) => write::AttributeValue::Block(d.to_slice()?.to_vec()), AttributeValue::Udata(u) => write::AttributeValue::Udata(u), AttributeValue::Data1(d) => write::AttributeValue::Data1(d), AttributeValue::Data2(d) => write::AttributeValue::Data2(d), diff --git a/tests/all/debug/lldb.rs b/tests/all/debug/lldb.rs index b6a097eb60e7..abc4eceef80e 100644 --- a/tests/all/debug/lldb.rs +++ b/tests/all/debug/lldb.rs @@ -23,7 +23,11 @@ fn lldb_with_script(args: &[&str], script: &str) -> Result { let mut me = std::env::current_exe().expect("current_exe specified"); me.pop(); // chop off the file name me.pop(); // chop off `deps` - me.push("wasmtime"); + if cfg!(target_os = "windows") { + me.push("wasmtime.exe"); + } else { + me.push("wasmtime"); + } cmd.arg(me); cmd.arg("--"); @@ -169,3 +173,31 @@ check: resuming )?; Ok(()) } + +#[test] +#[ignore] +#[cfg(all( + any(target_os = "linux", target_os = "macos"), + target_pointer_width = "64" +))] +pub fn test_debug_inst_offsets_are_correct_when_branches_are_removed() -> Result<()> { + let output = lldb_with_script( + &[ + "--disable-cache", + "-g", + "--opt-level", + "0", + "tests/all/debug/testsuite/two_removed_branches.wasm", + ], + r#"r"#, + )?; + + // We are simply checking that the output compiles. + check_lldb_output( + &output, + r#" +check: exited with status +"#, + )?; + Ok(()) +} diff --git a/tests/all/debug/testsuite/two_removed_branches.wasm b/tests/all/debug/testsuite/two_removed_branches.wasm new file mode 100644 index 000000000000..3a907b9bd0bc Binary files /dev/null and b/tests/all/debug/testsuite/two_removed_branches.wasm differ diff --git a/tests/all/debug/testsuite/two_removed_branches.wat b/tests/all/debug/testsuite/two_removed_branches.wat new file mode 100644 index 000000000000..f00c7931f32e --- /dev/null +++ b/tests/all/debug/testsuite/two_removed_branches.wat @@ -0,0 +1,59 @@ +(module + (type (;0;) (func)) + (type (;1;) (func (param i32 i32 i32) (result i32))) + (func (;0;) (type 1) (param i32 i32 i32) (result i32) + (local i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32) + i32.const -1 + local.set 6 + local.get 5 + i32.load offset=12 + local.set 8 + local.get 6 + local.set 9 + local.get 8 + local.get 9 + i32.eq + local.set 10 + i32.const 1 + local.set 11 + local.get 10 + local.get 11 + i32.and + local.set 12 + block ;; label = @1 + block ;; label = @2 + local.get 12 + i32.eqz + br_if 0 (;@2;) + i32.const 1 + local.set 13 + local.get 13 + local.set 14 + br 1 (;@1;) + end + local.get 5 + i32.load offset=12 + local.set 15 + local.get 15 + local.set 14 + end + local.get 14 + local.set 16 + local.get 5 + i32.load offset=8 + local.set 17 + local.get 5 + i32.load offset=4 + local.set 18 + local.get 16 + local.get 17 + local.get 18 + call 1 + return) + (func (;1;) (type 1) (param i32 i32 i32) (result i32) + i32.const -1) + (func (;2;) (type 0)) + (table (;0;) 1368 1368 funcref) + (memory (;0;) 800 8000) + (export "memory" (memory 0)) + (export "_start" (func 2)))