Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

document when atomic loads are guaranteed read-only #115577

Merged
merged 9 commits into from
Oct 17, 2023

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 5, 2023

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

rustbot commented Sep 5, 2023

r? @cuviper

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

@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 Sep 5, 2023
@cuviper
Copy link
Member

cuviper commented Sep 5, 2023

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

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 5, 2023
@rustbot rustbot assigned dtolnay and unassigned cuviper Sep 5, 2023
@Amanieu
Copy link
Member

Amanieu commented Sep 5, 2023

Some additions to the table:
aarch64 8 bytes1
arm: 4 bytes2
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
Copy link
Member Author

RalfJung commented Sep 5, 2023

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

@bjorn3
Copy link
Member

bjorn3 commented Sep 6, 2023

Can GCC guarantee this too? cc @antoyo

@antoyo
Copy link
Contributor

antoyo commented Sep 7, 2023

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

bjorn3 commented Sep 7, 2023

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

antoyo commented Sep 7, 2023

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

RalfJung commented Sep 7, 2023

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

dtolnay commented Sep 7, 2023

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

rfcbot commented Sep 7, 2023

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

-- opsem makes the call

@RalfJung
Copy link
Member Author

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

@RalfJung RalfJung added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-opsem Relevant to the opsem team and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 22, 2023
@RalfJung
Copy link
Member Author

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

rfcbot commented Sep 22, 2023

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

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.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 22, 2023
Comment on lines 105 to 108
//! | `i(3|5|6)86-`, `arm`, `thumb`, `mips(|el)-`, `powerpc-`, `riscv32`, `sparc-` | 4 bytes |
//! | `x86_64-`, `aarch64-`, `loongarch64-`, `mips64(|el)-`, `powerpc64-`, `riscv64` | 8 bytes |
//! | `powerpc64le-` | 16 bytes |
//! | `s390x-` | 16 bytes |
Copy link
Member

@taiki-e taiki-e Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! | `i(3|5|6)86-`, `arm`, `thumb`, `mips(|el)-`, `powerpc-`, `riscv32`, `sparc-` | 4 bytes |
//! | `x86_64-`, `aarch64-`, `loongarch64-`, `mips64(|el)-`, `powerpc64-`, `riscv64` | 8 bytes |
//! | `powerpc64le-` | 16 bytes |
//! | `s390x-` | 16 bytes |
//! | `i(3|5|6)86-`, `arm`, `thumb`, `mips(|el)-`, `mipsisa32r6(|el)-`, `powerpc-`, `riscv32`, `sparc-` | 4 bytes |
//! | `x86_64`, `aarch64`, `arm64`, `loongarch64-`, `mips64(|el)-`, `mipsisa64r6(|el)-`, `powerpc64-`, `riscv64`, `sparc(64|v9)-` | 8 bytes |
//! | `powerpc64le-`, `s390x-` | 16 bytes |

Changes in the above suggestion:

  • x86_64- -> x86_64: x86_64h(-apple-darwin) is not x86_64- but is x86_64
  • aarch64- -> aarch64: aarch64_be-(-unknown-linux-gnu(|_ilp32)|-unknown-linux-netbsd) are not aarch64- but is aarch64
  • add arm64: arm64_32(-apple-watchos) is not aarch64- but is aarch64
  • add sparc(64|v9)- (target_arch: sparc64), mipsisa32r6(|el)- (target_arch: mips32r6), mipsisa64r6(|el)- (target_arch: mips64r6)
  • merge powerpc64le- and s390x- into the same row

