remove aliased mutable borrows about f3 HOT 10 CLOSED

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. 😄

from f3.

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.

from f3.

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.

from f3.

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.

from f3.

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}
    }
}

from f3.

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.

from f3.

// 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

from f3.

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.

from f3.

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)

from f3.

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

from f3.