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

[axos88]

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.

[oli-obk]

Reborrows are already fine in const eval. It's only when you have a reference in a final constant that we prevent "invalid" (according to an on-purpose-restrictive definition). We can lift restrictions if there's a good argument for that.

Allowing undereferencable references in constants is OK, promoteds can't deref anyway. At least I hope we also prevent implicit derefs in field accesses

[RalfJung]

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.. [..]
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.

To reiterate, this is not at all about the dynamic semantics of Rust.

This is entirely about what checks we are making on the value that we compute (at compile-time) for a const or static of reference type. These values can be used by other statics/constants and even promoteds, so we want to be a bit careful here, in particular for the latter.

[axos88]

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.

I also disagree with this statement. Yes, there are cases where this makes sense (such as a serial port, or an I2C bus, where the hardware has complex state). But in other cases, such as driving a few LEDs for debuging purposes, it's not.

I want to be able to change my debug LEDs from all over the place without having to pass down the reference to them and fight rusts ownership system, and then possibly remove them in production code (imagine the complexity conditional compilation would bring when I need to have them on in debug mode, and off in production). Also you could think about those blinking LEDs as the most simplistic stdout, and this IS global even in std, isn't it?
So in some cases yes, it makes sense to pass down references to pieces of hardware to ensure only one piece of code can manipulate it, but in other cases I want to be in control, because I believe I can ensure that nothing bad will happen(TM). If that makes for unsafe code, that's totally fine, but it needs to be possible

[oli-obk]

Stdout in libstd uses a mutable static that is initialized at runtime. This would work for these references that can't be dereferenced at compile-time, because you only have the odd value at runtime. At the same time thisvwould fit into the ownership scheme, because you then lose access to that pin, so no other driver may attempt to use it. Many boards allow remapping pins, so "just an LED" is not a necessarily universally true statement.

You can always create a global function that generates the value for you and call that instead of using a constant.

Note that I'm not against changing the behaviour here just because I disagree with the way ppl are using hardware on embedded. I won't block us making these rules less strict, as long as we're clear on what such a change entails. Right now, any reference in a constant's value can be dereferenced in another constant. We'd lose this guarantee. As @nagisa said, it's not necessarily a guarantee that makes sense or is desirable, but we should be aware that we'd change it and that this may have far ranging consequences. E.g. it would forever forbid us from allowing dereferencing in promoteds, even though that would be sane to allow right now. If we had allowed it on stable, then it would be a breaking change to allow undereferencable references in constants

[RalfJung]

I am not worried about restricting what new features can be added to promoteds. They are already too powerful anyway and extensions should IMO happen in a more explicit mechanism (such as const blocks).

[Centril]

At any rate, I don't think this is something that should be changed by issue or PR here since it has wider implications. I would like to see an elaborated RFC if something is to be changed here. (But please continue discussing...)

[RalfJung]

To be fair, we introduced and strengthened those checks without much ado. So it seems weird to require an RFC for weakening them.

[Centril]

Well we could also just FCP in one way (my current preference is to keep things as-is) but it seems like there's a debate here which is why an RFC seems appropriate.

[oli-obk]

There's no debate. There's just the "is everyone aware of and OK with the consequences?". So far no one has argued against doing it. I'm mostly arguing that the use case is invalid, but that's just a personal opinion not founded in any evidence.

I guess the only thing speaking against it is that you can't tell from a constant's type whether a deref is OK, but we already can't tell whether subtracting 1 from a usize constant is ok

[RalfJung]

we already can't tell whether subtracting 1 from a usize constant is ok

In which sense? The MIR subtraction operations (checked and unchecked) are AFAIK guaranteed to be okay on a usize. (We do check that they are not pointers.)

[oli-obk]

Well.. 0usize stored in a constant FOO will abort const eval if you do 1/FOO. So the type itself is no guarantee that the evaluation will work

[RalfJung]

That's panicking, which is very different (IMO) from raising another kind of evaluation error.

[oli-obk]

That difference is only relevant when we're talking about implicit promotion, because there the difference to the runtime behaviour is noticable. It is very different conceptually, but not in how it's noticable from a user perspective

[axos88]

I'm not sure I entirely follow your last message, however I am getting a feeling that statics in rust are conflating two things, what you call promotion: compile-time consts and run-time constants. The former being a compile-time only thing that never reaches the end binary and the latter being an entry in the .rodata section, living in the actual memory space of the end application, that is read from that particular memory space whenever its value is required. Oh, and then there is the mutable values, that would end up in .data, that can be modified, which are denoted static mut in rust. My code would obviously not uphold the requirements of the compile-time constants, but I don't see anything that should disallow them becoming run-time constants.

Wouldn't the clean solution be to separate these concepts? Say

