remove aliased mutable borrows

Question

remove aliased mutable borrows

pftbest opened this issue 8 years ago · 10 comments

I have this crazy idea and maybe you can tell me if I'm mistaken. My point is: why do we need to have mutable reference to volatile register to modify it? As we are talking about hardware registers as the name of the crate implies, the value behind it should only be accessed via volatile operations, so from the end user perspective there is no difference if the value was changed by the hardware or concurrently via another shared reference (for some hardware the value in register can be changed by simply reading the register). This requirement to have mutable reference does not give any guarantees or safety in any way.
So I propose to lift this restriction in volatile register crate and to use only immutable shared references to access the register structs. This will simplify the code considerably and remove this unsafe dance with aliased mutable borrows.

Answer 1 · 2016-11-02T14:06:20.000Z

I have this crazy idea

You are not crazy. Zinc did this (R/W registers modifiable via a &- reference).

maybe you can tell me if I'm mistaken

I'm afraid that I can't because I'm not 100% sure myself.

the value was changed by the hardware

Indeed! The hardware can change the value of a register that you are looking at via a &- reference at any time. That's what makes these while !peripheral.status_register.read().some_bit() {} lines so bizarre.

So my problem with making everything modifiable via &- is that things like these become totally safe:

static SERIAL: SerialPort = /* .. */

fn main() -> ! {
    loop {
        // "Blocks"
        SERIAL.write_all(b"The quick brown fox jumps over the lazy dog.");
    }
}

// Interrupt handler that kicks every N micro/milli -seconds
fn _tim7() {
    SERIAL.write_u8(b'X');
}

Even the implementation of SerialPort would contain zero unsafe!