Since target triples are complex and cannot be handled correctly on the user side without a build script, I recommend using target_arch based table (like #115577 (comment)) if possible.

Copy link
Member Author

@RalfJung RalfJung Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using target_arch based table (like #115577 (comment)) if possible.

Yeah that's what we had earlier, but powerpc64le- has target_arch = powerpc64 so then we can't tell it apart from powerpc64-.

Would it make sense to use target_cpu? That's a field I can see in the JSON target spec, anyway. But it doesn't seem exposed as a cfg so I guess that's not helpful either...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recommendation is to guarantee nothing about 128-bit atomics as said in #115577 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I guess we're back to a table that just says "pointer size" is a convoluted way then. 🤷

//! |---------------|---------|
//! | `x86`, `arm`, `mips`, `mips32r6, `powerpc`, `riscv32`, `sparc`, `hexagon` | 4 bytes |
//! | `x86_64`, `aarch64`, `loongarch64`, `mips64`, `mips64r6`, `powerpc64`, `riscv64`, `sparc64` | 8 bytes |
//! | `s390x`, `powerpc64` with `target_feature = "quadword-atomics"` | 16 bytes |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quadword-atomics is not a valid rust target feature. Also note that even if valid, the powerpc target feature is not stable.

const POWERPC_ALLOWED_FEATURES: &[(&str, Option<Symbol>)] = &[
// tidy-alphabetical-start
("altivec", Some(sym::powerpc_target_feature)),
("power10-vector", Some(sym::powerpc_target_feature)),
("power8-altivec", Some(sym::powerpc_target_feature)),
("power8-vector", Some(sym::powerpc_target_feature)),
("power9-altivec", Some(sym::powerpc_target_feature)),
("power9-vector", Some(sym::powerpc_target_feature)),
("vsx", Some(sym::powerpc_target_feature)),
// tidy-alphabetical-end
];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we have some targets that enable it. How do you suggest we reflect that in the list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is reasonable to document it once the 128-bit atomics in that target and the corresponding target feature have stabilized.

@RalfJung RalfJung removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Oct 17, 2023
@RalfJung
Copy link
Member Author

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

@Amanieu
Copy link
Member

Amanieu commented Oct 17, 2023

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

bors commented Oct 17, 2023

📌 Commit e494df4 has been approved by Amanieu

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 Oct 17, 2023
@bors
Copy link
Contributor

bors commented Oct 17, 2023

⌛ Testing commit e494df4 with merge 93e62a2...

@bors
Copy link
Contributor

bors commented Oct 17, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 17, 2023
@bors bors merged commit 93e62a2 into rust-lang:master Oct 17, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 17, 2023
@rust-timer
Copy link
Collaborator

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%)

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 18, 2023
57: Pull upstream master 2023 10 18 r=pietroalbini a=Veykril

* rust-lang/rust#116505
* rust-lang/rust#116840
* rust-lang/rust#116767
* rust-lang/rust#116855
  * rust-lang/rust#116827
  * rust-lang/rust#116787
  * rust-lang/rust#116719
  * rust-lang/rust#116717
  * rust-lang/rust#111072
* rust-lang/rust#116844
* rust-lang/rust#115577
* rust-lang/rust#116756
* rust-lang/rust#116518



Co-authored-by: Urgau <[email protected]>
Co-authored-by: Esteban Küber <[email protected]>
Co-authored-by: Deadbeef <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: Camille GILLOT <[email protected]>
Co-authored-by: Celina G. Val <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: Arthur Lafrance <[email protected]>
Co-authored-by: Nikolay Arhipov <[email protected]>
Co-authored-by: Nikita Popov <[email protected]>
Co-authored-by: bors <[email protected]>
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 19, 2023
@RalfJung RalfJung deleted the atomic-load branch October 20, 2023 17:04
@taiki-e
Copy link
Member

taiki-e commented Oct 28, 2023

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

bors added a commit to rust-lang/miri that referenced this pull request Oct 28, 2023
accept some atomic loads from read-only memory

matches rust-lang/rust#115577
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Nov 4, 2023
…Jung

accept some atomic loads from read-only memory

matches rust-lang#115577
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 3, 2024
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.
 * For an external LLVM, set dependency of llvm >= 16, in accordance
   with the upstream changes.
 * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported.
 * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded
   LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it,
   resulting in an illegal instruction fault during the build.
   Ref. rust-lang/rust#117231

Upstream changes:

Version 1.75.0 (2023-12-28)
==========================

- [Stabilize `async fn` and return-position `impl Trait` in traits.]
  (rust-lang/rust#115822)
- [Allow function pointer signatures containing `&mut T` in `const` contexts.]
  (rust-lang/rust#116015)
- [Match `usize`/`isize` exhaustively with half-open ranges.]
  (rust-lang/rust#116692)
- [Guarantee that `char` has the same size and alignment as `u32`.]
  (rust-lang/rust#116894)
- [Document that the null pointer has the 0 address.]
  (rust-lang/rust#116988)
- [Allow partially moved values in `match`.]
  (rust-lang/rust#103208)
- [Add notes about non-compliant FP behavior on 32bit x86 targets.]
  (rust-lang/rust#113053)
- [Stabilize ratified RISC-V target features.]
  (rust-lang/rust#116485)

Compiler
--------

- [Rework negative coherence to properly consider impls that only
  partly overlap.] (rust-lang/rust#112875)
- [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.]
  (rust-lang/rust#116493)
- [Consider alias bounds when computing liveness in NLL.]
  (rust-lang/rust#116733)
- [Add the V (vector) extension to the `riscv64-linux-android` target spec.]
  (rust-lang/rust#116618)
- [Automatically enable cross-crate inlining for small functions]
  (rust-lang/rust#116505)
- Add several new tier 3 targets:
    - [`csky-unknown-linux-gnuabiv2hf`]
      (rust-lang/rust#117049)
    - [`i586-unknown-netbsd`]
      (rust-lang/rust#117170)
    - [`mipsel-unknown-netbsd`]
      (rust-lang/rust#117356)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------

- [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.]
  (rust-lang/rust#96979)
- [Implement `BufRead` for `VecDeque<u8>`.]
  (rust-lang/rust#110604)
- [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.]
  (rust-lang/rust#110729)
- [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.]
  (rust-lang/rust#113747)
- [Implement `Default` for `ExitCode`.]
  (rust-lang/rust#114589)
- [Guarantee representation of None in NPO]
  (rust-lang/rust#115333)
- [Document when atomic loads are guaranteed read-only.]
  (rust-lang/rust#115577)
- [Broaden the consequences of recursive TLS initialization.]
  (rust-lang/rust#116172)
- [Windows: Support sub-millisecond sleep.]
  (rust-lang/rust#116461)
- [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl]
  (rust-lang/rust#100806)
- [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.]
  (rust-lang/rust#115108)

Stabilized APIs
---------------

- [`Atomic*::from_ptr`]
  (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr)
- [`FileTimes`]
  (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html)
- [`FileTimesExt`]
  (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html)
- [`File::set_modified`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified)
- [`File::set_times`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times)
- [`IpAddr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical)
- [`Ipv6Addr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical)
- [`Option::as_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice)
- [`Option::as_mut_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice)
- [`pointer::byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add)
- [`pointer::byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset)
- [`pointer::byte_offset_from`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from)
- [`pointer::byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub)
- [`pointer::wrapping_byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add)
- [`pointer::wrapping_byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset)
- [`pointer::wrapping_byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub)

These APIs are now stable in const contexts:

- [`Ipv6Addr::to_ipv4_mapped`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped)
- [`MaybeUninit::assume_init_read`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read)
- [`MaybeUninit::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed)
- [`mem::discriminant`]
  (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html)
- [`mem::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html)

Cargo
-----

- [Add new packages to `[workspace.members]` automatically.]
  (rust-lang/cargo#12779)
- [Allow version-less `Cargo.toml` manifests.]
  (rust-lang/cargo#12786)
- [Make browser links out of HTML file paths.]
  (rust-lang/cargo#12889)

Rustdoc
-------

- [Accept less invalid Rust in rustdoc.]
  (rust-lang/rust#117450)
- [Document lack of object safety on affected traits.]
  (rust-lang/rust#113241)
- [Hide `#[repr(transparent)]` if it isn't part of the public ABI.]
  (rust-lang/rust#115439)
- [Show enum discriminant if it is a C-like variant.]
  (rust-lang/rust#116142)

Compatibility Notes
-------------------

- [FreeBSD targets now require at least version 12.]
  (rust-lang/rust#114521)
- [Formally demote tier 2 MIPS targets to tier 3.]
  (rust-lang/rust#115238)
- [Make misalignment a hard error in `const` contexts.]
  (rust-lang/rust#115524)
- [Fix detecting references to packed unsized fields.]
  (rust-lang/rust#115583)
- [Remove support for compiler plugins.]
  (rust-lang/rust#116412)
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. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging this pull request may close these issues.