In the last post we implemented a state machine to manage breakpoints in rusty_trap. We also added support for hitting a single breakpoint multiple times. Now, I want to start adding support for multiple distinct breakpoints.

One way to approach adding a new feature is to refactor to make the code easily accept the new feature. That’s what I set out to do today, but it didn’t go the way I planned.

Multiple Breakpoints

I still start with writing a test for my goal - to support multiple breakpoints. This will help me identify the refactorings I need:

#[test]
fn it_can_handle_multiple_breakpoints () {
    let mut foo_count:  i32 = 0;
    let mut main_count: i32 = 0;

    let inferior = rusty_trap::trap_inferior_exec(Path::new("./target/debug/loop"), &[])
        .unwrap();
    let bp_foo  = rusty_trap::trap_inferior_set_breakpoint(inferior, 0x5555555587b0);
    let bp_main = rusty_trap::trap_inferior_set_breakpoint(inferior, 0x555555558bd0);
    rusty_trap::trap_inferior_continue(inferior, &mut |passed_inferior, bp_passed| {
        assert_eq!(passed_inferior, inferior);
        if bp_passed == bp_foo {
            foo_count  += 1
        } else if bp_passed == bp_main {
             main_count += 1
        } else {
            panic!("Unknown breakpoint {}", bp_passed)
        }
    });

    assert_eq!(foo_count, 5);
    assert_eq!(main_count, 5)
}

and of course this test fails which is expected. Though the error isn’t exactly what I expected:

---- it_can_handle_multiple_breakpoints stdout ----
	thread 'it_can_handle_multiple_breakpoints' panicked at 'assertion failed: `(left == right)` (left: `2`, right: `5`)', tests/lib.rs:63

So it looks like we only detected 2 two hits to bp_foo. I would have expected to get all 5 an maybe more (due to misattribution of bp_main).

I also see this

test it_can_handle_multiple_breakpoints ... thread '<main>' panicked at 'assertion failed: c.borrow().is_none()', ../src/libstd/sys/common/thread_info.rs:54
FAILED

I’m not sure exactly what this is about or when it happened. The test must have gotten at least two assert_eq!(foo_count, 5);

Implementation

As I mentioned, the test should point me in the direction I need to refactor. So, I need to consider: hat do we need to do in order to implement this feature?

Looking over breakpoint/mod.rs I think we need to

  • Remove global_breakpoint in favor of a vector of breakpoints
  • Update breakpoint::handle to lookup the correct breakpoint
  • Fix trap_inferior_set_breakpoint to add to the vector of breakpoints

Also, instead of having a global vector of breakpoints. I’ll keep the vector in the Inferior. This will help when we need to support multiple inferiors. One problem is that this will create a circular dependency between inferior and breakpoint modules.

I need some private structure for breakpoint module that fits into Inferior. How do I do this in Rust? Maybe I can use a generic that lets me store/fetch it in a Vector. A little googling turns up that I can use the Any trait.

If I want to put a Vector inside Inferior then I will need to remove the Copy trait from Inferior. I don’t want to to be copying the Vectors. Given this I should remove global_inferior.

Remove global_inferior

I’ll do this by dynamically allocating an inferior and returning it to the application. This eliminates the need for Copy trait on Inferior.

I start by commenting out global_inferior and letting the compiler tell me what to fix:

