Owned VolatileCell type?

[phil-opp]

Some people expressed interest in a volatile Cell-like type that owns the value, similar to the v0.3 interface of this crate. I think that such a type would be a nice addition to the pointer types proposed in #29, but the question is whether it is possible to implement such a type in a sound way.

The fundamental issue is that references are marked as dereferenceable by the compiler, which permits LLVM to insert spurious reads. This is not valid for volatile values since they might change in between reads. This issue was e.g. discussed in this thread on the Rust internals forum in 2019, with the conclusion that such a type would require special language support.

About half a year ago, rust-lang/rust#98017 was merged, which (if I understand it correctly) removed the dereferenceable annotation from &T where T contains an UnsafeCell. Maybe this change makes it possible to safely implement a VolatileCell with an inner UnsafeCell now?

[phil-opp]

I started to prototype a possible implementation in https://github.com/rust-osdev/volatile/blob/volatile-cell/src/cell.rs (based on #29). It looks like there is indeed no dereferenceable attribute emitted for VolatileCell read on recent compiler versions:

; volatile::cell::VolatileCell<T,A>::read
; Function Attrs: mustprogress nofree noinline nounwind nonlazybind willreturn uwtable
define internal fastcc i32 @"_ZN8volatile4cell25VolatileCell$LT$T$C$A$GT$4read17hb9d32dab412340c7E"(ptr noundef nonnull align 4 %self) unnamed_addr #6 {
start:
  %0 = load volatile i32, ptr %self, align 4, !noalias !176
  ret i32 %0
}

In comparison, I built with Rust 1.61, which does not contain rust-lang/rust#98017. We see that the dereferenceable attribute is set for the %self parameter:

; volatile::cell::VolatileCell<T,A>::read
; Function Attrs: mustprogress nofree noinline nounwind nonlazybind uwtable willreturn
define internal fastcc i32 @"_ZN8volatile4cell25VolatileCell$LT$T$C$A$GT$4read17haea1052646ef9bf4E"(i32* noundef align 4 dereferenceable(4) %self) unnamed_addr #6 {
start:
  %0 = load volatile i32, i32* %self, align 4
  ret i32 %0
}

So this looks promising. However, I'm not sure if this is something that we can rely on. Is this behavior guaranteed by the language now?

---

The above result was acquired using cargo +1.61 rustc --tests --release -- --emit=llvm-ir. This places a *.ll file under target/release/deps. To ensure that VolatileCell::read is not inlined, I marked it as #[inline(never)].

[phil-opp]

Unfortunately, the dereferenceable attribute is still emitted for methods that take &mut self, e.g. VolatileCell::write. We could take &self in these methods as a fix, but this would force us to remove the Send implementation.

Alternatively, we might be able to exploit rust-lang/rust#106180 once it's merged. As I understand it, adding a PhantomPinned field to the VolatileCell type should remove the dereferenceable attribute then. Of course, the question is again whether we can rely on this behavior.

[phil-opp]

cc @RalfJung @Lokathor @kevinaboos

[Lokathor]

I kinda fundamentally don't get what you're going for here. The whole point of MMIO being separate types is that it's not normal memory. And one of the things about it not being normal memory is that you don't ever actually have exclusive ownership of the memory. In addition to however the rust code decides to divide up the task of accessing that address, there's also the hardware that's always also at least looking at that address, and possibly modifying what's at that address. You are permanently sharing an address with an additional party, since before your program begins.

[repnop]

Unfortunately, the dereferenceable attribute is still emitted for methods that take &mut self, e.g. VolatileCell::write. We could take &self in these methods as a fix, but this would force us to remove the Send implementation.

