Const validation rejects references that might be dangling (but could actually be valid at run-time)

[axos88]

View all comments

Apparently it's undefined behaviour to use:

const gpio: &RegisterBlock = unsafe { (& (*lpc176x5x::GPIO::ptr())) };

but not undefined to use:

fn x() {
  let gpio: &RegisterBlock = unsafe { (& (*lpc176x5x::GPIO::ptr())) };
  (...)
}

with:

    pub const fn ptr() -> *const gpio::RegisterBlock {
        0x2009_c000 as *const _
    }

Error message is:

error[E0080]: it is undefined behavior to use this value
  --> src/rtos.rs:27:1
   |
27 | const gpio: &RegisterBlock = unsafe { (& (*lpc176x5x::GPIO::ptr())) };
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (created from integer)
   |
   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior

This is happening on in an embedded envrionment, where there is no MMU, and access to that address is correct.
I'm not sure why it would be undefined to use it as a const, but not undefined to use it as a variable value?
Perpahs it has something to do with the borrow checker not being able to track mutable and unmutable references to that value, but this is an immutable access, so it should be fine?

[axos88]

Using *const instead of & fixes the issue with the compiler, but now it's needed to be dereferenced on each access:

const gpio: *const RegisterBlock = unsafe { lpc176x5x::GPIO::ptr() };

unsafe fn x() {
  (*gpio).pin2.write( |w| w.bits(0xDEADBEEF))
}

[Centril]

cc @oli-obk @RalfJung

[RalfJung]

Apparently it's undefined behaviour to use:
[...]
but not undefined to use:

Correction: the rules are the same in both cases, we just don't have a lint catching the latter.

There is no difference between the static version and the let version. It is just much easier to check statics than it is to check the body of a function.

This is happening on in an embedded envrionment, where there is no MMU, and access to that address is correct.

That is an interesting corner case. The reason we error is that in general, it is not correct to just access some random integer cast to a pointer.

This is a very good argument for making that error suppressible, likely by folding it into the deny-by-default const_error.

However, also note that if this is an MMIO block, references are likely not what you want. Also see this long discussion. The gist of it is that under current rules, the compiler is allowed to insert spurious reads from references. Which crate is providing the GPIO module/type here?

[axos88]

All pac crates generated from the svd files for ARM-Cortex (but might be all ARM) microcontrollers use this type of code. The svd2rust generates this code from the honky svd xml files.

It this exact case it's https://github.com/lpc-rs/lpc-pac.

But accessing "random addresses" is THE way to communicate with the hardware when you are on bare metal.

[RalfJung]

accessing "random addresses" is THE way to communicate with the hardware when you are on bare metal.

I understand. We just were not aware people would put those as references into a static, hence the check that rejects "random addresses" in references.

And also see the issues mentioned above with using references. It is incorrect in Rust to have a reference to volatile (MMIO, hardware-register) memory. It does not matter how brief that reference exists, the moment you just create a reference, the compiler is allowed to speculatively read it.

I am aware that this pattern is widely used on embedded, but that doesn't change the fact that it is incorrect, and has always been incorrect since at least Rust 1.0. Unfortunately the people implementing that pattern were not aware of this subtlety. :/
And it also seems like noone is motivated enough to push for an RFC. (Several proposals for fixing this have been made in the various threads.) I guess this is because "it works" even though it is incorrect and might break when LLVM optimizes code the wrong way.

The alternative using raw pointers mostly avoids that problem, though I cannot easily tell if it does not also locally create references.

---

So the question for rustc now is, do we want to stop erroring for this because of embedded devices? The one usecase we have here is not actually correct for references, but for reasons entirely unrelated to what this is checking. I guess it is conceivable that non-MMIO memory might also sit at a fixed address on embedded, and then references are perfectly fine.

[cramertj]

@RalfJung

It is incorrect in Rust to have a reference to volatile (MMIO, hardware-register) memory. It does not matter how brief that reference exists, the moment you just create a reference, the compiler is allowed to speculatively read it.

I know you know this, but it seems worth re-mentioning this library exists and is heavily used in the rust embedded ecosystem, and references to values of this type are often obtained by transmuting integers to references.

