Allow volatile access to non-Rust memory, including address 0 #141260

LuigiPiucco · 2025-05-19T17:17:29Z

This PR relaxes the ub_check in the read_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)

RalfJung · 2025-05-20T19:16:01Z

Cc @rust-lang/opsem @nikic

library/core/src/ptr/mod.rs

RalfJung

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?

library/core/src/ptr/mod.rs

RalfJung · 2025-05-23T13:50:31Z

@rustbot author

rustbot · 2025-05-23T13:50:35Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

RalfJung · 2025-05-23T16:22:45Z

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.

library/core/src/ptr/mod.rs

LuigiPiucco · 2025-05-23T16:26:03Z

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.

RalfJung · 2025-05-23T16:28:19Z

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

LuigiPiucco · 2025-05-23T16:53:32Z

@rustbot ready

If the PR needs further revision, would you prefer to run the author/ready sequence every time, or to avoid it if we're both replying quickly? I don't mind the pings, but if you do I can adjust.

RalfJung · 2025-05-23T16:56:02Z

An explicit "ready for review" signal is always a good idea. :)

RalfJung

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.

library/core/src/ptr/mod.rs

library/core/src/ub_checks.rs

LuigiPiucco · 2025-06-01T16:57:21Z

Would it be OK to change the Git history like in this branch? The tip is the same, I just squashed and reordered some commits, and removed the ones that got reverted (to correct the warning above). I can also rebase onto latest master if that would be good.

library/core/src/ptr/mod.rs

RalfJung · 2025-06-02T07:26:40Z

Would it be OK to change the Git history like in this branch? The tip is the same, I just squashed and reordered some commits, and removed the ones that got reverted (to correct the warning above). I can also rebase onto latest master if that would be good.

Yeah, feel free to clean up the history. Force-pushes that keep the base commit the same are generally preferred as github's UI is better at dealing with them.

You have conflicts though so a rebase on master is probably needed anyway, so you might as well do that.

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.

tests/ui/precondition-checks/read_volatile.rs

library/core/src/ptr/mod.rs

RalfJung · 2025-06-04T14:54:56Z

library/core/src/ptr/mod.rs

+/// Rust does not currently have a rigorously and formally defined memory model, so the precise
+/// semantics of what "volatile" means here is subject to change over time. That being said, the
+/// semantics will almost always end up pretty similar to [C11's definition of volatile][c11].


I wonder if we still need this paragraph. The next paragraph says basically as much as C says, after all.

@rust-lang/opsem what do you think?

library/core/src/ptr/mod.rs

nikomatsakis · 2025-06-04T15:37:20Z

@rfcbot reviewed

We need to allow Rust code to do anything that is needed to target esoteric systems.

traviscross · 2025-06-04T15:38:37Z

@rfcbot reviewed

Makes sense and seems necessary.

scottmcm · 2025-06-04T15:40:29Z

I'm always sad when I see volatile, so being kinda sad about this is consistent with that, I guess. If types folks are confident it's fine, then 🤷 I guess.

@rfcbot reviewed

tmandry · 2025-06-04T15:42:04Z

@rfcbot reviewed

rfcbot · 2025-06-04T15:42:09Z

🔔 This is now entering its final comment period, as per the review above. 🔔

Also remove a now-unneeded `allow` line.

RalfJung · 2025-06-04T16:03:30Z

@scottmcm FWIW I think volatile can be justified in a quite principled way. There's a lot of baggage around volatile and it is misunderstood by many, but at its core is a sensible semantics that I hope we can carve out over time. Happy to discuss this more at some point in a suitable venue :)

RalfJung · 2025-06-04T16:04:15Z

@LuigiPiucco that wording you suggested sounds good. :)

LuigiPiucco · 2025-06-04T17:20:13Z

I forgot to bless the lint's tests. Poor CI...

rustbot assigned RalfJung May 19, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 19, 2025

