Skip to content

Conversation

LuigiPiucco
Copy link
Contributor

@LuigiPiucco LuigiPiucco commented May 19, 2025

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)

@rustbot 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
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented May 20, 2025

Cc @rust-lang/opsem @nikic

Copy link
Member

@RalfJung RalfJung left a 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?

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

@rustbot author

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

rustbot commented May 23, 2025

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

@rustbot 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 LuigiPiucco force-pushed the volatile-null branch 2 times, most recently from ccb7d35 to aae2722 Compare May 23, 2025 16:22
@RalfJung
Copy link
Member

RalfJung commented May 23, 2025

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.

@LuigiPiucco
Copy link
Contributor Author

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
Copy link
Member

RalfJung commented May 23, 2025

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
Copy link
Contributor Author

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

@rustbot 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
Copy link
Member

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

Copy link
Member

@RalfJung RalfJung left a 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.

@RalfJung RalfJung added T-lang Relevant to the language team T-opsem Relevant to the opsem team labels May 23, 2025
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.

docs: revise restrictions on volatile operations

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.

fix: don't lint null as UB for volatile

Also remove a now-unneeded `allow` line.

fix: additional wording nits
@LuigiPiucco
Copy link
Contributor Author

Both changes applied, it should be good now.

@rustbot ready

@rustbot 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 Jul 18, 2025
@RalfJung
Copy link
Member

Looks good, thanks!
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 19, 2025

