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)

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.

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.

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

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

RalfJung · 2025-05-23T18:38:00Z

This sounds good to me apart from minor nits, thanks. :)
Since I wrote parts of it, someone else should do the final review.
r? @JakobDegen maybe?

Also, let's get the process started of formally approving this:
@rfcbot merge

rfcbot · 2025-05-23T18:38:02Z

library/core/src/ptr/mod.rs

JakobDegen

Just nits

library/core/src/ptr/mod.rs

Co-Authored-By: RalfJung <post@ralfj.de>

LuigiPiucco · 2025-05-23T22:09:35Z

@rustbot ready

I tried to apply all comments and make this PR not conflict with the one changing to allocation. They should be able to apply regardless of order now.

RalfJung

On top of these comments, maybe we should also say something about provenance for the "access address outside the machine" case. After all, we probably don't want to permit that when using a pointer that has a well-defined provenance. Such a pointer pretty much has to be created with with_exposed_provenance, right?

library/core/src/ptr/mod.rs

LuigiPiucco · 2025-05-24T14:57:18Z

About provenance, I'll do some reading to try and propose a comment on its interaction with volatile, but it may be something I'm not fully qualified to do. I'll first push the other requested changes.

This reverts commit aa975c6. "Allocations" in line 116 in library/core/src/ptr/mod.rs was kept, however, because it was added as part of this commit series in rust-lang@5a52202#diff-c175d4e27676febf62c061d31cf9756d256b46e2e44cc6b3177d4ff75e932567R116-R118

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 and others added 3 commits May 23, 2025 12:50

fix: remove tests for null failure in volatile

4b3ce76

docs: further revision on read_volatile

5a52202

LuigiPiucco force-pushed the volatile-null branch from 3ea26bd to ccb7d35 Compare May 23, 2025 16:17

docs: further revision of write_volatile

aae2722

LuigiPiucco force-pushed the volatile-null branch 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

docs: fix links and typos

d0b7250

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, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 23, 2025

rustbot assigned JakobDegen May 23, 2025

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 23, 2025

rustbot unassigned RalfJung May 23, 2025

RalfJung changed the title ~~Allow volatile access to address 0~~ Allow volatile access to non-Rust memory, including address 0 May 23, 2025

traviscross reviewed May 23, 2025

View reviewed changes

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

JakobDegen approved these changes 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

RalfJung reviewed May 23, 2025

View reviewed changes

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

LuigiPiucco and others added 3 commits May 23, 2025 18:25

docs: address feedback and apply suggestions

b1e8d37

make changes compatible with rust-lang#141224

aa975c6

Co-Authored-By: RalfJung <post@ralfj.de>

refactor: deduplicate maybe_is_aligned impl

82e197b

This comment has been minimized.

Sign in to view

fix: broken links

e3181e0

RalfJung reviewed May 24, 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 Show resolved Hide resolved

RalfJung mentioned this pull request May 24, 2025

## Pre-Pre-RFC: core::arch::{load, store} and stricter volatile semantics rust-lang/unsafe-code-guidelines#321

Open

LuigiPiucco added 2 commits May 24, 2025 12:02

docs: enhance wording

80cb937

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

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!

RalfJung commented May 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

JakobDegen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LuigiPiucco commented May 23, 2025

Uh oh!

This comment has been minimized.

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LuigiPiucco commented May 24, 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

rfcbot commented May 23, 2025 •

edited by CAD97

Loading