It is my understanding that &mut should pretty much never be involved with a VolatileCell abstraction, as it makes too many guarantees about the state of the underlying data (mainly, asserting that the data will not change outside of that unique reference). Shared references should always be used with UnsafeCell to assert mutation can occur from behind the compiler's back, but as Ralf points out in this older Zulip thread, removing dereferenceable from shared mutable references is still not enough to guarantee this abstraction is sound, and at least the consensus seemed to me at the time that a proper lang item for a VolatileCell type would be the basis for any further abstractions like this as it would be known to the compiler. I'm not aware of any movement on that front currently, however.

[phil-opp]

I kinda fundamentally don't get what you're going for here.

The main motivation for an "owned" type like this that you can abstract the MMIO memory using normal repr(C) Rust structs. The only difference would be that the leaf fields are wrapped inside VolatileCell. This way, no macros are needed and you also don't need to do any address calculations yourself.

In addition to however the rust code decides to divide up the task of accessing that address, there's also the hardware that's always also at least looking at that address, and possibly modifying what's at that address. You are permanently sharing an address with an additional party, since before your program begins.

I'm awary of that. The &mut self parameter is just for ensuring no concurrent access from the software side. We deal with the hardware side by restricting the interface of VolatileCell. For example, there is no way to get a reference to the wrapped value and the only available methods map to volatile operations.

[phil-opp]

@repnop Thanks a lot for the info!

It is my understanding that &mut should pretty much never be involved with a VolatileCell abstraction, as it makes too many guarantees about the state of the underlying data (mainly, asserting that the data will not change outside of that unique reference).

That's what I feared.. There is no way to restrict an inner field to a shared reference, right? I mean something like Rc<T>, but without needing an inner pointer.

but as Ralf points out in this older Zulip thread, removing dereferenceable from shared mutable references is still not enough to guarantee this abstraction is sound

Thanks, I was not aware of that Zulip thread. From the thread:

[RalfJ] as I said further up this thread, the proposed change in rust-lang/rust#98017 does not help with VolatileCell, for the reasons stated by Connor Horman

I assume this refers to this comment above?

With the change to UnsafeCell to disable dereferencable (to allow self deallocation, approximating dereferencable_on_entry), would this make a lib volcel not problematic at the LLVM level?

note that UnsafeCell is still dereferenceable-on-entry. LLVM just doesnt have a flag for that (yet).
so, no luck yet for VolatileCell, sorry. :/ one problem at a time.

I'm now sure if I understand this correctly, but I assume that the issue is the &self argument in the UnsafeCell::get method, which is still marked as dereferenceable? If so, couldn't we work around that by using the pointer-based UnsafeCell::raw_get method instead? Or is the issue somewhere else?

[Freax13]

Not sure if this is something, but just wanted to mention the idea:
Instead of creating references to T, create references to zero sized types. This bypasses the whole dereferencable problem by reducing the amount of bytes that LLVM could dereference to 0. This might cause problems with provenance though.

[Freax13]

Is the only problem with dereferencable that it can cause spurious reads? If so, that might not necessarily be a problem for MMIO regions where reads don't have side-effects (e.g. prefetchable PCI bars).

[Lokathor]

The main motivation for an "owned" type like this that you can abstract the MMIO memory using normal repr(C) Rust structs. The only difference would be that the leaf fields are wrapped inside VolatileCell. This way, no macros are needed and you also don't need to do any address calculations yourself.

I just don't think Rust as it is today can handle this. Until there's an actual language level change I wouldn't ever suggest anyone try to do that.

I realize that this is probably "easy" for me to say because I only program for a single embedded device and not a whole ecosystem of stuff, but for the GBA I never use volatile with types that can't be read/written in a single instruction (1, 2, or 4 bytes). It's just ints or newtyped ints all the way through.

If you absolutely need it to look struct-like based on the usual struct definition syntax, I'd suggest a proc-macro that accepts a C struct as the input but expands to a newtype over usize (holding the base address) and with methods for the address of each "field" of the region (each method offsets by a const that the proc-macro has computed).

If you want a VolatileCell wrapper that would need an RFC so it can get proper compiler support.

[phil-opp]