There are other places, too, where int->ptr occurs where the integer does not originate from a ptr->int cast, such as the address returned by the zx_vmar_map syscall (and presumably other similar calls that map something into memory and return an address pointing to the resulting mapping).

[RalfJung]

@cramertj yes, that is a problem. :/ I mean not the part where integers are transmuted, that's fine, but the part where embedded programs use &VolatileCell.

such as the address returned by the zx_vmar_map syscall

I doubt we will ever permit calling that syscall in CTFE. ;) The bug here is strictly about the checks we do on the value computed for a static or const. We do not consider it UB in general to have a raw integer in a reference (assuming that the pointer created this way is actually dereferencable).

[oli-obk]

It is incorrect in Rust to have a reference to volatile (MMIO, hardware-register) memory. It does not matter how brief that reference exists, the moment you just create a reference, the compiler is allowed to speculatively read it.

In the embedded case, that is totally fine, except for MMIO where reading mutates hardware. We don't ever read the VolatileCell<T> that is behind the reference, instead we read_volatile(&self as *const VolatileCell<T> as *const T), which is the only place we actually care about the read. So if ther are spurious reads whose value is unused inserted around that, it doesn't matter except for performance (and the already mentioned "read mutates hardware" case).

But all that seems orthogonal to the issue at hand. We don't permit references (in constants and statics) that can't be dereferenced during const eval. We should not change this behaviour. Also: we don't permit &Cell<T> in constants, why should we permit &VolatileCell<T>?

@axos88 why do you want these values stored inside statics? It seems to go against Rust's owernship scheme to have global state unless absolutely necessary. If you want that function to have access to that hardware register, pass the register down via arguments.

[RalfJung]

We don't ever read the VolatileCell that is behind the reference, instead we read_volatile(&self as *const VolatileCell as *const T), which is the only place we actually care about the read. So if ther are spurious reads whose value is unused inserted around that, it doesn't matter except for performance (and the already mentioned "read mutates hardware" case).

The general expectation with volatile is that only the accesses explicitly written by the programmer happen, nothing else. This expectation can be violated with &VolatileCell.

But all that seems orthogonal to the issue at hand.

Agreed.

Also: we don't permit &Cell in constants, why should we permit &VolatileCell?

Hm, true. OTOH, we do permit &Cell in statics, but the code above won't work for statics either.

It seems to go against Rust's owernship scheme to have global state unless absolutely necessary.

In this case the hardware is global state. Just forcing people to use local definitions does not make it any less global. Sure Rust discourages global state, but I see no reason to pretend something isn't global when in fact it is.

[oli-obk]

In this case the hardware is global state. Just forcing people to use local definitions does not make it any less global. Sure Rust discourages global state, but I see no reason to pretend something isn't global when in fact it is.

I disagree strongly, but I'm also heavily involved with https://github.com/embed-rs/stm32f7-discovery which treats hardware in an ownership based way, allowing us to use Send and Sync to prevent interrupts from modifying hardware in an uncontrolled manner. Also amongst other things, the ownership semantics make sure that we don't accidentally give multiple high level hardware drivers access to the same registers.

Anyway, independently of any opinions on ownership of hardware, the question that needs to be answered in order for this issue to be resolved is whether

const X: u32 = *Y;

should always compile successfully for any Y with &u32 type.

[nagisa]

Synthetic (or “synthesized”) pointers (pointers materialized out of “thin air”) are valid and should have their own operational semantics with respect to e.g. aliasing analysis specified, just like they do in LLVM/C/C++/etc.. Even making a reference out of them should be fine as long as the requirements for references can be satisfied. Synthetic MMIO pointers only rarely do satisfy the requirements, but they sometimes do, and those cases where they do should have defined semantics.

I believe that we should allow reborrowing data under synthetic pointers as references some way, but I’d be fine with rust making it hard(-er than usual) to achieve.

---

treats hardware in an ownership based way

Hardware registers are still global state, ownership semantics these crates add are an abstraction over this global state. A perfectly reasonable alternate design would be to have these register blocks be behind some sort of a mutex, which makes globality of these register blocks more apparent.

should always compile successfully for any Y with &u32 type.

This would have non-local semantics when X is used, so I’m against it. But it is worth noting that this is a different kind of operation compared to &*Y, and therefore perhaps not too relevant to this discussion.