Refactor fail
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::handleto lookup the correct breakpoint - Fix
trap_inferior_set_breakpointto 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:
- Taking the pid field from the
Inferiorrather than assumingInferiorTrapis a pid. - 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.
Enjoying the post? Sign up for the newsletter or support the work on Buy Me a Coffee .
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.
View all posts and upcoming topics →