Debugging: allow breakpoints to be set at "function start" by slipping forward to first opcode. (#12791)

LLDB, when instructed to `break main`, looks at the DWARF metadata for
`main` and finds its PC range, then sets a breakpoint at the first
PC. This is reasonable behavior for native ISAs! That PC better be a
real instruction!

On Wasm, however, (i) toolchains typically emit the PC range as
*including* the *locals count*, a leb128 value that precedes the first
opcode and any types of locals; (ii) our gdbstub component that
bridges LLDB to our debug APIs (#12771) only supports *exact* PCs for
breakpoints, so when presented with a PC that does not actually point
to an opcode, setting the breakpoint is effectively a no-op. There
will always be a difference of at least 1 byte between the
start-of-function offset and first-opcode offset (for a leb128 of `0`
for no locals), so a breakpoint "on" a function will never work.

I initially prototyped a fix that adds a sequence point at the start
of every function (which, again, is *guaranteed* to be distinct from
the first opcode), and the branch is [here], but I didn't like the
developer experience: this meant that when a breakpoint at a function
start fired, LLDB had a weird interstitial state where no line-number
applied.

The behavior that would be closer in line with "native" debug
expectations is that we add a bit of fuzzy-ish matching: setting a
breakpoint at function start should break at the first opcode, even if
that's a few (or many) bytes later. There are two options here:
special-case function start, or generally change the semantics of our
breakpoint API so that "add breakpoint at `pc`" means "add breakpoint
at next opcode at or after `pc`". I opted for the latter in this PR
because it's more consistent.

The logic is a little subtle because we're effectively defining an
n-to-1 mapping with this "snap-to-next" behavior, so we have to
refcount each breakpoint (consider setting a breakpoint at function
start *and* at the first opcode, then deleting them, one at a time). I
believe the result is self-consistent, even if a little more
complicated now. And, importantly, with #12771 on top of this change,
it produces the expected behavior for the (very simple!) debug script
"`b main`; `continue`".

[here]: https://github.com/cfallin/wasmtime/tree/breakpoint-at-func-start

Author: cfallin

crates/environ/src/frame_table.rs

@@ -297,6 +297,23 @@ impl<'a> FrameTable<'a> {
         range.map(|i| self.breakpoint_patch(i))
     }
 
+    /// Find the nearest breakpoint PC at or after the given PC.
+    pub fn nearest_breakpoint(&self, pc: u32) -> Option<u32> {
+        match self
+            .breakpoint_pcs
+            .binary_search_by_key(&pc, |p| p.get(LittleEndian))
+        {
+            Ok(_) => Some(pc),
+            Err(i) => {
+                if i < self.breakpoint_pcs.len() {
+                    Some(self.breakpoint_pcs[i].get(LittleEndian))
+                } else {
+                    None
+                }
+            }
+        }
+    }
+
     /// Return an iterator over all breakpoint patches.
     ///
     /// Returned tuples are (Wasm PC, breakpoint data).

crates/wasmtime/src/runtime/debug.rs

@@ -943,8 +943,16 @@ pub trait DebugHandler: Clone + Send + Sync + 'static {
 pub(crate) struct BreakpointState {
     /// Single-step mode.
     single_step: bool,
-    /// Breakpoints added individually.
-    breakpoints: BTreeSet<BreakpointKey>,
+    /// Breakpoints added individually. Maps from the actual
+    /// (possibly slipped-forward) breakpoint key to a reference
+    /// count. Multiple requested PCs may map to the same actual
+    /// breakpoint when they are slipped forward.
+    breakpoints: BTreeMap<BreakpointKey, usize>,
+    /// When a requested breakpoint PC does not exactly match an
+    /// opcode boundary, we "slip" it forward to the next available
+    /// PC. This map records the redirect from the requested key to
+    /// the actual key so that `remove_breakpoint` can undo it.
+    breakpoint_redirects: BTreeMap<BreakpointKey, BreakpointKey>,
 }
 
 /// A breakpoint.
@@ -1003,7 +1011,7 @@ impl BreakpointState {
         &'a self,
         registry: &'a ModuleRegistry,
     ) -> impl Iterator<Item = Breakpoint> + 'a {
-        self.breakpoints.iter().map(|key| key.get(registry))
+        self.breakpoints.keys().map(|key| key.get(registry))
     }
 
     pub(crate) fn is_single_step(&self) -> bool {
@@ -1068,18 +1076,36 @@ impl<'a> BreakpointEdit<'a> {
     /// Add a breakpoint in the given module at the given PC in that
     /// module.
     ///
+    /// If the requested PC does not fall exactly on an opcode
+    /// boundary, the breakpoint is "slipped" forward to the next
+    /// available opcode PC.
+    ///
     /// No effect if the breakpoint is already set.
     pub fn add_breakpoint(&mut self, module: &Module, pc: u32) -> Result<()> {
-        let key = BreakpointKey::from_raw(module, pc);
-        self.state.breakpoints.insert(key);
-        log::trace!("patching in breakpoint {key:?}");
-        let mem =
-            Self::get_code_memory(self.state, self.registry, &mut self.dirty_modules, module)?;
         let frame_table = module
             .frame_table()
             .expect("Frame table must be present when guest-debug is enabled");
-        let patches = frame_table.lookup_breakpoint_patches_by_pc(pc);
-        Self::patch(patches, mem, true);
+        let actual_pc = frame_table.nearest_breakpoint(pc).unwrap_or(pc);
+        let requested_key = BreakpointKey::from_raw(module, pc);
+        let actual_key = BreakpointKey::from_raw(module, actual_pc);
+
+        if actual_pc != pc {
+            log::trace!("slipping breakpoint from {requested_key:?} to {actual_key:?}");
+            self.state
+                .breakpoint_redirects
+                .insert(requested_key, actual_key);
+        }
+
+        let refcount = self.state.breakpoints.entry(actual_key).or_insert(0);
+        *refcount += 1;
+        if *refcount == 1 {
+            // First reference: actually patch the code.
+            log::trace!("patching in breakpoint {actual_key:?}");
+            let mem =
+                Self::get_code_memory(self.state, self.registry, &mut self.dirty_modules, module)?;
+            let patches = frame_table.lookup_breakpoint_patches_by_pc(actual_pc);
+            Self::patch(patches, mem, true);
+        }
         Ok(())
     }
 
@@ -1088,16 +1114,32 @@ impl<'a> BreakpointEdit<'a> {
     ///
     /// No effect if the breakpoint was not set.
     pub fn remove_breakpoint(&mut self, module: &Module, pc: u32) -> Result<()> {
-        let key = BreakpointKey::from_raw(module, pc);
-        self.state.breakpoints.remove(&key);
-        if !self.state.single_step {
-            let mem =
-                Self::get_code_memory(self.state, self.registry, &mut self.dirty_modules, module)?;
-            let frame_table = module
-                .frame_table()
-                .expect("Frame table must be present when guest-debug is enabled");
-            let patches = frame_table.lookup_breakpoint_patches_by_pc(pc);
-            Self::patch(patches, mem, false);
+        let requested_key = BreakpointKey::from_raw(module, pc);
+        let actual_key = self
+            .state
+            .breakpoint_redirects
+            .remove(&requested_key)
+            .unwrap_or(requested_key);
+        let actual_pc = actual_key.1;
+
+        if let Some(refcount) = self.state.breakpoints.get_mut(&actual_key) {
+            *refcount -= 1;
+            if *refcount == 0 {
+                self.state.breakpoints.remove(&actual_key);
+                if !self.state.single_step {
+                    let mem = Self::get_code_memory(
+                        self.state,
+                        self.registry,
+                        &mut self.dirty_modules,
+                        module,
+                    )?;
+                    let frame_table = module
+                        .frame_table()
+                        .expect("Frame table must be present when guest-debug is enabled");
+                    let patches = frame_table.lookup_breakpoint_patches_by_pc(actual_pc);
+                    Self::patch(patches, mem, false);
+                }
+            }
         }
         Ok(())
     }
@@ -1141,7 +1183,7 @@ impl<'a> BreakpointEdit<'a> {
             let mem =
                 Self::get_code_memory(self.state, self.registry, &mut self.dirty_modules, &module)?;
             Self::apply_single_step(mem, &module, enabled, |key| {
-                self.state.breakpoints.contains(key)
+                self.state.breakpoints.contains_key(key)
             })?;
         }
 

tests/all/debug.rs

@@ -1420,3 +1420,86 @@ async fn early_epoch_yield_still_has_vmctx() -> wasmtime::Result<()> {
 
     Ok(())
 }
+
+#[tokio::test]
+#[cfg_attr(miri, ignore)]
+async fn breakpoint_slips_to_first_opcode() -> wasmtime::Result<()> {
+    let _ = env_logger::try_init();
+
+    // Breakpoints set at the function body start (which includes the
+    // local declarations and precedes the first opcode) should be
+    // "slipped" forward to the first opcode. This matches how LLDB
+    // sets breakpoints using DWARF `DW_AT_low_pc`.
+    //
+    // For the WAT below, `wasm-objdump -d` shows:
+    //
+    // ```
+    // 000023 func[0] <main>:
+    //  000024: 20 00                      | local.get 0
+    //  000026: 20 01                      | local.get 1
+    //  000028: 6a                         | i32.add
+    //  000029: 0b                         | end
+    // ```
+    //
+    // 0x23 is the function body start (locals count byte), while 0x24
+    // is the first opcode. Setting a breakpoint at 0x23 should slip
+    // to 0x24.
+    let (module, mut store) = get_module_and_store(
+        |_config| {},
+        r#"
+    (module
+      (func (export "main") (param i32 i32) (result i32)
+        local.get 0
+        local.get 1
+        i32.add))
+    "#,
+    )?;
+
+    debug_event_checker!(
+        D, store,
+        { 0 ;
+          wasmtime::DebugEvent::Breakpoint => {
+              let stack = store.debug_exit_frames().next().unwrap();
+              let (func, pc) = stack.wasm_function_index_and_pc(&mut store).unwrap().unwrap();
+              assert_eq!(func.as_u32(), 0);
+              // The breakpoint should fire at the first opcode
+              // (0x24), not at the function body start (0x23).
+              assert_eq!(pc, 0x24);
+          }
+        }
+    );
+
+    let (handler, counter) = D::new_and_counter();
+    store.set_debug_handler(handler);
+
+    store
+        .edit_breakpoints()
+        .unwrap()
+        .add_breakpoint(&module, 0x23)?;
+
+    let instance = Instance::new_async(&mut store, &module, &[]).await?;
+    let func = instance.get_func(&mut store, "main").unwrap();
+    let mut results = [Val::I32(0)];
+    func.call_async(&mut store, &[Val::I32(1), Val::I32(2)], &mut results)
+        .await?;
+    assert_eq!(counter.load(Ordering::Relaxed), 1);
+    assert_eq!(results[0].unwrap_i32(), 3);
+
+    // The actual breakpoint stored should be at the slipped PC.
+    let breakpoints = store.breakpoints().unwrap().collect::<Vec<_>>();
+    assert_eq!(breakpoints.len(), 1);
+    assert_eq!(breakpoints[0].pc, 0x24);
+
+    // Removing with the originally requested PC should work.
+    store
+        .edit_breakpoints()
+        .unwrap()
+        .remove_breakpoint(&module, 0x23)?;
+    func.call_async(&mut store, &[Val::I32(1), Val::I32(2)], &mut results)
+        .await?;
+    // Counter should not have incremented now that we removed the
+    // breakpoint.
+    assert_eq!(counter.load(Ordering::Relaxed), 1);
+
+    Ok(())
+}