-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deal with rustc/LLVM bug around LLVM's dereferenceable
and volatile ops
#387
base: master
Are you sure you want to change the base?
Conversation
Can/should we ask people in charge from the core teams to give estimates for fixing this? Giving them some heads up about the impact for the bare-metal/embedded space upfront to consider when having a second look at the bug, which maybe helps to bump the relative priority for fixing it? |
I would say that, in general, you're using both potential paths wrong.
You need to mix the two approaches. |
Hm, if we have used the "generic" approach before, then certainly not in a lot of places, most HALs multiply the code via macro plus that also depends on the laziness/correctness of the SVD author. I'm not actually too worried about additional in svd2rust to generate the additional implementations. I can also see option 3) Wait for const generics to be stabilized, then we can use the addresses to instantiate the peripherals. |
It was asked that I put my more detailed explanation of what I think is wrong with the example, so here goes:
Instead, what you should do is also make the Here is an example of me doing that sort of thing. In my case, I'm declaring it as just a I hope this helps! |
It seems that this is getting traction now At the root of the problem as can be seen by Niko's comments in the issue he referenced (thanks @jamesmunns for highlighting). I would vote for holding off a bit from fixing this ourselves now with workarounds and closely track what happens there. |
I forgot to mention in the RFC / PR description but the "ZST approach" let us move registers out of peripherals -- this feature was requested in rust-embedded/svd2rust#213 -- so we may want to implement that approach or something like regardless of what happens with the rustc/LLVM bug. |
So wait a second. I want to make sure I understand what is motivating this RFC. I believe the problem is specifically that It seems to me that it is a touch premature to conclude that This has obviously been discussed numerous times before. rust-lang/unsafe-code-guidelines#33 has quite a lot of material. A quick skim turned up that @RalfJung previously suggested removing the deferenceable attribute for references to In short, I suppose, I don't claim to be caught up on the discussion here, but I think that before making drastic changes it would be good to prioritize this aspect of the discussion for deeper lang team discussion. |
I think the point of making this sort of a change would be that the lang team doesn't have to be consulted at all because it doesn't ask for any language changes. We just change a few libraries and we're back to soundness. |
(Meta comment: I am not sure if calling this a "rustc bug" is appropriate; if anything this is a "Rust language bug", the compiler implements the language as intended. And I think more accurately this is a "Rust language feature request"; references to MMIO memory are not supported right now and supporting them is a feature we'd like to have. Certainly there is no "LLVM bug" anywhere in sight here.)
It is unsound under the current conservative rules. Not sure what else "unsound" could mean. Sure, it could be made sound by language changes, but that is true for many cases of unsoundness we are facing. ;)
Absolutely! But it isn't right now, so there are two things we can explore in parallel: (a) how to make it expressible; this involves language changes. (b) how to write an ergonomic MMIO library under the current conservative rules; this can be explored by libraries without lang team involvement. |
Why would it be expressible? That part genuinely doesn't make much sense to me. If you have a reference to an MMIO location, then within rust's type system that logically means that you can also own an MMIO location by de-referencing it, which is pretty nonsense because you never own the MMIO location. |
You don't own memory behind shared references either, so that doesn't seem like an issue to me. |
I'm more talking about mental modeling of the types, not literal ownership. But if I'm the only one who thinks it's weird then whatever. |
The situation is very similar for concurrent data structures like crossbeam's channels, where the conceptual idea of "ownership" also has to expanded to properly explain what happens. But shared references can handle the intricate interactions of multiple threads cooperatively implementing a channel; I do not see why they couldn't also model MMIO interactions. |
Hi everyone, I wanted to post an update from the @rust-lang/lang side of things. On 2020-02-10, we held a "design meeting" where we talked over the question of the dereferenceable attribute that is attached to references in detail. You can see the minutes and there is also a recording available if you'd like to watch. The meeting was not, I don't think, conclusive -- I'm hoping to find some time to try and summarize the key conclusions, in part to understand them better myself! Still, there are a few things I can say. Going into the meeting, I hoped that we would find that it seemed "obviously correct" that Moreover, it seemed like there may be just a categorical difference between MMIO and the other use cases that motivate making In LLVM terms, these might be addressed by upcoming changes that change dereferenceable to mean "dereferenceable on function entry". We could potentially adapt stacked borrows in a similar way, though this would mean foregoing some of the optimizations we had hoped for, and it wouldn't solve all the cases we might wish (see "What does this help, and what doesn’t it help?" in the notes). The issue with MMIO however is different -- in this case, you want something like So, in conclusion, we were weighing two options. Neither of them is wholly satisfying, and only one of them helps with MMIO:
It's hard to say which of these is better, and I guess I would say that we'll probably spend some time looking for a "third way". But this does mean that we can't say for sure whether whatever solution we find would help with MMIO. So what does that mean for this RFC? In the meeting, we discussed that the right way to model volatile memory might be to use raw pointers and not references. So instead of passing around Either way, it seemed like if you had a #[derive(Copy)]
struct VolatileRef<T> {
pointer: *const T
}
impl<T> VolatileRef<T> {
unsafe fn new(ptr: *const T) { ... }
fn get(self) -> T {
unsafe { ... }
}
} so that you can encapsulate the unsafety into the Anyway, I don't want to try and design an ergonomic API in this comment, I'm merely pointing out this as a possible direction to consider. I guess the overall conclusion would be:
|
@nikomatsakis This does not quite belong to this thread, but I haven't found another place to discuss those meeting notes. Please point me to one, if any. I think there could be a use case for non- |
I would suggest a struct with a raw pointer, len, and a PhantomData for lifetime. |
@HadrienG2 thanks (and you're right that there's no obvious place to collect feedback on the notes). I agree with @Lokathor that a struct could be used in that case to "emulate" a reference, but I can see why it might be nice to have some more native way to model it as well. |
A few thoughts about this issue/RFC:
By the way, I saw that redox uses |
Regarding volatile and
No, |
Looking at the redox line in isolation, any with no other knowledge of their project, it's just plain wrong. unless by "Mmio" they for some reason mean something else entirely. |
The current implementation of the peripheral API generated by
svd2rust
isaffected by bug rust-lang/rust#55005. The (hypothetical) observable effect of
this bug on the
svd2rust
API is that LLVM may insert spurious reads to MMIOregisters in optimized builds. These spurious reads in turn may cause some bits
of the register to be cleared (e.g. interrupt flags) or flipped, changing the
intended behavior of the program.
The way to prevent this rustc/LLVM bug is to never create a reference (
&-
or
&mut-
) to a MMIO register, or in general to memory that is meant to beaccessed only through volatile operations. This document explores alternative
implementations of the
svd2rust
API to avoid this bug.Rendered