Compiling rusty_trap v0.1.0 (file:///home/joseph/src/rust/rusty_trap)
src/lib.rs:79:26: 79:41 error: unresolved name `global_inferior` [E0425]
src/lib.rs:79                 unsafe { global_inferior = attach_inferior(pid).ok().unwrap() };
                                    ^~~~~~~~~~~~~~~
src/lib.rs:79:26: 79:41 help: run `rustc --explain E0425` to see a detailed explanation
src/lib.rs:91:28: 91:43 error: unresolved name `global_inferior` [E0425]
src/lib.rs:91     let mut inf = unsafe { global_inferior };
                                      ^~~~~~~~~~~~~~~
src/lib.rs:91:28: 91:43 help: run `rustc --explain E0425` to see a detailed explanation
src/lib.rs:105:18: 105:33 error: unresolved name `global_inferior` [E0425]
src/lib.rs:105         unsafe { global_inferior = inf };
                             ^~~~~~~~~~~~~~~
src/lib.rs:105:18: 105:33 help: run `rustc --explain E0425` to see a detailed explanation
error: aborting due to 3 previous errors
Could not compile `rusty_trap`.

Changes to inferior/mod.rs:

diff --git a/src/inferior/mod.rs b/src/inferior/mod.rs
index 17eaa77..e5eaa55 100644
--- a/src/inferior/mod.rs
+++ b/src/inferior/mod.rs
@@ -1,6 +1,9 @@
 use libc::pid_t;
 use libc::c_void;
 use std::ops::{Add, Sub};
+use std::any::Any;
+use std::vec::Vec;

 #[derive(Copy, Clone)]
 pub enum InferiorState {
@@ -9,13 +12,14 @@ pub enum InferiorState {
     SingleStepping
 }

-#[derive(Copy, Clone)]
+//#[derive(Copy, Clone)]
 pub struct Inferior {
     pub pid: pid_t,
-    pub state: InferiorState
+    pub state: InferiorState,
+    pub privates: Vec<Box<Any>>
 }

-pub type TrapInferior = pid_t;
+pub type TrapInferior = Box<Inferior>;

 #[derive(Copy, Clone)]
 pub struct InferiorPointer(pub u64);

Right now, the only important changes is the change to TrapInferior. The changes to Inferior are still work in progress.

Changes to lib.rs:

diff --git a/src/lib.rs b/src/lib.rs
index ac9e444..70ded88 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -29,13 +29,13 @@ struct Breakpoint {
     original_breakpoint_word : i64
 }

-static mut global_breakpoint : Breakpoint = Breakpoint {
-    shift: 0,
-    target_address: InferiorPointer(0),
-    aligned_address: InferiorPointer(0),
-    original_breakpoint_word: 0
-};
-static mut global_inferior : Inferior = Inferior { pid: 0, state: InferiorState::Stopped };
+// static mut global_breakpoint : Breakpoint = Breakpoint {
+//     shift: 0,
+//     target_address: InferiorPointer(0),
+//     aligned_address: InferiorPointer(0),
+//     original_breakpoint_word: 0
+// };
+//static mut global_inferior : Inferior = Inferior { pid: 0, state: InferiorState::Stopped, privates: vec![] };

 mod ffi {
     use libc::{c_int, c_long};
@@ -62,10 +62,10 @@ fn exec_inferior(filename: &Path, args: &[&str]) -> () {
     unreachable!();
 }

-fn attach_inferior(pid: pid_t) -> Result<Inferior, Error> {
+fn attach_inferior(pid: pid_t) -> Result<Box<Inferior>, Error> {
     match waitpid(pid, None) {
         Ok(WaitStatus::Stopped(pid, signal::SIGTRAP)) =>
-            return Ok(Inferior {pid: pid, state: InferiorState::Running}),
+            return Ok(Box::new(Inferior {pid: pid, state: InferiorState::Running, privates: vec![]})),
         Ok(_) => panic!("Unexpected stop in attach_inferior"),
         Err(e) => return Err(e)
     }
@@ -76,8 +76,8 @@ pub fn trap_inferior_exec(filename: &Path, args: &[&str]) -> Result<TrapInferior
         match fork() {
             Ok(Child)                      => exec_inferior(filename, args),
             Ok(Parent(pid))                => {
-                unsafe { global_inferior = attach_inferior(pid).ok().unwrap() };
-                return Ok(pid)
+                let inf = attach_inferior(pid).ok().unwrap();
+                return Ok(inf)
             },
             Err(Error::Sys(errno::EAGAIN)) => continue,
             Err(e)                         => return Err(e)
@@ -86,22 +86,20 @@ pub fn trap_inferior_exec(filename: &Path, args: &[&str]) -> Result<TrapInferior
 }

 pub fn trap_inferior_continue<F>(inferior: TrapInferior, callback: &mut F) -> i8
-    where F: FnMut(TrapInferior, TrapBreakpoint) -> () {
+    where F: FnMut(&TrapInferior, TrapBreakpoint) -> () {

-    let mut inf = unsafe { global_inferior };
+    let mut inf = inferior;
     ptrace_util::cont(inf.pid);
     loop {
         inf.state = match waitpid(inf.pid, None) {
             Ok(WaitStatus::Exited(_pid, code)) => return code,
             Ok(WaitStatus::Stopped(_pid, signal::SIGTRAP)) =>
-                breakpoint::handle(inf, callback),
+                breakpoint::handle(&inf, callback),
             Ok(WaitStatus::Stopped(_pid, signal)) => {
               panic!("Unexpected stop on signal {} in trap_inferior_continue.  State: {}", signal, inf.state as i32)
           },
           Ok(_) => panic!("Unexpected stop in trap_inferior_continue"),
           Err(_) => panic!("Unhandled error in trap_inferior_continue")
       };
-
-        unsafe { global_inferior = inf };
   }
}

Here I’ve commened out global_inferior and allocated an Inferior dynamically in attach_inferior using Box. I’ve also started passing around references to the Box<Inferior> rather than moving the pointer.

Changes to breakpoint/mod.rs:

diff --git a/src/breakpoint/mod.rs b/src/breakpoint/mod.rs
index a069427..1d4aa1d 100644
--- a/src/breakpoint/mod.rs
+++ b/src/breakpoint/mod.rs
@@ -19,33 +19,33 @@ static mut global_breakpoint : Breakpoint = Breakpoint {
 };


-fn step_over(inferior: TrapInferior, bp: Breakpoint) -> () {
-    poke_text(inferior, bp.aligned_address, bp.original_breakpoint_word);
-    set_instruction_pointer(inferior, bp.target_address);
-    single_step(inferior);
+fn step_over(inferior: &TrapInferior, bp: Breakpoint) -> () {
+    let pid = inferior.pid;
+    poke_text(pid, bp.aligned_address, bp.original_breakpoint_word);
+    set_instruction_pointer(pid, bp.target_address);
+    single_step(pid);
 }

-fn set(inferior: TrapInferior, bp: Breakpoint) -> () {
+fn set(inferior: &TrapInferior, bp: Breakpoint) -> () {
     let mut modified = bp.original_breakpoint_word;
     modified &= !0xFFi64 << bp.shift;
     modified |= 0xCCi64 << bp.shift;
-    poke_text(inferior, bp.aligned_address, modified);
+    poke_text(inferior.pid, bp.aligned_address, modified);
 }

-pub fn handle<F>(inf: Inferior,  mut callback: &mut F) -> InferiorState
-    where F: FnMut(TrapInferior, TrapBreakpoint) -> () {
-    let inferior = inf.pid;
+pub fn handle<F>(inf: &TrapInferior,  mut callback: &mut F) -> InferiorState
+    where F: FnMut(&TrapInferior, TrapBreakpoint) -> () {

     let bp = unsafe { global_breakpoint };
     match inf.state {
         InferiorState::Running => {
-            callback(inferior, 0);
-            step_over(inferior, bp);
+            callback(&inf, 0);
+            step_over(&inf, bp);
             InferiorState::SingleStepping
         },
         InferiorState::SingleStepping => {
-            set(inferior, bp);
-            cont(inferior);
+            set(&inf, bp);
+            cont(inf.pid);
             InferiorState::Running
         },
         _ => panic!("Unsupported breakpoint encountered during supported inferior state")
@@ -58,10 +58,10 @@ pub fn trap_inferior_set_breakpoint(inferior: TrapInferior, location: u64) -> Tr
         shift : (location - aligned_address) * 8,
         aligned_address: InferiorPointer(aligned_address),
         target_address: InferiorPointer(location),
-        original_breakpoint_word: peek_text(inferior, InferiorPointer(aligned_address))
+        original_breakpoint_word: peek_text(inferior.pid, InferiorPointer(aligned_address))
     };

-    set(inferior, bp);
+    set(&inferior, bp);

     unsafe {
         global_breakpoint = bp;

These changes are mostly of one of two forms:

  1. Taking the pid field from the Inferior rather than assuming InferiorTrap is a pid.
  2. Passing references to the Box<Inferior> around instead of moving. Previously these were copies. Passing references fixes up ownership now that we don’t copy.

These changes all seem well and good until I get to the tests. I expected to have to fix up a few things (in fact the API changed in thatthe callback now takes a reference). But I didn’t understand the real problem until I saw the error:

$ cargo test
   Compiling rusty_trap v0.1.0 (file:///home/joseph/src/rust/rusty_trap)
<std macros>:5:8: 5:18 error: binary operation `==` cannot be applied to type `&Box<rusty_trap::inferior::Inferior>` [E0369]
<std macros>:5 if ! ( * left_val == * right_val ) {
                      ^~~~~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
tests/lib.rs:19:9: 19:47 note: expansion site
note: in expansion of closure expansion
tests/lib.rs:18:55: 22:6 note: expansion site

So, it seems that these references are not comparable. Which I guess makes some sense. I actually, shouldn’t have more than one copy of the pointer or reference to the pointer in the first place. That must be violating the borrow rules.

Next steps

I can’t really proceed with this refactoring. I’m going to need to go back to where I started and try something different. I’m thinking I will go back to returning the pid, or some opaque handle, to the application and then using that to lookup the Inferior data structure.

I’ll try again next week.