Vibe Coding an API Refactor

In my last post I used ownership to drive the API design for multiple breakpoints, I ended up with an API for the inferiors that I wasn’t entirely happy with. I wrote

Now, I say it’s clean meaning it compiled without any warnings. But I don’t really like the API, it doesn’t feel clean to me. For you rustaceans that have a better knowledge of idiomatic Rust than I do, what’s the right way to create APIs like trap_inferior_exec? I’m dissatisfied with the way it is now because it forces the user to have a mutable Inferior just so that it can pass it back into other rusty_trap functions. The user doesn’t really want to know what’s in there or modify anything. They would depend on rusty_trap to do that for them. So I’d like to return at the very least an immutable object. I guess it’s mutable because they expect rusty_trap to mutate it. Let me know in the comments what you think about this or if you have an suggestions for a cleaner API.

When I had GitHub Copilot with the Claude-3.7 Sonnet Thought model proofread the post it answered this question and suggested:

An opaque handle type that internally manages mutability
A builder pattern for configuration
A more functional approach where operations return new states

I dug in a little deeper and asked

A functional approach is a nice idea but does this imply copying the data in and out of the rusty_trap library? Or do the API calls take ownership and the return the same object back to the caller?

and I think this version of a “functional approach” is a beautiful one, and it aligns well with Rust’s idioms:

let inferior = trap_inferior_exec(...)?;
let inferior = trap_inferior_set_breakpoint(inferior, ADDRESS);
let (inferior, exit_code) = trap_inferior_continue(inferior, callback);

Given that this was Claude’s suggestion I’m going to let it write the code and see how it handles the implementation.

For reference I’m starting at commit e972885.

Me: Initial Ask

I would like to implement the API you suggested

let inferior = trap_inferior_exec(...)?;
let inferior = trap_inferior_set_breakpoint(inferior, ADDRESS);
   let (inferior, exit_code) = trap_inferior_continue(inferior, callback);

Hmm, does trap_inferior_continue need to return the inferior. By the time it has returned the inferior has exited.

Claude: On Returning the Inferior from `trap_inferior_continue`

You raise an excellent question about whether trap_inferior_continue should return the inferior when it has already exited. This depends on your design goals:

Reasons to Not Return the Inferior

The process has terminated, so the primary purpose of the Inferior is complete
It signals more clearly to the user that the debugging session is finished
It’s more memory efficient as Rust will drop the value

fn trap_inferior_continue<F>(inferior: Inferior, callback: F) -> i32
where
    F: FnMut(&Inferior, TrapBreakpoint)
{
    // Implementation that consumes inferior
    exit_code
}

// Usage
let inferior = trap_inferior_exec(path, &[])?;
let inferior = trap_inferior_set_breakpoint(inferior, ADDRESS);
let exit_code = trap_inferior_continue(inferior, callback); // inferior consumed

Reasons to Return the Inferior

Consistency: All API functions follow the same ownership pattern
Post-mortem analysis: The Inferior might contain valuable data even after exit
Future extension: You might add features that work with completed inferiors
Multiple inferiors: In a multi-process debugger, one inferior might exit while others continue

// If you return the inferior:
impl Inferior {
    fn continue_execution<F>(mut self, callback: F) -> (Self, i32)
    where
        F: FnMut(&Self, TrapBreakpoint)
    {
        // Implementation
        self.state = InferiorState::Exited;
        (self, exit_code)
    }
    
    // Then you can add methods that work on exited inferiors
    fn get_memory_dump(&self) -> Option<MemoryDump> {
        if self.state == InferiorState::Exited {
            // Get memory dump from saved state
        } else {
            None
        }
    }
}

Recommended Approach

I’d suggest adding an explicit state for exited inferiors and returning the inferior for consistency. This gives you the most flexibility:

// Add to your InferiorState enum
pub enum InferiorState {
    Running,
    Stopped,
    SingleStepping,
    Exited(i32), // Store the exit code
}

Then users can check the state to determine what operations are valid:

let (inferior, _) = inferior.continue_execution(callback);
match inferior.state {
    InferiorState::Exited(code) => println!("Process exited with code {}", code),
    _ => inferior.continue_execution(callback), // Continue if not exited
}

This approach maintains consistency while making the lifecycle explicit.

An aside: My Thoughts returning the inferior from `trap_inferior_continue`

I think by the time trap_inferior_continue returns the inferior no longer exists so it’s not possible to dump memory or inspect anything. So I’m not going to implement this.

Let’s just move forward with the inferior ownership API.

Me: Let’s move on

Ok, let’s leave the return value as is for now. Can you implement the changes to pass the inferior and ownership into rusty_trap and then return it back to the user via the return value?

