-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Allow volatile access to non-Rust memory, including address 0 #141260
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
7ea9ad7
to
f652fc2
Compare
This comment has been minimized.
This comment has been minimized.
Cc @rust-lang/opsem @nikic |
2745d4f
to
d8b495b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a question, and I also pushed a commit where I did an edit pass over the read_volatile
docs. If you agree with what I did there, could you apply the same edits to write_volatile
?
This comment has been minimized.
This comment has been minimized.
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
ccb7d35
to
aae2722
Compare
It'd help if you didn't amend and force-push my commit, then I could much easier see what you changed on top of my changes... Now I guess I'll have to download your PR and compare locally against my commit (which was 3ea26bd). In general, please do not force-push while review is still in progress. Github is terrible at dealing with force-pushes. The reviewer will ask you to squash the commits before approval. |
Sorry, I'll add new commits instead of amending from now on. Other projects I worked on seemed to prefer amending, that's why I was doing it like that. |
Yeah, every project finds their own way to work around github's deficiencies. :/ I've not yet seen a good way to deal with github's abysmal treatment of force-pushes, though. I often have to pretty much re-review a PR as github provides 0 support for figuring out what actually changes in a series of pushes and force-pushes. When you create your first PR here, a bot posts a message telling you about our PR workflow. But it's probably easy to forget that... |
@rustbot ready If the PR needs further revision, would you prefer to run the |
An explicit "ready for review" signal is always a good idea. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about the inter-thread synchronization, let's repeat that in the function-level docs. As usual, my read
comments apply to write
as well.
I forgot to bless the lint's tests. Poor CI... |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@rustbot author |
This comment has been minimized.
This comment has been minimized.
ba9228e
to
5acc68b
Compare
@JakobDegen this changed a bunch since your last review so it probably needs another look. |
I'll run this directive for the sake of keeping things tidy. @rustbot ready |
5acc68b
to
67eaa1d
Compare
@RalfJung, I have rebased the branch on master and squashed the history. The only "new" change is removing the c11 link, which became orphaned and I had forgotten to remove it. I say this here because the relevant changes were squashed into the revision commit, to avoid surprise. I'll also re-request Jakob's review through GitHub, since the PR changed a lot. |
According to https://discourse.llvm.org/t/rfc-volatile-access-to-non-dereferenceable-memory-may-be-well-defined/86303/4, LLVM allows volatile operations on null and handles it correctly. This should be allowed in Rust as well, because I/O memory may be hard-coded to address 0 in some cases, like the AVR chip ATtiny1626. A test case that ensured a failure when passing null to volatile was removed, since it's now valid. Due to the addition of `maybe_is_aligned` to `ub_checks`, `maybe_is_aligned_and_not_null` was refactored to use it.
A distinction between usage on Rust memory vs. non-Rust memory was introduced. Documentation was reworded to explain what that means, and make explicit that: - No trapping can occur from volatile operations; - On Rust memory, all safety rules must be respected; - On Rust memory, the primary difference from regular access is that volatile always involves a memory dereference; - On Rust memory, the only data affected by an operation is the one pointed to in the argument(s) of the function; - On Rust memory, provenance follows the same rules as non-volatile access; - On non-Rust memory, any address known to not contain Rust memory is valid (including 0 and usize::MAX); - On non-Rust memory, no Rust memory may be affected (it is implicit that any other non-Rust memory may be affected, though, even if not referenced by the pointer). This should be relevant when, for example, reading register A causes a flag to change in register B, or writing to A causes B to change in some way. Everything affected mustn't be inside an allocation. - On non-Rust memory, provenance is irrelevant and a pointer with none can be used in a valid way.
Also remove a now-unneeded `allow` line.
67eaa1d
to
bedba77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take over this one since we haven't heard from @JakobDegen in a bit.
r? @RalfJung
I just have a few wording nits. :)
@rustbot author
@@ -28,7 +28,8 @@ | |||
//! undefined behavior to perform two concurrent accesses to the same location from different | |||
//! threads unless both accesses only read from memory. Notice that this explicitly | |||
//! includes [`read_volatile`] and [`write_volatile`]: Volatile accesses cannot | |||
//! be used for inter-thread synchronization. | |||
//! be used for inter-thread synchronization, regardless of whether it is acting on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! be used for inter-thread synchronization, regardless of whether it is acting on | |
//! be used for inter-thread synchronization, regardless of whether they are acting on |
/// reusing data from a previous read, such as from a register on which previous load of that | ||
/// memory was performed. Other than that, all the usual rules for memory accesses apply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// reusing data from a previous read, such as from a register on which previous load of that | |
/// memory was performed. Other than that, all the usual rules for memory accesses apply | |
/// reusing data from a previous read. Other than that, all the usual rules for memory accesses apply |
I think it's fairly clear what this means.
/// reusing data from a previous read, such as from a register on which previous load of that | ||
/// memory was performed. Other than that, all the usual rules for memory accesses apply | ||
/// (including provenance). In particular, just like in C, whether an operation is volatile has | ||
/// no bearing whatsoever on questions involving concurrent access from multiple threads. Volatile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// no bearing whatsoever on questions involving concurrent access from multiple threads. Volatile | |
/// no bearing whatsoever on questions involving concurrent accesses from multiple threads. Volatile |
/// Here, any address value is possible, including 0 and [`usize::MAX`], so long as the semantics | ||
/// of such a read are well-defined by the target hardware. The provenance of the pointer is | ||
/// irrelevant, and it can be created with [`without_provenance`]. The access must not trap. It | ||
/// can cause side-effects, but those must not affect Rust-allocated memory in in any way. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// can cause side-effects, but those must not affect Rust-allocated memory in in any way. This | |
/// can cause side-effects, but those must not affect Rust-allocated memory in any way. This |
/// usage that need to be distinguished: | ||
/// | ||
/// - When a volatile operation is used for memory inside an [allocation], it behaves exactly like | ||
/// [`write()`], except for the additional guarantee that it won't be elided or reordered (see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// [`write()`], except for the additional guarantee that it won't be elided or reordered (see | |
/// [`write`], except for the additional guarantee that it won't be elided or reordered (see |
for consistency
/// synchronization. Note that volatile memory operations where T is a zero-sized type are noops | ||
/// and may be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// synchronization. Note that volatile memory operations where T is a zero-sized type are noops | |
/// and may be ignored. | |
/// synchronization. | |
/// | |
/// Note that volatile memory operations where T is a zero-sized type are noops | |
/// and may be ignored. |
writes have this outside the enumeration, that seems to make more sense to me.
/// dropped when operating on Rust memory. | ||
/// | ||
/// Additionally, it does not drop `src`. Semantically, `src` is moved into the location pointed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// dropped when operating on Rust memory. | |
/// | |
/// Additionally, it does not drop `src`. Semantically, `src` is moved into the location pointed to | |
/// dropped when operating on Rust memory. | |
/// Additionally, it does not drop `src`. Semantically, `src` is moved into the location pointed to |
A single paragraph on drop works better than a long list of tiny paragraphs, IMO.
This PR relaxes the
ub_check
in theread_volatile
/write_volatile
pointer operations to allow passing null. This is needed to support processors which hard-code peripheral registers on address 0, like the AVR chip ATtiny1626. LLVM understands this as valid and handles it correctly, as tested in my PR to add a note about it (rustc generates the same LLVM IR as expected there when this PR is applied, and consequently the same AVR assembly).Follow-up and implementation of the discussions in:
r? @RalfJung
Also fixes rust-lang/unsafe-code-guidelines#29 (about as good as it'll get, null will likely never be a "normal" address in Rust)