This comment has been minimized.

Sign in to view

LuigiPiucco force-pushed the volatile-null branch from 7ea9ad7 to f652fc2 Compare May 19, 2025 17:47

This comment has been minimized.

Sign in to view

RalfJung reviewed May 20, 2025

View reviewed changes

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

LuigiPiucco force-pushed the volatile-null branch from 2745d4f to d8b495b Compare May 22, 2025 17:02

LuigiPiucco requested a review from RalfJung May 22, 2025 17:02

RalfJung reviewed May 23, 2025

View reviewed changes

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung force-pushed the volatile-null branch from c75643f to 3ea26bd Compare May 23, 2025 13:21

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 23, 2025

LuigiPiucco force-pushed the volatile-null branch 2 times, most recently from ccb7d35 to aae2722 Compare May 23, 2025 16:22

RalfJung reviewed May 23, 2025

View reviewed changes

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2025

RalfJung reviewed May 23, 2025

View reviewed changes

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

library/core/src/ub_checks.rs Show resolved Hide resolved

RalfJung added T-lang Relevant to the language team T-opsem Relevant to the opsem team labels May 23, 2025

This comment has been minimized.

Sign in to view

workingjubilee reviewed Jun 1, 2025

View reviewed changes

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 2, 2025

LuigiPiucco added 2 commits June 2, 2025 13:22

LuigiPiucco force-pushed the volatile-null branch from 49d14c8 to 80a4707 Compare June 2, 2025 16:44

LuigiPiucco commented Jun 2, 2025

View reviewed changes

tests/ui/precondition-checks/read_volatile.rs Outdated Show resolved Hide resolved

traviscross added the P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang label Jun 4, 2025

RalfJung reviewed Jun 4, 2025

View reviewed changes

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jun 4, 2025

LuigiPiucco added 2 commits June 4, 2025 12:46

docs: address review on wording

96e3b63

fix: don't lint null as UB for volatile

ffd0f1b

Also remove a now-unneeded `allow` line.

docs: deduplicate comment on ZSTs

e836ce0

traviscross removed I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels Jun 4, 2025

This comment has been minimized.

Sign in to view

bless output for changed lint

8368ac9

tomtor mentioned this pull request Jun 7, 2025

Compiler generates wrong code for VPORTA.output (port at 0x1) Rahix/avr-hal#666

Open

Allow volatile access to non-Rust memory, including address 0 #141260

Are you sure you want to change the base?

Allow volatile access to non-Rust memory, including address 0 #141260

Uh oh!

Conversation

LuigiPiucco commented May 19, 2025 • edited by RalfJung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

RalfJung commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

RalfJung commented May 23, 2025

Uh oh!

rustbot commented May 23, 2025

Uh oh!

RalfJung commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LuigiPiucco commented May 23, 2025

Uh oh!

RalfJung commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LuigiPiucco commented May 23, 2025

Uh oh!

RalfJung commented May 23, 2025

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

LuigiPiucco commented Jun 1, 2025

Uh oh!

This comment has been minimized.

Uh oh!

RalfJung commented Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikomatsakis commented Jun 4, 2025

Uh oh!

traviscross commented Jun 4, 2025

Uh oh!

scottmcm commented Jun 4, 2025

Uh oh!

tmandry commented Jun 4, 2025

Uh oh!

rfcbot commented Jun 4, 2025

Uh oh!

RalfJung commented Jun 4, 2025 via email

Uh oh!

RalfJung commented Jun 4, 2025 via email

Uh oh!

This comment has been minimized.

LuigiPiucco commented Jun 4, 2025

Uh oh!

Uh oh!

LuigiPiucco commented May 19, 2025 •

edited by RalfJung

Loading

RalfJung commented May 20, 2025 •

edited

Loading

RalfJung commented May 23, 2025 •

edited

Loading

RalfJung commented May 23, 2025 •

edited

Loading