document when atomic loads are guaranteed read-only

[RalfJung]

Based on this discussion in Zulip.

The values for x86 and x86_64 are complete guesswork on my side, and I have no clue what the values might be for other architectures. I hope we can get the right people to chime in to gather the required information. :)

I'll update Miri to respect these rules once we have more data.

[rustbot]

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

[cuviper]

I'm going to put this in the API domain, since it's making guarantees.

@rustbot label -T-libs +t-libs-api
r? libs-api

[Amanieu]

Some additions to the table:
aarch64 8 bytes¹
arm: 4 bytes²
riscv64: 8 bytes
riscv32: 4 bytes

Footnotes

1. ARMv8.4 raises this to 16 bytes, but I don't think we need to document this here. We're only guaranteeing minimums. ↩

2. Similarly, this is raised to 8 bytes with the LPAE extension. ↩

[RalfJung]

Cc @rust-lang/opsem for help in filling out the table

[bjorn3]

Can GCC guarantee this too? cc @antoyo

[antoyo]

Can GCC guarantee this too? cc @antoyo

I'm not sure I have enough context/understanding of the situation.
Does Rust support atomic loads of bigger size (e.g. more than 8 bytes on x86_64)?
Isn't __atomic_load always guaranteed read-only?
Or are we talking about cases where Rust atomic loads use __atomic_compare_exchange?

[bjorn3]

On x86 LLVM emits cmpxchg8b for 64bit loads and on x86_64 cmpxchg16b for 128bit loads as far as I understand, which is considered a write (and thus causes SIGSEGV on read-only memory) even if it wouldn't actually change the value at the memory location in question. For values of at most the register width LLVM will use mov on x86/x86_64 which is guaranteed read-only. Does GCC behave the same?

[antoyo]

On x86 LLVM emits cmpxchg8b for 64bit loads and on x86_64 cmpxchg16b for 128bit loads as far as I understand, which is considered a write (and thus causes SIGSEGV on read-only memory) even if it wouldn't actually change the value at the memory location in question. For values of at most the register width LLVM will use mov on x86/x86_64 which is guaranteed read-only. Does GCC behave the same?

The doc mentions:

16-byte integral types are also allowed if ‘__int128’ (see 128-bit Integers) is supported by the architecture.

but trying to do so on my computer results in the following error:

undefined reference to `__atomic_load_16'

and I read somewhere that 16-byte atomic operations always call a function in gcc, so not read-only.

I assume we would need to call __atomic_always_lock_free for every value in this table to make sure, but I'm not sure it would make sense that "always_lock_free" means "guaranteed read-only".

[RalfJung]

Rust atomics must always be lock-free, so if the GGC backend uses a lock implementation, that's already a bug -- but separate from the issue discussed here.

[dtolnay]

I'm going to put this in the API domain, since it's making guarantees.

@rust-lang/libs-api:

This is module-level documentation for core::sync::atomic. https://doc.rust-lang.org/1.72.0/core/sync/atomic/index.html

As I understand it, the new guarantee introduced here would make the following sound: we want a value that can be atomically loaded but the backing data might either be the read-write memory where interior mutable values ordinarily go, or could be readonly memory on certain architectures that have this new guarantee.

#[repr(transparent)] pub struct AtomicallyLoadableU64(AtomicU64); impl AtomicallyLoadableU64 { #[cfg(any(target_arch = "x86_64", target_arch = "aarch64", target_arch = "riscv64"))] pub fn from_readonly(readonly: &u64) -> &Self { unsafe { ... } } pub fn from_interior_mutable(interior_mutable: &AtomicU64) -> &Self { unsafe { ... } } pub fn load(&self, ordering: Ordering) -> u64 { self.0.load(ordering) } }

Perhaps we'd be happy to delegate this to a T-opsem FCP? I'm not sure that I would have any relevant API insight on this. It seems fine to guarantee, if this is a stable property these architectures and our compiler backends can guarantee.

@rfcbot poll libs-api -- opsem makes the call

[rfcbot]

Team member @dtolnay has asked teams: T-libs-api, for consensus on:

-- opsem makes the call

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

[RalfJung]

@BurntSushi @joshtriplett @m-ou-se there's an rfcbot poll waiting for your checkmark here. :)

[RalfJung]

The t-libs-api rfcbot poll has now reached the usual threshold for team consensus (a strict majority of approvals with at most two votes still open). So I assume we can hand this over to t-opsem.

@rfcbot merge

[rfcbot]

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

• @CAD97
• @JakobDegen
• @RalfJung
• @digama0
• @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]

All right, so we have FCP passed and approval from our domain expert.
@Amanieu I think this is ready to land. :)

[Amanieu]

I'm still not very happy about not supporting load(Acquire), I honestly think it's a bug that this doesn't work on read-only memory and we should fix targets (like armv5) where this happens. But this can be done later.

@bors r+ rollup

[bors]

📌 Commit e494df4 has been approved by Amanieu

It is now in the queue for this repository.

[bors]

⌛ Testing commit e494df4 with merge 93e62a2...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 93e62a2 to master...

[rust-timer]

Finished benchmarking commit (93e62a2): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-1.9%	[-2.8%, -0.9%]	2
All ❌✅ (primary)	-3.2%	[-3.2%, -3.2%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.6%	[-5.6%, -5.6%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 626.338s -> 623.96s (-0.38%)
Artifact size: 305.62 MiB -> 305.57 MiB (-0.02%)

[taiki-e]

@RalfJung

So, that sounds like we need an issue tracking that on that target, our atomics aren't lock-free as they should be? I haven't really been able to understand the details; @taiki-e I'd appreciate if you could file an issue. :)

Filed as #117305 and #117306.