I just don't think Rust as it is today can handle this.

Probably, but I still would like to understand what's the actual issue. Is there a potential codegen issue that could still lead to a dereferenceable attribute or is the only concern that we rely on unspecified compiler behavior?

Until there's an actual language level change I wouldn't ever suggest anyone try to do that.

I fully agree that a pointer-based approach is the better and safer solution for now. However, the cell-like approach is still quite popular in the ecosystem, e.g. through the vcell crate. So if there was a way to make such an interface safe with the current compiler versions, we could make several projects safer without requiring them to completely rewrite their volatile code.

[phil-opp]

@Freax13

Instead of creating references to T, create references to zero sized types. This bypasses the whole dereferencable problem by reducing the amount of bytes that LLVM could dereference to 0. This might cause problems with provenance though.

This would basically be the pointer-based approach, right? We could not create a struct with e.g. three volatile u16 fields this way.

Is the only problem with dereferencable that it can cause spurious reads? If so, that might not necessarily be a problem for MMIO regions where reads don't have side-effects (e.g. prefetchable PCI bars).

I guess it could still be an issue if values change between (spurious) reads and LLVM does not expect that?

[Freax13]

Instead of creating references to T, create references to zero sized types. This bypasses the whole dereferencable problem by reducing the amount of bytes that LLVM could dereference to 0. This might cause problems with provenance though.

This would basically be the pointer-based approach, right? We could not create a struct with e.g. three volatile u16 fields this way.

We could use a macro to create another struct. The struct would have fields with the same names as the original, except all types would be switched out for zero sized types. That way the whole struct would still be zero sized, but I'd still be possible to access the fields like normal fields.

[phil-opp]

Ah, but then we would need to store the field offsets somewhere. So we could get something similar to @Lokathor's proposal above:

If you absolutely need it to look struct-like based on the usual struct definition syntax, I'd suggest a proc-macro that accepts a C struct as the input but expands to a newtype over usize (holding the base address) and with methods for the address of each "field" of the region (each method offsets by a const that the proc-macro has computed).

[Freax13]

Ah, but then we would need to store the field offsets somewhere.

The offset could be part of the zero sized type for each field e.g. with const generics. I'm currently drafting something up, will publish later today.

So we could get something similar to @Lokathor's proposal above:

If you absolutely need it to look struct-like based on the usual struct definition syntax, I'd suggest a proc-macro that accepts a C struct as the input but expands to a newtype over usize (holding the base address) and with methods for the address of each "field" of the region (each method offsets by a const that the proc-macro has computed).

Using methods has the limitation that splitting a reference to struct into references to fields is not possible.

[Freax13]

Ah, but then we would need to store the field offsets somewhere.

The offset could be part of the zero sized type for each field e.g. with const generics. I'm currently drafting something up, will publish later today.

This worked surprisingly well: https://github.com/Freax13/zst-volatile

[Lokathor]

Is there a potential codegen issue that could still lead to a dereferenceable attribute or is the only concern that we rely on unspecified compiler behavior?

One problem, among possibly several, is that people seem to want to be able to write code like x.field = 2; and have it "just do the right thing", but in all other code that is a normal write operation, and it would take lang/compiler awareness of the special type to make it be a volatile operation. You can't get all the nice syntax sugar and borrow checker support and all that without lang and compiler support.

I'm probably not against some sort of VolatileCell type, but I also don't wanna have to do an entire RFC and impl cycle because there would be a number of difficult bikesheds to paint I'm sure. That's why I personally just gave up on the structure thing and went with what Rust of today does support well.

I'm definitely happy to keep answering questions though.

[kevinaboos]

Speaking as someone who needs a cell-like Volatile wrapper, we don't require a direct interface like x.field = 2; -- the older style interface with explicit .read() and .write(val) methods was both acceptable and preferred. Like most Rust devs, I tend to prefer explicitness, which may also help to avoid the dereferenceable issue mentioned above (?).

