volatile: allow accesses to non-AM memory to trap

[RalfJung]

View all comments

Fixes rust-lang/unsafe-code-guidelines#610 by saying that yes, volatile accesses may trap. This also fixes what I believe to be the last case of "time-traveling UB" we have in Rust: with this change, UB always fully sequenced wrt observable events (I/O and volatile accesses). UB can still travel around unobservable events such as non-volatile memory accesses.

For writes, LLVM has already implemented the new semantics for a long time. For reads, LLVM only recently implemented the "may trap" semantics with LLVM 23. We still allow compiling rustc with older versions of LLVM. To (hopefully) prevent old LLVM from screwing this up with optimizations, this PR makes the backend emit an inline asm block without willreturn, which should prevent optimizations that break programs where a volatile load traps.

Cc @rust-lang/opsem @rust-lang/wg-llvm

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, jhpratt, nia-e

[cuviper]

For reads, LLVM only recently implemented the "may trap" semantics with LLVM 23.

However, fixes for miscompilations often do not get backported to past LLVM versions, so using rustc with older versions of LLVM comes with an increased risk of soundness bugs.

I think the intent here is mainly to acknowledge the existence of bugs in LLVM which independent packagers might want to patch for Rust's sake. It seems quite different to use this for actual UB semantics, because now you're talking about safety guarantees that cannot be upheld (IIUC).

[RalfJung]

We do sometimes have soundness bugs that are fixed by an LLVM update (so you still have the bug if you use an older LLVM).

But yeah this is different since we'd basically be introducing a new way for rustc+LLVM22 to be wrong rather than just failing to fix an old way. I just don't like having to wait another 6 months for this. :/

I wonder if there's something we can do by using more creative codegen when we are running with LLVM22, like an inline asm block after the volatile read, with that asm not being willreturn. That should prevent LLVM from reordering stuff?

[robofinch]

The access is allowed to trap. It can also cause other side-effects, but those must not affect Rust-allocated memory in any way.

What if the access results in running a signal handler that affects Rust-allocated memory? It seems very desirable for such signal handlers to be sound.

[jhpratt]

As a doc-only change, there's no concerns regarding implementation. This solely needs approval from the relevant team (whether that's lang or opsem, not my call).

[RalfJung]

What if the access results in running a signal handler that affects Rust-allocated memory? It seems very desirable for such signal handlers to be sound.

That's UB. The compiler is allowed to reorder non-volatile accesses around the volatile access.
This PR is specifically about traps (as in, aborting execution). Please take discussion of other effects elsewhere, e.g. a new Zulip thread.

[RalfJung]

I have added a little hack for LLVM22 and older that inserts a black_box after the volatile read. That should prevent LLVM from moving later UB to before the volatile read, and it makes the intrinsic as a whole non-willreturn. At least I think that's the case, would be good to have a review by an LLVM expert. :)

r? @nikic
(I know you are on vacation, no rush, this can wait)

[RalfJung]

Odd, that test doesn't include any volatile, not sure why it would be affected here... is the stack protector logic itself written in Rust and somehow in the same translation unit so the black_box can affect analyses?

Anyway, it seems like that just means some heuristics will work less well with LLVM 22.

[RalfJung]

With the inline asm "black box" implementation strategy I think this is good to go also for LLVM22 and older.

@rfcbot merge opsem

Cc @rust-lang/lang -- I think this is a sufficiently low-level point that we only need opsem FCP here, but please speak up if you disagree.

[rust-rfcbot]

@RalfJung has proposed to merge this. The next step is review by the rest of the tagged team members:

• @CAD97
• @RalfJung
• @digama0
• @joshlf
• @saethlin

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[RalfJung]

Just another test that needs LLVM 23 now due to the black box we insert on LLVM 22. Odd that it did not show up in PR CI.
I added the min-llvm-version annotation. But the test also says

// On LLVM 17 and earlier LLVM's own data layout specifies that i128 has 8 byte alignment,
// while rustc wants it to have 16 byte alignment. This test checks that we handle this
// correctly.

That's all ancient history. Is that test even worth keeping around @nikic?

@bors try jobs=x86_64-gnu-llvm-21-3

[Zalathar]

Odd that it did not show up in PR CI.

It looks like the x86_64-gnu-llvm-21 job in PR CI only runs a small subset of tests, and instead relies on the aarch64 jobs to do the real work.

But this particular test (tests/codegen-llvm/i128-x86-align.rs) is //@ only-x86_64, so perhaps it doesn't run against LLVM 21 in PR CI at all.

[rust-bors]

☀️ Try build successful (CI)
Build commit: cf8cad7 (cf8cad762733051b2dee52cea92eaf3be6f92ebb)
Base parent: 3a43cb6 (3a43cb66f97d08ccde87a00599fac2d3c50eb177)

[RalfJung]

@bors r=nikic

[rust-bors]

📌 Commit ece2980 has been approved by nikic

It is now in the queue for this repository.

[rust-timer]

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (0181ab7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

This perf run didn't have relevant results for this metric.

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: missing data
Artifact size: 401.86 MiB -> 401.00 MiB (-0.21%)