Cranelift: fix debuginfo wrt cold blocks and non-monotonic layout.

The debuginfo analyses are written with the assumption that the order of
instructions in the VCode is the order of instructions in the final
machine ocde. This was previously a strong invariant, until we
introduced support for cold blocks. Cold blocks are implemented by
reordering during emission, because the VCode ordering has other
requirements related to lowering (respecting def-use dependencies in the
reverse pass), so it is much simpler to reorder instructions at the last
moment. Unfortunately, this causes the breakage we now see.

This commit fixes the issue by skipping all cold instructions when
emitting value-label ranges (which are translated into debuginfo). This
means that variables defined in cold blocks will not have DWARF
metadata. But cold blocks are usually compiler-inserted slowpaths, not
user code, so this is probably OK. Debuginfo is always best-effort, so
in any case this does not violate any correctness constraints.

cranelift/codegen/src/machinst/debug.rs

@@ -378,16 +378,26 @@ where
 ///   which that label is bound.
 pub(crate) fn compute<I: VCodeInst>(
     insts: &[I],
-    inst_ends: &[u32],
+    layout_info: &InstsLayoutInfo,
-    label_insn_index: &[u32],
 ) -> ValueLabelsRanges {
-    let inst_start = |idx: usize| if idx == 0 { 0 } else { inst_ends[idx - 1] };
+    let inst_start = |idx: usize| {
+        if idx == 0 {
+            0
+        } else {
+            layout_info.inst_end_offsets[idx - 1]
+        }
+    };
 
     trace!("compute: insts =");
     for i in 0..insts.len() {
-        trace!(" #{} end: {} -> {:?}", i, inst_ends[i], insts[i]);
+        trace!(
+            " #{} end: {} -> {:?}",
+            i,
+            layout_info.inst_end_offsets[i],
+            insts[i]
+        );
     }
-    trace!("label_insn_index: {:?}", label_insn_index);
+    trace!("label_insn_index: {:?}", layout_info.label_inst_indices);
 
     // Info at each block head, indexed by label.
     let mut block_starts: HashMap<u32, AnalysisInfo> = HashMap::new();
@@ -409,7 +419,7 @@ pub(crate) fn compute<I: VCodeInst>(
         trace!("at block {} -> state: {:?}", block, state);
         // Iterate for each instruction in the block (we break at the first
         // terminator we see).
-        let mut index = label_insn_index[block as usize];
+        let mut index = layout_info.label_inst_indices[block as usize];
         while index < insts.len() as u32 {
             state.step(&insts[index as usize]);
             trace!(" -> inst #{}: {:?}", index, insts[index as usize]);
@@ -446,18 +456,33 @@ pub(crate) fn compute<I: VCodeInst>(
     // value-label locations.
 
     let mut value_labels_ranges: ValueLabelsRanges = HashMap::new();
-    for block in 0..label_insn_index.len() {
+    for block in 0..layout_info.label_inst_indices.len() {
-        let start_index = label_insn_index[block];
+        let start_index = layout_info.label_inst_indices[block];
-        let end_index = if block == label_insn_index.len() - 1 {
+        let end_index = if block == layout_info.label_inst_indices.len() - 1 {
             insts.len() as u32
         } else {
-            label_insn_index[block + 1]
+            layout_info.label_inst_indices[block + 1]
         };
         let block = block as u32;
         let mut state = block_starts.get(&block).unwrap().clone();
         for index in start_index..end_index {
             let offset = inst_start(index as usize);
-            let end = inst_ends[index as usize];
+            let end = layout_info.inst_end_offsets[index as usize];
+
+            // Cold blocks cause instructions to occur out-of-order wrt
+            // others. We rely on the monotonic mapping from instruction
+            // index to offset in machine code for this analysis to work,
+            // so we just skip debuginfo for cold blocks. This should be
+            // generally fine, as cold blocks generally constitute
+            // slowpaths for expansions of particular ops, rather than
+            // user-written code.
+            if layout_info.start_of_cold_code.is_some()
+                && offset >= layout_info.start_of_cold_code.unwrap()
+            {
+                continue;
+            }
+
+            assert!(offset <= end);
             state.step(&insts[index as usize]);
 
             for (label, locs) in &state.label_to_locs {

cranelift/codegen/src/machinst/vcode.rs

@@ -107,9 +107,10 @@ pub struct VCode<I: VCodeInst> {
     /// Do we generate debug info?
     generate_debug_info: bool,
 
-    /// Instruction end offsets, instruction indices at each label, and total
+    /// Instruction end offsets, instruction indices at each label,
-    /// buffer size.  Only present if `generate_debug_info` is set.
+    /// total buffer size, and start of cold code.  Only present if
-    insts_layout: RefCell<(Vec<u32>, Vec<u32>, u32)>,
+    /// `generate_debug_info` is set.
+    insts_layout: RefCell<InstsLayoutInfo>,
 
     /// Constants.
     constants: VCodeConstants,
@@ -119,6 +120,13 @@ pub struct VCode<I: VCodeInst> {
     has_value_labels: bool,
 }
 
+#[derive(Debug, Default)]
+pub(crate) struct InstsLayoutInfo {
+    pub(crate) inst_end_offsets: Vec<CodeOffset>,
+    pub(crate) label_inst_indices: Vec<CodeOffset>,
+    pub(crate) start_of_cold_code: Option<CodeOffset>,
+}
+
 /// A builder for a VCode function body. This builder is designed for the
 /// lowering approach that we take: we traverse basic blocks in forward
 /// (original IR) order, but within each basic block, we generate code from
@@ -316,7 +324,7 @@ impl<I: VCodeInst> VCode<I> {
             safepoint_insns: vec![],
             safepoint_slots: vec![],
             generate_debug_info,
-            insts_layout: RefCell::new((vec![], vec![], 0)),
+            insts_layout: RefCell::new(Default::default()),
             constants,
             has_value_labels: false,
         }
@@ -467,8 +475,8 @@ impl<I: VCodeInst> VCode<I> {
         buffer.reserve_labels_for_blocks(self.num_blocks() as BlockIndex);
         buffer.reserve_labels_for_constants(&self.constants);
 
-        let mut inst_ends = vec![0; self.insts.len()];
+        let mut inst_end_offsets = vec![0; self.insts.len()];
-        let mut label_insn_iix = vec![0; self.num_blocks()];
+        let mut label_inst_indices = vec![0; self.num_blocks()];
 
         // Construct the final order we emit code in: cold blocks at the end.
         let mut final_order: SmallVec<[BlockIndex; 16]> = smallvec![];
@@ -481,12 +489,14 @@ impl<I: VCodeInst> VCode<I> {
                 final_order.push(block);
             }
         }
-        final_order.extend(cold_blocks);
+        let first_cold_block = cold_blocks.first().cloned();
+        final_order.extend(cold_blocks.clone());
 
         // Emit blocks.
         let mut safepoint_idx = 0;
         let mut cur_srcloc = None;
         let mut last_offset = None;
+        let mut start_of_cold_code = None;
         for block in final_order {
             let new_offset = I::align_basic_block(buffer.cur_offset());
             while new_offset > buffer.cur_offset() {
@@ -496,9 +506,13 @@ impl<I: VCodeInst> VCode<I> {
             }
             assert_eq!(buffer.cur_offset(), new_offset);
 
+            if Some(block) == first_cold_block {
+                start_of_cold_code = Some(buffer.cur_offset());
+            }
+
             let (start, end) = self.block_ranges[block as usize];
             buffer.bind_label(MachLabel::from_block(block));
-            label_insn_iix[block as usize] = start;
+            label_inst_indices[block as usize] = start;
 
             if cfg_metadata {
                 // Track BB starts. If we have backed up due to MachBuffer
@@ -545,7 +559,7 @@ impl<I: VCodeInst> VCode<I> {
                 if self.generate_debug_info {
                     // Buffer truncation may have happened since last inst append; trim inst-end
                     // layout info as appropriate.
-                    let l = &mut inst_ends[0..iix as usize];
+                    let l = &mut inst_end_offsets[0..iix as usize];
                     for end in l.iter_mut().rev() {
                         if *end > buffer.cur_offset() {
                             *end = buffer.cur_offset();
@@ -553,7 +567,7 @@ impl<I: VCodeInst> VCode<I> {
                             break;
                         }
                     }
-                    inst_ends[iix as usize] = buffer.cur_offset();
+                    inst_end_offsets[iix as usize] = buffer.cur_offset();
                 }
             }
 
@@ -582,14 +596,18 @@ impl<I: VCodeInst> VCode<I> {
         }
 
         if self.generate_debug_info {
-            for end in inst_ends.iter_mut().rev() {
+            for end in inst_end_offsets.iter_mut().rev() {
                 if *end > buffer.cur_offset() {
                     *end = buffer.cur_offset();
                 } else {
                     break;
                 }
             }
-            *self.insts_layout.borrow_mut() = (inst_ends, label_insn_iix, buffer.cur_offset());
+            *self.insts_layout.borrow_mut() = InstsLayoutInfo {
+                inst_end_offsets,
+                label_inst_indices,
+                start_of_cold_code,
+            };
         }
 
         // Create `bb_edges` and final (filtered) `bb_starts`.
@@ -622,8 +640,8 @@ impl<I: VCodeInst> VCode<I> {
             return ValueLabelsRanges::default();
         }
 
-        let layout = &self.insts_layout.borrow();
+        let layout_info = &self.insts_layout.borrow();
-        debug::compute(&self.insts, &layout.0[..], &layout.1[..])
+        debug::compute(&self.insts, &*layout_info)
     }
 
     /// Get the offsets of stackslots.