Claude: Implementing Ownership-Based Debugger API

Here’s the implementation of the ownership-passing API pattern we discussed. I’ll modify the key functions to take ownership of the inferior and return it back to the user.

Updated `trap_inferior_set_breakpoint` Function

First, let’s modify the breakpoint setting function to take ownership and return a tuple:

pub fn trap_inferior_set_breakpoint(mut inferior: TrapInferior, location: u64) -> (TrapInferior, TrapBreakpoint) {
    let aligned_address = location & !0x7u64;
    let target_address = InferiorPointer(location);
    inferior.breakpoints.insert(
        target_address,
        Breakpoint {
            shift: (location - aligned_address) * 8,
            aligned_address: InferiorPointer(aligned_address),
            target_address,
            original_breakpoint_word: peek_text(inferior.pid, InferiorPointer(aligned_address)),
        },
    );

    set(&inferior, inferior.breakpoints.get(&target_address).unwrap());

    (inferior, InferiorPointer(location))
}

Updated `trap_inferior_continue` Function

Next, the continue function follows the same pattern:

pub fn trap_inferior_continue<F>(mut inferior: TrapInferior, mut callback: F) -> (TrapInferior, i32)
where
    F: FnMut(&TrapInferior, TrapBreakpoint),
{
    inferior.state = InferiorState::Running;
    ptrace_util::cont(inferior.pid);
    loop {
        inferior.state = match waitpid(Pid::from_raw(inferior.pid), None) {
            Ok(WaitStatus::Exited(_pid, code)) => return (inferior, code),
            Ok(WaitStatus::Stopped(_pid, signal::SIGTRAP)) => {
                breakpoint::handle(&mut inferior, &mut callback)
            }
            Ok(WaitStatus::Stopped(_pid, signal)) => {
                panic!(
                    "Unexpected stop on signal {} in trap_inferior_continue.  State: {}",
                    signal, inferior.state as i32
                )
            }
            Ok(_) => panic!("Unexpected stop in trap_inferior_continue"),
            Err(_) => panic!("Unhandled error in trap_inferior_continue"),
        };
    }
}

Updating the `breakpoint::handle` Function

We need to modify this function to borrow the inferior instead of taking ownership:

pub fn handle<F>(inferior: &mut Inferior, callback: &mut F) -> InferiorState
where
    F: FnMut(&TrapInferior, TrapBreakpoint),
{
    let bp = find_breakpoint_matching_inferior_instruction_pointer(inferior)
        .expect("Could not find breakpoint");

    match inferior.state {
        InferiorState::Running => (),
        _ => panic!("Unhandled error in breakpoint::handle"),
    }
    callback(inferior, bp.target_address);
    step_over(inferior, bp);
    return match waitpid(Pid::from_raw(inferior.pid), None) {
        Ok(WaitStatus::Stopped(_pid, signal::SIGTRAP)) => {
            set(inferior, bp);
            cont(inferior.pid);
            InferiorState::Running
        }
        Ok(WaitStatus::Exited(_pid, _code)) => InferiorState::Running,
        Ok(WaitStatus::Stopped(_pid, signal)) => {
            panic!(
                "Unexpected stop on signal {} in breakpoint::handle.  State: {}",
                signal, inferior.state as i32
            )
        }
        Ok(_) => panic!("Unexpected stop in breakpoint::handle"),
        Err(_) => panic!("Unhandled error in breakpoint::handle"),
    };
}

Updated Test Example

Here’s how the tests would change to use the new API:

#[test]
fn it_can_set_breakpoints() {
    let mut breakpoint_count: i32 = 0;

    let inferior = rusty_trap::trap_inferior_exec(Path::new("./target/debug/twelve"), &[]).unwrap();
    let expected_pid = inferior.pid;
    let (inferior, bp) = rusty_trap::trap_inferior_set_breakpoint(inferior, 0x000055555555b821);
    let (_, _) = rusty_trap::trap_inferior_continue(inferior, |passed_inferior, passed_bp| {
        assert_eq!(passed_inferior.pid, expected_pid);
        assert_eq!(passed_bp, bp);
        breakpoint_count += 1;
    });

    assert_eq!(breakpoint_count, 1);
}

Multiple Breakpoints Test

Here’s the updated multiple breakpoints test:

