Should register `fn modify` be unsafe?

[jonathanpallant]

The system registers have associated functions which do a read-modify-write cycle, but the functions are not unsafe to call and do not require the (unsafe) creation of an object that represents unique ownership of that register. This means safe code can have undefined behaviour, through the main thread doing a modify on a register, which is interrupted by an exception that also does a modify on the same register. That's not ideal.

Rather than model each register as an object with a unique owner, I think the modify function should be unsafe.

I think the read and write functions can remain safe, because the effect would be no worse than conflicting store operations on an atomic variable (which is safe).

[jonathanpallant]

The other option would be for modify to take a critical-section, which seems quite expensive in terms of clock cycles.

[rikyborg]

I think the modify function should be unsafe

I think the read and write functions can remain safe

If I understand correctly, this would have the strange effect that the two functions below run exactly the same code but the first uses unsafe while the second uses only a safe API. If the first one can have UB, so does the second. But then we have safe code causing UB...

use aarch32_cpu::register::Sctlr;

fn enable_mpu_modify() {
    // comment safety requirements here
    unsafe {
        Sctlr::modify(|s| {
            s.set_m(true);
        });
    }
}

fn enable_mpu_inline() {
    // all safe code here...
    let mut s = Sctlr::read();
    s.set_m(true);
    Sctlr::write(s);
}

---

Can a racy modify cause UB, or is it just a logic bug? I don't have a good-enough mental picture to know the answer, but here's a thought.

In the context of atomics, I could implement a racy increment:

use core::sync::atomic::{AtomicUsize, Ordering::Relaxed};
static COUNT: AtomicUsize = AtomicUsize::new(0);

fn increment_good() {
    let _ = COUNT.fetch_add(1, Relaxed);
}

fn increment_racy() {
    let old = COUNT.load(Relaxed);
    let new = old.wrapping_add(1);
    COUNT.store(new, Relaxed);
}

Here, increment_racy has the same issues Sctlr::modify has, but I wouldn't say it can cause UB. I might "just" get a wrong COUNT value in the end, which is bad, but not UB in the Rust sense.

---

The other option would be for modify to take a critical-section, which seems quite expensive in terms of clock cycles.

If the conclusion from above is that a racy modify is not Rust UB, then perhaps one other option is to leave modify as is, with docs explaining the racy issue. And to then have a second modify_strong function that takes the critical section for when the racy hazard is not acceptable.

[jonathanpallant]

My thinking was that, like the atomic APIs, doing a split load/store means you are pretty much opting in to the racy behaviour where your store might replace something else that recently happened.

The modify API sort of feels more like a fetch_add operation, in that losing data might be surprising.

[rikyborg]

The modify API sort of feels more like a fetch_add operation, in that losing data might be surprising.

I agree, it's surprising and easy to overlook. At the very least it should be well documented.

My point was just that unsafe might not be the right language construct to deal with it if it doesn't cause Rust UB. To quote a recent TWIR quote of the week:

It's a footgun, yes, but it's a sound footgun.

On the other hand, if modify can actually cause UB, then so does write, so they should both be unsafe.

---

But I don't feel too strongly about it, I just thought I might share my thoughts. By the way, thank you for the work on these crates!

[jonathanpallant]

Yeah, it's a really interesting question and one that has come up in the working group multiple times.

Here's a non MMIO equivalent program:

static mut REGISTER: u32 = 0;

pub fn read_register() -> u32 {
    let ptr = &raw const REGISTER;
    unsafe {
        ptr.read_volatile()
    }
}

pub fn write_register(value: u32) {
    let ptr = &raw mut REGISTER;
    unsafe {
        ptr.write_volatile(value);
    }
}

pub fn modify_register<F>(f: F) where F: FnOnce(u32) -> u32 {
    let value = read_register();
    let new_value = f(value);
    write_register(new_value);
}

Does this program have UB? It certainly offers no protections against concurrent writes, and the modify operation is lossy. But I don't know for certain one way or another.

[jonathanpallant]

The other option is to remove modify entirely.

[diondokter]

Does this program have UB?

If multiple threads/interrupts use these functions at the 'same' time, then yes, data races are explicitly UB: https://doc.rust-lang.org/nomicon/races.html#data-races-and-race-conditions

However, that's likely not the case when it's an actual register, since (mmio) registers aren't part of the Rust abstract machine. The Rustonomicon states:

Safe Rust guarantees an absence of data races, which are defined as:

• two or more threads concurrently accessing a location of memory
• one or more of them is a write
• one or more of them is unsynchronized

So data races only apply to memory (like static vars), and thus not registers.