• static mut (.data)
• static readonly (.rodata)
• static const (compile-time only, guaranteed to be inlined and optimized away. Maybe just #[inline]?)

Wouldn't this make it much easier to reason what can and cannot become static const?
In some cases (again in embedded) I DO want to have control over which constants actually end up in .rodata instead of being optimized away, because I may want to have access to a memory space where that value is stored. Can't remember the exact use case, but I vaguely remember having to use a static mut instead of a static once to overcome this.

[axos88]

In this particular storing dangling references would be a no-no for static consts, but would be okay in static readonlys. Dereferencing them would be disallowed, since it can't happen at compile time.

[RalfJung]

compile-time consts and run-time constants. The former being a compile-time only thing that never reaches the end binary and the latter being an entry in the .rodata section, living in the actual memory space of the end application, that is read from that particular memory space whenever its value is required.

"promotion" is unfortunately a heavily overloaded term, but the one I am talking about here is the automatic creation of .rodata entries. See this RFC and also this document for further details.

Compile-time constants ("constant propagation") are entirely irrelevant for the discussion in this issue.

Oh, and then there is the mutable values, that would end up in .data, that can be modified, which are denoted static mut in rust.

static FOO: Mutex<i32> also ends up in .data.

[nikomatsakis]

We discussed this in our 2019-10-10 meeting -- we considered a proposal by @RalfJung. We didn't reach any firm decisions but at least some people thought that this seemed reasonable:

• const <expr> — explicit const-promoted expression form (“anonymous constant”)
• forever decide not to “promote” *<expr>
• loosen the validation rules so that &T doesn’t have to be a valid pointer

As an immediate step, @ecstatic-morse has been working towards better specifying the const promotion rules, and that seems like an important part of the puzzle to work out first.

[RalfJung]

Btw @axos88 I think we never got to the bottom of whether RegisterBlock contains MMIO memory? Because if it does, this

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

is wrong. &RegisterBlock gives the compiler permission to perform arbitrary reads from the memory the reference points to, even if the code would never read from that memory. Last time I tried to discuss this, we got derailed.

So, what is that RegisterBlock type? It sure sounds like something that would describe MMIO memory.

[axos88]

It is MMIO, but in case of the processor we are coding for there is no MPU, so code can always read from that memory region.

[axos88]

Well... Except if the peripheral is powered off, but i'm not sure that's something we would want to catch at the compiler level.

One could argue that because of that caveat this should be wrapped in unsafe code, but it should be able to compile. The user of said reference should then be responsible for checking if accessing that memory area is permissible in that context, which is basically the definition of unsafe code, is it not?

[RalfJung]

@axos88 the thing with references is that they can get accessed by the compiler even if the code does not access them. That's what it means for them to be "dereferencable". For example, if you have

if foo { let val = *bar; }

then if bar is a reference the compiler is permitted to move that read out of the if.

[RalfJung]

The "wrapping in unsafe code" is exactly what using raw pointers here achieves.

[ewoudje]

Has there been any change? Or a way to disable the compiler for checking it?

I have this problem with a custom lazy static implementation. It makes static values with an UnsafeCell but the value in UnsafeCell are all zero's (wich later get initialized). It works until i add a reference in the struct then it stops working and says there is a NULL reference.

pub struct Initializer<T> {
    data: UnsafeCell<T>,
    initialized: RwLock<bool>
}

[RalfJung]

It is UB to have a NULL reference, so that error sounds correct to me.

If you work with uninitialized memory, you need to use MaybeUninit.

[ewoudje]

It is UB to have a NULL reference, so that error sounds correct to me.

If you work with uninitialized memory, you need to use MaybeUninit.

Oops my bad didn't know that existed...
Thanks for the help!

[RalfJung]

Basically the same issue but for function pointers recently came up on Zulip.

[Isaac-Leonard]

I've been working on converting this repo over to rust: https://github.com/esp32-open-mac/esp32-open-mac but am having usability issues with making the registers work.
I've made my own mini register implementation that wraps VolatileCell and I have got a function:

    pub const fn at(addr: usize) -> &'static Self {
        unsafe { (addr as *const Register<T>).as_ref().unwrap() }
    }

For easily creating pointers without having to copy and paste the internal representation for every register I make.
For now I'm attempting to keep my code as close to the original as I can to ensure correctness before refactering and making it more idiomatic rust but have come across this error:
error[E0080]: it is undefined behavior to use this value
--> src/hardware.rs:75:1
|
75 | const WIFI_TX_CONFIG_BASE: &Register = Register::at(0x3ff73d1c);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered a dangling reference (0x3ff73d1c[noalloc] has no provenance)
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
= note: the raw bytes of the constant (size: 4, align: 4) {
1c 3d f7 3f │ .=.?
}

note: erroneous constant encountered
--> src/hardware.rs:296:5
|
296 | WIFI_TX_CONFIG_BASE
| ^^^^^^^^^^^^^^^^^^^
...,...,...
It seems as though my only option is to have the constants be raw pointers rather than references and then have to call as_ref().unwrap() every time I want to use one of the registers until I refacter properly, is this correct?
Is there a better way to go about this for now?
I've enabled the relivent features in my crate too:
#![feature(const_ptr_as_ref)]
#![feature(const_option)]
And yes I'm aware that VolatileCell is unsound but currently theres no better option and from the thread linked above there have been no actual issues encountered due to it so far.

[RalfJung]

And yes I'm aware that VolatileCell is unsound

It's unlikely that we will do a language change to cater an unsound library.

The proper solution is to use raw pointers. AFAIK there are multiple ways to use them. They might not be quite as ergonomic and VolatileCell, but they seem "good enough" for most cases. I suggest asking on https://users.rust-lang.org/ or on Zulip about alternatives to VolatileCell.