#[test]
fn it_can_handle_more_than_one_breakpoint() {
    let mut bp_main_count: i32 = 0;
    let mut bp_foo_count: i32 = 0;

    let inferior = rusty_trap::trap_inferior_exec(Path::new("./target/debug/loop"), &[]).unwrap();
    let expected_pid = inferior.pid;
    let (inferior, bp_main) = rusty_trap::trap_inferior_set_breakpoint(inferior, ADDRESS_OF_MAIN);
    let (inferior, bp_foo) = rusty_trap::trap_inferior_set_breakpoint(inferior, ADDRESS_OF_FOO);
    let (_, _) = rusty_trap::trap_inferior_continue(inferior, |passed_inferior, passed_bp| {
        assert_eq!(passed_inferior.pid, expected_pid);
        if passed_bp == bp_main {
            bp_main_count += 1;
        } else if passed_bp == bp_foo {
            bp_foo_count += 1;
        } else {
            panic!(
                "Unexpected breakpoint {} encountered.  Expected {} or {}",
                passed_bp, bp_main, bp_foo
            );
        }
    });

    assert_eq!(bp_main_count, 1);
    assert_eq!(bp_foo_count, 5);
}

This API pattern provides several benefits:

Makes ownership transfers explicit
Eliminates mutable references to shared state
Creates a more fluent and natural API
Enables method chaining in the future (with method implementations)
Makes it harder to use the API incorrectly

The main downside is that we need to reassign the inferior at each step, but this clearly communicates the ownership transfer semantics, which is a good thing in Rust.

Aside: Review

That looks pretty good. I think I had mentioned to Claude earlier (before this post) that I don’t think trap_inferor_set_breakpoint should return the breakpoint anymore. This was in the example I gave Claude up front. The reason this is unnecessary is that the user now just uses the address as the handle of the breakpoint. But maybe that would be weird actually, trap_inferor_set_breakpoint shouldn’t only return and inferior as that would be confusing. I’d think it just returns a breakpoint as that’s what it did before.

There were two other tests that Claude left for me as an exercises. I could ask it to fix them but it doesn’t seem worth the time. But that’s one thing I find is that often the AI leaves me a lot of the work. I thought the point was for it to do work for me. It often seems more like it’s some Senior engineer helping me out and leaving me to do the grunt work…

I plugged everything in and ran the tests and they pass:

    Running tests/lib.rs (/home/jkain/projects/rusty_trap/target/debug/deps/lib-a0fc739a67a23e62)

running 4 tests
test it_can_exec ... ok
test it_can_set_breakpoints ... ok
test it_can_handle_a_breakpoint_more_than_once ... ok
test it_can_handle_more_than_one_breakpoint ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

And that’s pretty nice. Claude gave us a good suggestion and was able to implement it for us. Here’s it’s changes as a diff for reference:

modified   src/breakpoint/mod.rs
@@ -68,7 +68,7 @@ where
     };
 }
 
-pub fn trap_inferior_set_breakpoint(inferior: &mut TrapInferior, location: u64) -> TrapBreakpoint {
+pub fn trap_inferior_set_breakpoint(mut inferior: TrapInferior, location: u64) -> (TrapInferior, TrapBreakpoint) {
     let aligned_address = location & !0x7u64;
     let target_address = InferiorPointer(location);
     inferior.breakpoints.insert(
@@ -81,7 +81,7 @@ pub fn trap_inferior_set_breakpoint(inferior: &mut TrapInferior, location: u64)
         },
     );
 
-    set(inferior, inferior.breakpoints.get(&target_address).unwrap());
+    set(&inferior, inferior.breakpoints.get(&target_address).unwrap());
 
-    InferiorPointer(location)
+    (inferior, InferiorPointer(location))
 }
modified   src/lib.rs
@@ -68,7 +68,7 @@ pub fn trap_inferior_exec(filename: &Path, args: &[&str]) -> Result<Inferior, Er
     }
 }
 