📌 Commit 8a8717e has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2025
@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 19, 2025
Kobzol added a commit to Kobzol/rust that referenced this pull request Jul 19, 2025
Allow volatile access to non-Rust memory, including address 0

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](llvm/llvm-project@6387c82#diff-81bbb96298c32fa901beb82ab3b97add27a410c01d577c1f8c01000ed2055826) (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:
- https://internals.rust-lang.org/t/pre-rfc-conditionally-supported-volatile-access-to-address-0/12881/7
- Rahix/avr-device#185;
- [#t-lang > Adding the possibility of volatile access to address 0](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Adding.20the.20possibility.20of.20volatile.20access.20to.20address.200/with/513303502)
- https://discourse.llvm.org/t/rfc-volatile-access-to-non-dereferenceable-memory-may-be-well-defined/86303

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)
jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 20, 2025
Allow volatile access to non-Rust memory, including address 0

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](llvm/llvm-project@6387c82#diff-81bbb96298c32fa901beb82ab3b97add27a410c01d577c1f8c01000ed2055826) (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:
- https://internals.rust-lang.org/t/pre-rfc-conditionally-supported-volatile-access-to-address-0/12881/7
- Rahix/avr-device#185;
- [#t-lang > Adding the possibility of volatile access to address 0](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Adding.20the.20possibility.20of.20volatile.20access.20to.20address.200/with/513303502)
- https://discourse.llvm.org/t/rfc-volatile-access-to-non-dereferenceable-memory-may-be-well-defined/86303

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)
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Jul 20, 2025
Allow volatile access to non-Rust memory, including address 0

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](llvm/llvm-project@6387c82#diff-81bbb96298c32fa901beb82ab3b97add27a410c01d577c1f8c01000ed2055826) (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:
- https://internals.rust-lang.org/t/pre-rfc-conditionally-supported-volatile-access-to-address-0/12881/7
- Rahix/avr-device#185;
- [#t-lang > Adding the possibility of volatile access to address 0](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Adding.20the.20possibility.20of.20volatile.20access.20to.20address.200/with/513303502)
- https://discourse.llvm.org/t/rfc-volatile-access-to-non-dereferenceable-memory-may-be-well-defined/86303

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)
bors added a commit that referenced this pull request Jul 20, 2025
Rollup of 12 pull requests

Successful merges:

 - #141260 (Allow volatile access to non-Rust memory, including address 0)
 - #143604 (Stabilize `const_float_round_methods`)
 - #143833 (Ban projecting into SIMD types [MCP838])
 - #143988 ([rustdoc] Make aliases search support partial matching)
 - #144078 (Fix debuginfo-lto-alloc.rs test)
 - #144111 (Remove deprecated `MaybeUninit` slice methods)
 - #144116 (Fixes for LLVM 21)
 - #144134 (Cleanup unicode table gen)
 - #144142 (Add implicit sized bound to trait ascription types)
 - #144148 (Remove pretty print hack for async blocks)
 - #144169 (interpret: fix TypeId pointers being considered data pointers)
 - #144196 (Initialize mingw for the runner's user)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Jul 20, 2025
Rollup of 11 pull requests

Successful merges:

 - #141260 (Allow volatile access to non-Rust memory, including address 0)
 - #143604 (Stabilize `const_float_round_methods`)
 - #143988 ([rustdoc] Make aliases search support partial matching)
 - #144078 (Fix debuginfo-lto-alloc.rs test)
 - #144111 (Remove deprecated `MaybeUninit` slice methods)
 - #144116 (Fixes for LLVM 21)
 - #144134 (Cleanup unicode table gen)
 - #144142 (Add implicit sized bound to trait ascription types)
 - #144148 (Remove pretty print hack for async blocks)
 - #144169 (interpret: fix TypeId pointers being considered data pointers)
 - #144196 (Initialize mingw for the runner's user)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6d7d366 into rust-lang:master Jul 20, 2025
11 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 20, 2025
rust-timer added a commit that referenced this pull request Jul 20, 2025
Rollup merge of #141260 - LuigiPiucco:volatile-null, r=RalfJung

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

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](llvm/llvm-project@6387c82#diff-81bbb96298c32fa901beb82ab3b97add27a410c01d577c1f8c01000ed2055826) (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:
- https://internals.rust-lang.org/t/pre-rfc-conditionally-supported-volatile-access-to-address-0/12881/7
- Rahix/avr-device#185;
- [#t-lang > Adding the possibility of volatile access to address 0](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Adding.20the.20possibility.20of.20volatile.20access.20to.20address.200/with/513303502)
- https://discourse.llvm.org/t/rfc-volatile-access-to-non-dereferenceable-memory-may-be-well-defined/86303

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)
@LuigiPiucco LuigiPiucco deleted the volatile-null branch July 20, 2025 14:28
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jul 21, 2025
Rollup of 11 pull requests

Successful merges:

 - rust-lang/rust#141260 (Allow volatile access to non-Rust memory, including address 0)
 - rust-lang/rust#143604 (Stabilize `const_float_round_methods`)
 - rust-lang/rust#143988 ([rustdoc] Make aliases search support partial matching)
 - rust-lang/rust#144078 (Fix debuginfo-lto-alloc.rs test)
 - rust-lang/rust#144111 (Remove deprecated `MaybeUninit` slice methods)
 - rust-lang/rust#144116 (Fixes for LLVM 21)
 - rust-lang/rust#144134 (Cleanup unicode table gen)
 - rust-lang/rust#144142 (Add implicit sized bound to trait ascription types)
 - rust-lang/rust#144148 (Remove pretty print hack for async blocks)
 - rust-lang/rust#144169 (interpret: fix TypeId pointers being considered data pointers)
 - rust-lang/rust#144196 (Initialize mingw for the runner's user)

r? `@ghost`
`@rustbot` modify labels: rollup
Muscraft pushed a commit to Muscraft/rust that referenced this pull request Jul 21, 2025
Allow volatile access to non-Rust memory, including address 0

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](llvm/llvm-project@6387c82#diff-81bbb96298c32fa901beb82ab3b97add27a410c01d577c1f8c01000ed2055826) (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:
- https://internals.rust-lang.org/t/pre-rfc-conditionally-supported-volatile-access-to-address-0/12881/7
- Rahix/avr-device#185;
- [#t-lang > Adding the possibility of volatile access to address 0](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Adding.20the.20possibility.20of.20volatile.20access.20to.20address.200/with/513303502)
- https://discourse.llvm.org/t/rfc-volatile-access-to-non-dereferenceable-memory-may-be-well-defined/86303

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)
Muscraft pushed a commit to Muscraft/rust that referenced this pull request Jul 21, 2025
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#141260 (Allow volatile access to non-Rust memory, including address 0)
 - rust-lang#143604 (Stabilize `const_float_round_methods`)
 - rust-lang#143988 ([rustdoc] Make aliases search support partial matching)
 - rust-lang#144078 (Fix debuginfo-lto-alloc.rs test)
 - rust-lang#144111 (Remove deprecated `MaybeUninit` slice methods)
 - rust-lang#144116 (Fixes for LLVM 21)
 - rust-lang#144134 (Cleanup unicode table gen)
 - rust-lang#144142 (Add implicit sized bound to trait ascription types)
 - rust-lang#144148 (Remove pretty print hack for async blocks)
 - rust-lang#144169 (interpret: fix TypeId pointers being considered data pointers)
 - rust-lang#144196 (Initialize mingw for the runner's user)

r? `@ghost`
`@rustbot` modify labels: rollup
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Jul 29, 2025
Allow volatile access to non-Rust memory, including address 0

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](llvm/llvm-project@6387c82#diff-81bbb96298c32fa901beb82ab3b97add27a410c01d577c1f8c01000ed2055826) (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:
- https://internals.rust-lang.org/t/pre-rfc-conditionally-supported-volatile-access-to-address-0/12881/7
- Rahix/avr-device#185;
- [#t-lang > Adding the possibility of volatile access to address 0](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Adding.20the.20possibility.20of.20volatile.20access.20to.20address.200/with/513303502)
- https://discourse.llvm.org/t/rfc-volatile-access-to-non-dereferenceable-memory-may-be-well-defined/86303

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What about: Targets where NULL is a valid pointer