CODEWITHSUNDEEP
rustborrow-checker
Davide Carella
Davide Carella

Reputation: 165

Cannot borrow as mutable more than once at a time

I'm trying to build a CHIP-8 emulator in Rust to learn the langauge. I'm currently stuck trying to solve this error the compiler gives me which I wrote in the title.

I will describe the current structure of the emulator and then I will indicate where it fails.

First of all I have VM struct defined as follows

pub struct VM {
    cpu: CPU,
    memory: Memory
}

and then I have the CPU struct which has a method defined as

pub fn execute(&mut self, vm: &mut VM) -> Result<(), &'static str> {
    // ....
}

Finally the method that fails is VM::cpu_execute defined as this

pub fn cpu_execute(&mut self) -> Result<(), &'static str> {
   self.cpu.execute(&mut self)
}

This is where it fails.

I understand the error in and of itself, but in this context I really don't know how to fix it. The reason the code looks like this is so that the CPU and other VM modules can communicate: for example the CPU can access the memory by doing vm.memory() / vm.memory_mut().

I hope the question and the code is clear.

Upvotes: 4

Views: 801

Answers (3)

Ahmed Masud
Ahmed Masud

Reputation: 22402

Approach I: Simple approach

This approach says pass the bits that are mutating separately (e.g. if your CPU is mutating the memory of the VM, just pass that to the execute):


#[derive(Default, Debug)]
struct CPU;

#[derive(Default, Debug)]
struct Memory { data: Vec<u8> }

#[derive(Default, Debug)]
pub struct VM {
    cpu: CPU,
    memory: Memory
}


impl CPU {
    pub fn execute(&mut self, mem: &mut Memory) -> Result<(), &'static str> {
        println!("Executing");
        mem.data = b"deadbeaf".to_vec();
        Ok(())
    }    
}

impl VM {
    pub fn run(&mut self) {
        self.cpu.execute(&mut self.memory);
        println!("CPU changed memory to {:#?}", self.memory);
    }
}

pub fn main() {
    let mut vm = VM::default();
    vm.run();
}

Pros:

Quite simple to understand and deal with

Cons:

Somewhat limited, and you'd have to pass the stuff around.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=54836ccd4d8dc091846a2bb64df2d4f4

Approach II: Using Rc<RefCell>

This is also somewhat naïve in design:

use std::rc::Rc;
use std::cell::RefCell;

#[derive(Default, Debug)]
struct CPU {
    ip: u8, // instruction pointer
    r1: u8, // register 1
    r2: u8, // register 2
}

#[derive(Default, Debug)]
struct Memory { data: Vec<u8> }

#[derive(Default, Debug)]
pub struct VM {
    cpu: Rc<RefCell<CPU>>,
    memory: Rc<RefCell<Memory>>
}


impl CPU {
    pub fn execute(&mut self, vm: &VM) -> Result<(), &'static str> {
        println!("Executing");
        let mut mem = vm.memory.borrow_mut();
        mem.data = b"deadbeaf".to_vec();
        self.ip += 1; // increments instruction pointer. 
        Ok(())
    }    
}

impl VM {
    pub fn run(&self) {
        let mut cpu = self.cpu.borrow_mut();
        cpu.execute(&self).unwrap();
        println!("CPU changed memory to {:#?}", self.memory);
    }
}