Forgive me if this has been answered, but were there any soundness or other problem(s) identified with the old version <= 0.2.7 of this crate? I know there was a limitation regarding volatile access to slices, but I haven't come across bugs regarding volatile access for standard POD types.

[phil-opp]

Forgive me if this has been answered, but were there any soundness or other problem(s) identified with the old version <= 0.2.7 of this crate?

As I understand it, the general issue is that we create references to the volatile value, which permits the compiler to insert spurious reads. For example, the read method in v0.2.7 takes &self as argument, which the compiler annotates with a deferenceable attribute. This attribute allows LLVM to read it at any time, which can be problematic for reads with side effects. I don't know if this can also be an issue for reads without side effects, e.g. when the value changes in between spurious reads.

[RalfJung]

About half a year ago, rust-lang/rust#98017 was merged, which (if I understand it correctly) removed the dereferenceable annotation from &T where T contains an UnsafeCell. Maybe this change makes it possible to safely implement a VolatileCell with an inner UnsafeCell now?

I think this has already been said, but the answer is no. References are still assumed to point to "normal" memory where reads do not have side-effects. The LLVM dereferenceable says a ton of things and we had to stop using it because some of these go too far for &UnsafeCell, so now the LLVM IR we generate is no longer obviously incompatible with a VolatileCell -- but that's only because a ton of information is lost when translating from MIR to LLVM; the Rust semantics here are still incompatible with VolatileCell.

To support something like VolatileCell, one of two things needs to happen:

• an RFC gets accepted that declares that &UnsafeCell memory may indeed be volatile, and no spurious accesses are permitted
• an RFC gets accepted that adds VolatileCell as a new primitive to Rust, with the desired semantics

[RalfJung]

One problem, among possibly several, is that people seem to want to be able to write code like x.field = 2;

FWIW I think this would be a mistake. Just compare this with atomics: assigning to an atomic variable is very different from assigning to a local variable; atomic variable assignment is more like sending a message to everyone else who has access to this location. A lot of the issues around data races and the complexities when writing concurrent shared-memory code arise because with shared memory, "sending a message" and "mutating some local state" look exactly the same. That's why message passing simplifies things so much: now "sending a message" is very different from "mutating local state". But of course Rust shows that you don't need message passing to tame concurrency, you can actually stay in the shared memory idiom and still have all the benefits of message passing concurrency (explicit points of communication) with a sufficiently strong type system that guarantees that simple assignments are never "sending a message". x = 2; in Rust is always just "mutating local state" and never "sending a message".

Volatile writes, of course, are sending a message to some other device, so making them look like regular assignments would IMO be a big loss, and lose out on the "fearless concurrency" achievement of Rust.

[Lokathor]

Perhaps I should clarify: I meant to emphasize that what I've most often seen is that people want the field projection. So the write could be done with a method instead (x.field.write(2)) instead of an equal sign, as long as having a struct with fields lets you project from the base address to the field address (x.field), and still using the volatile wrapper around the field address you end up with.

[phil-opp]

Blocked on rust-lang/unsafe-code-guidelines#411

[phil-opp]

I finally had some time to take a closer look at @Freax13's proposal at https://github.com/Freax13/zst-volatile. I think it looks quite promising:

• It avoids the dereferenceable problem by using only zero-sized types for reference types.
• It supports field projection in current Rust.
• It does not need any extra memory since all field offsets are stored as zero-sized types as well.

The only potential issue I'm seeing is that it stores the base offset inside the reference to the ZST, which might violate pointer provenance. It effectively performs the following operation:

fn main() {
    let mut x = 5;

    let ptr = &mut x as *mut i32;
    let zst_ptr: *mut Pointer = ptr as usize as *mut Pointer;
    let zst_ref: &mut Pointer = unsafe { &mut *zst_ptr };
    unsafe {zst_ref.write(10)};
    
    dbg!(x);
}

struct Pointer;