At best this will produce this console output: ThXe XquXicX.... At worst the _tim7 interrupt handler may kick off when the registers are in some "inconsistent state" (probably won't be the case in this simple example but more complicated stuff will probably run into something like that).

And I'd really really like the compiler stopping me from doing this with a compiler error. It should even tell me that I need some synchronization here: "hey, this is not Sync!".

So, yeah. I don't know. I prefer to be conservative here. If I make any change, I'd probably even make the volatile reads unsafe as well because the hardware may change the value of registers at any time. 😄

Answer 2 · 2016-11-02T15:01:00.000Z

I'm OK with marking read() and write() as unsafe in volatile register, I am not OK with having this nvic() and nvic_mut() methods that look a bit ugly.

Answer 3 · 2016-11-02T15:10:39.000Z

I am not OK with having this nvic() and nvic_mut() methods that look a bit ugly.

What would be the alternative? a static mut NVIC? I don't like static variables because you can't do static mut NVIC: Nvic = unsafe { mem::transmute(0xdeadbeef) };. Instead, you have to declare those addresses in linker scripts. And integration between Cargo and linker scripts is poor.

Answer 4 · 2016-11-02T15:18:10.000Z

No I'm not talking about static, I am thinking about single nvic() method that would give &- reference that would allow unsafe writes to its registers.

Answer 5 · 2016-11-02T17:06:27.000Z

So my proposal is:

Mark read write modify methods in volatile_register as unsafe.
Allow write and modify on immutable reference
Remove all _mut() functions.

The reason I don't like &mut because it implies some kind of ownership, but we should have none.
For example nothing will prevent me from writing this code:

pub struct NVIC {
  cr1: RW<u32>,
  reserved: u32,
  cr2: RO<u32>,
}

fn main() {
    unsafe {
        let n = nvic_mut();
        n.reserved = 10;
        n.cr2 = RO{register: 0}
    }
}

Answer 6 · 2016-11-02T20:25:10.000Z

So my proposal is:

I'm not 100% convinced. It seems to me that this would result in even more unsafe blocks.

The reason I don't like &mut because it implies some kind of ownership

But that's the point! Then you can write:

fn main() -> ! {
    // ...
}

// interrupt handler
fn _tim7() {
    // I solemnly swear that this is the only `&mut -` reference to this peripheral
    // thus I can modify its contents as I see fit without worrying about data races
    let tim7 = unsafe { peripheral::tim7_mut() };

    // ...
}

And it's easy to find where I may be aliasing the register. (grep "unsafe.*_mut(").

Whereas with this:

// interrupts disabled during the execution of this function
fn init() {
    let tim7 = peripheral::tim7();

    // racy? (spoilers: no)
    let arr = unsafe { tim7.arr.read() };

    // ...

    // racy? (spoilers: no)
    let arr = unsafe { tim7.arr.read() };

    // ...
}

fn main() -> ! {
    init();
    enable_interrupts();

    let tim7 = peripheral::tim7();

    loop {
        // racy? (spoilers: yes)
        let arr = unsafe { tim7.arr.read() };
    }
}

fn _tim7() {
    // would have been `tim7_mut()` in the other model
    let tim7 = peripheral::tim7();

    // racy? (spoilers: yes)
    unsafe { tim7.arr.write(number); }
}

NOTE: arr can't be modified by the hardware

Then it's not clear which tim7 may be the source of aliasing.

For example nothing will prevent me from writing this code:

Actually, privacy prevents you from writing that. Except for n.reserved = ... but only if you are in the module where NVIC was defined which it's not usually the case.

Answer 7 · 2016-11-02T22:04:09.000Z

// racy? (spoilers: yes)
let arr = unsafe { tim7.arr.read() };

Well, the reads will never be racy because MMIO is not cached. So the only dangerous thing is read-modify-write aliased by another write. (Unless your register changes state on read operation, but in this case the current implementation is also unsafe). So you can grep on arr.write( to find all your race conditions.

It seems to me that this would result in even more unsafe blocks.

Well, maybe having unsafe on read is a bit too much.
Also having unsafe on each write is indeed more unsafe blocks than only one unsafe block on borrow, but I still think that the later case is not right, because you are allowing UB to escape it's unsafe prison.

let tim7 = unsafe {peripheral::tim7_mut()}; // <-- unsafe block is here

// ... 100 lines of code later

tim7.arr.write(number); // <-- Undefined Behaviour is here, 
                        // but this line looks like normal safe code

Answer 8 · 2016-11-03T19:38:23.000Z

I was thinking a little about this example

static SERIAL: SerialPort = /* .. */

fn main() -> ! {
    loop {
        // "Blocks"
        SERIAL.write_all(b"The quick brown fox jumps over the lazy dog.");
    }
}

// Interrupt handler that kicks every N micro/milli -seconds
fn _tim7() {
    SERIAL.write_u8(b'X');
}

and I'm not sure if it is even possible to prevent this kind of bugs in compile time. Certainly not on the register level.
At run time we can try to implement some reference counters (like RwLock) for nvic() and nvic_mut() but this would not work either because sometimes it is necessary to concurrently write to the different registers of the same peripheral. Hypothetical example: main thread updates dma.src in a loop while some interrupt updates dma.isr.

Answer 9 · 2016-11-08T03:07:08.000Z

I'm not sure if it is even possible to prevent this kind of bugs in compile time

It's certainly possible, at least in theory ... So, first you make SerialPort: !Sync that makes it impossible to put SerialPort in a static variable. Then you create a Mutex<T>: Sync that "locks the processor" by temporarily disabling the interrupts. Then you can write:

// static SERIAL: SerialPort = /* ... */;
//^ error: SerialPort doesn't implement `Sync`

static SERIAL: Mutex<SerialPort> = /* ... */;
// OK Mutex: Sync

fn main() -> ! {
    loop {
        // "Uninterruptible"
        SERIAL.lock().write_all(b"The quick brown fox jumps over the lazy dog.");
    }
}

// Interrupt handler that kicks every N micro/milli -seconds
// BUT this can't interrupt the SERIAL.lock() line in `main`
fn _tim7() {
    SERIAL.lock().write_u8(b'X');
}

With this program you always get a deterministic output:

The quick brown fox jumps over the lazy dog.XThe quick brown fox jumps over the lazy dog.X

Not that this program makes much sense but it gets synchronization right.

Here's an implementation of the Mutex used in the above example. (Here's another example that uses this Mutex)

Certainly not on the register level.

Certainly not but the registers should tell you that there's something wrong going on (through unsafe)

Answer 10 · 2017-08-24T20:23:07.000Z

Since we now have much better ways to avoid data races and undefined behavior when accessing registers, this issue is no longer relevant.