pub fn main() {
    let vm = VM::default();
    println!("VM: {:#?}", vm);
    vm.run();
    println!("VM: {:#?}", vm);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=725587bae3d71159fe94530ac8542e68

Here we are able to pass the VM around, and the resource attempts to borrow the appropriate resources mutably or immutably as needed.

Commentary

Please take what I say as highly opinionated and with a grain of salt (in other words don't have to take what I say as the "right" way to do things).

I think that may be you should think about how to redesign the code so that there is only one "coordinator" (state machine) rather than saying each component (e.g. CPU) knowing about every other component.

Edit 1: As so correctly pointed out in the comment below the Approach II I had added superfluous RefCell, here is Approach II Redux:

Approach II: Using RefCell

use std::cell::RefCell;

#[derive(Default, Debug)]
struct CPU {
    ip: u8, // instruction pointer
    _r1: u8, // register 1
    _r2: u8, // register 2
}

#[derive(Default, Debug)]
struct Memory { data: Vec<u8> }

#[derive(Default, Debug)]
pub struct VM {
    cpu: RefCell<CPU>,
    memory: RefCell<Memory>
}


impl CPU {
    pub fn execute(&mut self, vm: &VM) -> Result<(), &'static str> {
        println!("Executing");
        let mut mem = vm.memory.borrow_mut();
        mem.data = b"deadbeaf".to_vec();
        self.ip += 1; // increments instruction pointer. 
        Ok(())
    }    
}

impl VM {
    pub fn run(&self) {
        let mut cpu = self.cpu.borrow_mut();
        cpu.execute(&self).unwrap();
        println!("CPU changed memory to {:#?}", self.memory);
    }
}

pub fn main() {
    let vm = VM::default();
    println!("VM: {:#?}", vm);
    vm.run();
    println!("VM: {:#?}", vm);
}

Upvotes: 0

Aitch
Aitch

Reputation: 1697

In many other languages, you can pass a reference into a child object's method, but in Rust you can only do this without mut.

There are many ways to solve it, but I think the best way is to understand, that when you have VM = CPU + Mem, it's a design smell (in general, not only Rust) to call vm.cpu.execute(&mut vm), because what if:

impl CPU {
    fn execute(&mut self, vm: &mut VM) {
        vm.cpu = CPU::new(); // this is valid code
        // => if `self` is `vm.cpu` it now breaks `self` reference, so prevent this we are not allowed to borrow `mut` twice
    }
}

If you don't want to change your API, you can do this, but you leave your VM is a bad state for a short while, assuming that creating CPU is cheap.

use std::mem::take;

fn main() {
    let mut vm = VM::default();
    vm.cpu_execute().expect("should work");
}

#[derive(Default)]
pub struct VM {
    cpu: CPU,
    memory: Memory
}

impl VM {
    pub fn cpu_execute(&mut self) -> Result<(), &'static str> {
        // take cpu and leave it initialized with default impl
        // => in other words self.cpu is useless for a small amount
        // of time now.
        let mut cpu = take(&mut self.cpu);
        let result = cpu.execute(self);
        self.cpu = cpu; // put it back
        
        result
    }
}

#[derive(Default)]
struct CPU;

impl CPU {
    pub fn execute(&mut self, vm: &mut VM) -> Result<(), &'static str> {
        // design smell: vm.cpu is useless and != self
        Ok(())
    }
}

#[derive(Default)]
struct Memory;

I think the best is to get rid of the design smell a go like this:

fn main() {
    let mut vm = VM::default();
    vm.cpu_execute().expect("should work");
}

#[derive(Default)]
pub struct VM {
    cpu: CPU,
    memory: Memory
}

impl VM {
    pub fn cpu_execute(&mut self) -> Result<(), &'static str> {
        self.cpu.execute(&mut self.memory)
    }
}

#[derive(Default)]
struct CPU;

impl CPU {
    pub fn execute(&mut self, memory: &mut Memory) -> Result<(), &'static str> {
        Ok(())
    }
}

#[derive(Default)]
struct Memory;

Upvotes: 2

Chayim Friedman
Chayim Friedman

Reputation: 71380

You can pass only the VM (make this an associated method) and access the CPU via the VM:

pub fn execute(vm: &mut VM) -> Result<(), &'static str> {
    // ....
}

pub fn cpu_execute(&mut self) -> Result<(), &'static str> {
   CPU::execute(self)
}

But it may be better to take &mut self and pass the memory separately.

Upvotes: 0

Related Questions