impl Pointer {
    unsafe fn write(&mut self, v: i32) {
        let ptr = self as *mut Pointer as usize as *mut i32;
        unsafe { *ptr = v; }
    }
}

(playground)

When run under miri, it throws two "warning: integer-to-pointer cast" instances, but is happy otherwise. Given that strict pointer provenance rules are only a proposal right now, I see no reason why this approach should be unsound with current Rust versions. Or am I missing something?

If the approach is sound, it might be a good idea to integrate it into the volatile crate to give people an alternative to the various unsound VolatileCell implementations on crates.io. What do you think?

[phil-opp]

(I also tried to remove the two as usize casts and instead cast pointers directly. This fixes the "integer-to-pointer cast" warnings in miri, but leads to a stacked-borrows error:

error: Undefined Behavior: attempting a write access using <2715> at alloc1449[0x0], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:17:18
   |
17 |         unsafe { *ptr = v; }
   |                  ^^^^^^^^
   |                  |
   |                  attempting a write access using <2715> at alloc1449[0x0], but that tag does not exist in the borrow stack for this location
   |                  this error occurs as part of an access at alloc1449[0x0..0x4]
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <2715> would have been created here, but this is a zero-size retag ([0x0..0x0]) so the tag in question does not exist anywhere

It looks like zero-size retags are handled in a special way. I'm not familiar enough with miri to assess whether this is a violation of the stacked borrow model or just a limitation of the current implementation.)

[Freax13]

The problem with casting a reference to a pointer directly is that under stacked borrows the provenance of new pointer is the same the old reference had. In particular the spatial component of the provenance stays the same, so the new pointer can only access bytes that were already accessible by the reference it was created from. Any pointer created by casting from a ZST reference can access exactly 0 bytes.

Casting to an integer avoids this because it implicitly "exposes" the pointer's provenance when first casting the (non-ZST) pointer to an integer and picks the "exposed" provenance back up when casting back from an integer to a pointer. This is equivalent to calling expose_addr and from_exposed_addr. The downside of this is that the compiler can't do certain optimizations when addresses are exposed e.g. keeping the object entirely in registers, but given that volatile accesses are often used to access MMIO regions that can't be kept in registers by definition, this should likely not be a huge problem.

[phil-opp]

Thanks for the explanation!

The problem with casting a reference to a pointer directly is that under stacked borrows the provenance of new pointer is the same the old reference had.

If it stays the same, the &T -> &() -> &T cast should work, shouldn't it?

The downside of this is that the compiler can't do certain optimizations when addresses are exposed e.g. keeping the object entirely in registers, but given that volatile accesses are often used to access MMIO regions that can't be kept in registers by definition, this should likely not be a huge problem.

Agreed!

[Freax13]

The problem with casting a reference to a pointer directly is that under stacked borrows the provenance of new pointer is the same the old reference had.

If it stays the same, the &T -> &() -> &T cast should work, shouldn't it?

Almost. IIUC *const T -> *const () -> *const T should work, but &T -> &() -> &T should not. This doesn't work for references because a reference can never have larger spatial provenance than the type they point to. Another problem is that it's not possible to cast references like that, we'd have to convert the reference to a pointer, cast that to a different pointer type and then dereference that pointer into a reference (&*(my_ref as *const T as *const U)). IIRC the last step of dereferencing the pointer to a reference also narrows the spatial provenance to that of the target type.

At least that's my understanding, but I am by no means an expert, so feel free to double check everything I'm saying and correct me if I'm wrong.

[RalfJung]

What @Freax13 says sounds right. Note that with Tree Borrows the rules are a bit different and the non-usize version of this is actually allowed, but there is no final decision yet on whether this should be allowed for not. The usize version basically means that the provenance of the reference is irrelevant and uses from_exposed_addr to 'smuggle' the provenance through the code without actually carrying it anywhere, which means Miri loses track but also avoids the Stacked Borrows issue.