-pub fn trap_inferior_continue<F>(inferior: &mut TrapInferior, callback: &mut F) -> i32
+pub fn trap_inferior_continue<F>(mut inferior: TrapInferior, mut callback: F) -> (TrapInferior, i32)
 where
     F: FnMut(&TrapInferior, TrapBreakpoint),
 {
@@ -76,9 +76,9 @@ where
     ptrace_util::cont(inferior.pid);
     loop {
         inferior.state = match waitpid(Pid::from_raw(inferior.pid), None) {
-            Ok(WaitStatus::Exited(_pid, code)) => return code,
+            Ok(WaitStatus::Exited(_pid, code)) => return (inferior, code),
             Ok(WaitStatus::Stopped(_pid, signal::SIGTRAP)) => {
-                breakpoint::handle(inferior, callback)
+                breakpoint::handle(&mut inferior, &mut callback)
             }
             Ok(WaitStatus::Stopped(_pid, signal)) => {
                 panic!(
modified   tests/lib.rs
@@ -6,23 +6,19 @@ const ADDRESS_OF_FOO: u64 = 0x55555555b9e0;
 
 #[test]
 fn it_can_exec() {
-    let mut inferior =
-        rusty_trap::trap_inferior_exec(Path::new("./target/debug/twelve"), &[]).unwrap();
-    assert_eq!(
-        12,
-        rusty_trap::trap_inferior_continue(&mut inferior, &mut |_, _| {})
-    );
+    let inferior = rusty_trap::trap_inferior_exec(Path::new("./target/debug/twelve"), &[]).unwrap();
+    let (_inferior, exit_code) = rusty_trap::trap_inferior_continue(inferior, |_, _| {});
+    assert_eq!(12, exit_code);
 }
 
 #[test]
 fn it_can_set_breakpoints() {
     let mut breakpoint_count: i32 = 0;
 
-    let mut inferior =
-        rusty_trap::trap_inferior_exec(Path::new("./target/debug/twelve"), &[]).unwrap();
+    let inferior = rusty_trap::trap_inferior_exec(Path::new("./target/debug/twelve"), &[]).unwrap();
     let expected_pid = inferior.pid;
-    let bp = rusty_trap::trap_inferior_set_breakpoint(&mut inferior, 0x000055555555b821);
-    rusty_trap::trap_inferior_continue(&mut inferior, &mut |passed_inferior, passed_bp| {
+    let (inferior, bp) = rusty_trap::trap_inferior_set_breakpoint(inferior, 0x000055555555b821);
+    let (_, _) = rusty_trap::trap_inferior_continue(inferior, |passed_inferior, passed_bp| {
         assert_eq!(passed_inferior.pid, expected_pid);
         assert_eq!(passed_bp, bp);
         breakpoint_count += 1;
@@ -35,11 +31,10 @@ fn it_can_set_breakpoints() {
 fn it_can_handle_a_breakpoint_more_than_once() {
     let mut breakpoint_count: i32 = 0;
 
-    let mut inferior =
-        rusty_trap::trap_inferior_exec(Path::new("./target/debug/loop"), &[]).unwrap();
+    let inferior = rusty_trap::trap_inferior_exec(Path::new("./target/debug/loop"), &[]).unwrap();
     let expected_pid = inferior.pid;
-    let bp = rusty_trap::trap_inferior_set_breakpoint(&mut inferior, ADDRESS_OF_FOO);
-    rusty_trap::trap_inferior_continue(&mut inferior, &mut |passed_inferior, passed_bp| {
+    let (inferior, bp) = rusty_trap::trap_inferior_set_breakpoint(inferior, ADDRESS_OF_FOO);
+    rusty_trap::trap_inferior_continue(inferior, |passed_inferior, passed_bp| {
         assert_eq!(passed_inferior.pid, expected_pid);
         assert_eq!(passed_bp, bp);
         breakpoint_count += 1;
@@ -53,12 +48,11 @@ fn it_can_handle_more_than_one_breakpoint() {
     let mut bp_main_count: i32 = 0;
     let mut bp_foo_count: i32 = 0;
 
-    let mut inferior =
-        rusty_trap::trap_inferior_exec(Path::new("./target/debug/loop"), &[]).unwrap();
+    let inferior = rusty_trap::trap_inferior_exec(Path::new("./target/debug/loop"), &[]).unwrap();
     let expected_pid = inferior.pid;
-    let bp_main = rusty_trap::trap_inferior_set_breakpoint(&mut inferior, ADDRESS_OF_MAIN);
-    let bp_foo = rusty_trap::trap_inferior_set_breakpoint(&mut inferior, ADDRESS_OF_FOO);
-    rusty_trap::trap_inferior_continue(&mut inferior, &mut |passed_inferior, passed_bp| {
+    let (inferior, bp_main) = rusty_trap::trap_inferior_set_breakpoint(inferior, ADDRESS_OF_MAIN);
+    let (inferior, bp_foo) = rusty_trap::trap_inferior_set_breakpoint(inferior, ADDRESS_OF_FOO);
+    let (_, _) = rusty_trap::trap_inferior_continue(inferior, |passed_inferior, passed_bp| {
         assert_eq!(passed_inferior.pid, expected_pid);
         if passed_bp == bp_main {
             bp_main_count += 1;

Takeaways

I guess I’m taking two things away from this experience:

I should remember this Rust idiom of taking ownership of a passed argument and returning it give the object back to the caller. That’s a clean way of managing the ownership.
I can just vibe code this whole thing.

I joke about vibe coding the entire project but I have been using AI tools in my job a lot lately and I find they are pretty powerful. They may need to be pointed in the right direction and their work should be reviewed but they often surprise me both in terms of the code they write and also in terms of the insight they can provide. I plan to write more about AI tools in the future.

Just to close this out I’ve pushed out a PR:

This post is part of my Writing a Debugger series.
View all posts and upcoming topics →