Two fixes for the -g (debug info) option (bytecodealliance#6931)

* Fix up a debug info transform bug

"Block"s in DWARF are arbitrary-sized constants.

They are used for e. g. __int128 constants in C/C++, which is how this was hit.

* Add the ".vs" folder to ".gitignore"

This is generated by VS when using its "open folder feature".

* Monotonize instructon offsets after code emission

Turns out that this is easier then fixing them in-place when
removing the branch because a bunch of ambient state needs to
be passed through to the MachBuffer for that to become possible.

Testing: tested to fix the issue on both one of the reported
samples as well as my own.

* Move the last optimize_branches earlier

It is more obviously correct this way that the code which
is using buffer.cur_offset() is not reading stale values.

* Fix the zero-offset problem with NO_INST_OFFSET

Also delete EmitResult::inst_offsets, it was not used.

* Add a test

* Check both sides of the range for NO_INST_OFFSET

If either is unknown, err on the safe side.

.gitignore

@@ -11,6 +11,7 @@ cranelift.dbg*
 docs/_build
 docs/book
 .vscode/
+.vs/
 rusty-tags.*
 tags
 target

cranelift/codegen/src/machinst/buffer.rs

@@ -851,7 +851,8 @@ impl<I: VCodeInst> MachBuffer<I> {
         //   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;
         }
@@ -1446,10 +1447,6 @@ impl<I: VCodeInst> MachBuffer<I> {
     ) -> MachBufferFinalized<Stencil> {
         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);

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<CodeOffset>,
-
     /// 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<I: VCodeInst> VCode<I> {
     /// New empty VCode.
     fn new(
@@ -807,7 +805,7 @@ impl<I: VCodeInst> VCode<I> {
         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<I: VCodeInst> VCode<I> {
             }
         }
 
+        // 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<I: VCodeInst> VCode<I> {
             }
         }
 
+        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<I: VCodeInst> VCode<I> {
             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<I: VCodeInst> VCode<I> {
         }
     }
 
+    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]  [<would be 46>]
+        // 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: &regalloc2::Output,
@@ -1137,9 +1179,12 @@ impl<I: VCodeInst> VCode<I> {
                 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<I: VCodeInst> VCode<I> {
                 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

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),

tests/all/debug/lldb.rs

@@ -23,7 +23,11 @@ fn lldb_with_script(args: &[&str], script: &str) -> Result<String> {
     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(())
+}

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)))