Allow arithmetic and certain bitwise ops on AtomicPtr #96935

thomcc · 2022-05-11T07:48:42Z

This is mainly to support migrating from AtomicUsize, for the strict provenance experiment.

This is a pretty dubious set of APIs, but it should be sufficient to allow code that's using AtomicUsize to manipulate a tagged pointer atomically. It's under a new feature gate, #![feature(strict_provenance_atomic_ptr)], but I'm not sure if it needs its own tracking issue. I'm happy to make one, but it's not clear that it's needed.

I'm unsure if it needs changes in the various non-LLVM backends. Because we just cast things to integers anyway (and were already doing so), I doubt it.

API change proposal: rust-lang/libs-team#60

Fixes #95492

rust-highfive · 2022-05-11T07:48:45Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-05-11T07:48:46Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

thomcc · 2022-05-11T07:58:33Z

This is a new unstable API, so I guess this is appropriate.

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

These wouldn't stabilize separately from the rest of the strict provenance experiment, though, and at the moment are mostly to help test concurrent code under miri.

library/core/src/sync/atomic.rs

m-ou-se · 2022-05-11T21:36:32Z

I think fetch_add in this PR can easily be a footgun that I'd totally shoot myself with. For example, I might have an AtomicPtr to a 16-byte aligned struct, of which I use the lower 4 bits as some kind of counter. Then I'd probaly write a.fetch_add(1) assuming it'd add 1 to that counter instead of bumping the pointer 16 bytes over.

WaffleLapkin · 2022-05-11T22:10:44Z

@m-ou-se Isn't the same applicable to <*const T>::add?

m-ou-se · 2022-05-11T22:13:44Z

Not as much, because it's quite uncommon to do tricks like storing flags or counters inside a regular raw pointer. It's very common to do that with atomics though. There are many AtomicUsizes in the world that store a pointer and some flags, which could (should?) be upgraded tot AtomicPtr instead. Having AtomicPtr::atomic_add could be a big pitfall then.

thomcc · 2022-05-14T04:15:33Z

I think fetch_add in this PR can easily be a footgun that I'd totally shoot myself with. For example, I might have an AtomicPtr to a 16-byte aligned struct, of which I use the lower 4 bits as some kind of counter.

I don't disagree, but on the other hand if we have fetch_add default to bytes, something like a.fetch_add(1, ord).add(1) will do the wrong thing.

It's a tricky API problem -- I'm open to suggestions. One option would be to only offer the bytewise versions.

m-ou-se · 2022-05-14T07:38:35Z

I don't disagree, but on the other hand if we have fetch_add default to bytes

I'm not saying we should do that either. That's just as confusing.

m-ou-se · 2022-05-14T07:41:58Z

Maybe we should have fetch_byte_add and fetch_ptr_add or something like that.

library/core/tests/atomic.rs

thomcc · 2022-05-14T19:37:58Z

Maybe we should have fetch_byte_add and fetch_ptr_add or something like that.

I've gone ahead and renamed things along these lines.

WaffleLapkin · 2022-05-15T15:51:30Z

I'm slightly worried that having an inconsistent naming scheme is worse, than having fetch_add on pointers be in units of T. To me fetch_ptr_add sounds like it takes *const T instead of an usize.

thomcc · 2022-05-15T17:43:32Z

This problem is actually why it took so long for me to make this PR.

thomcc · 2022-05-16T21:49:39Z

Not as much, because it's quite uncommon to do tricks like storing flags or counters inside a regular raw pointer

FWIW this isn't exactly true. Code like our std::io::Error isn't that rare, especially in contexts like virtual machines and language runtimes. It's not limited to stuff like that though, you also see similar tricks in the semver and string_cache crates. And this sort of thing is a common footgun for them too, when ported from using integers to pointers.

I'm not sure how much this matters, but it does make some case that this footgun would not be unprecedented (although I'm unsure how good of an argument that is in and of itself).

thomcc · 2022-06-30T17:44:15Z

I probably need to update this after #97423. I'll take a look later today or tomorrow.

This is mainly to support migrating from AtomicUsize, for the strict provenance experiment. Fixes rust-lang#95492

rustbot · 2022-07-01T13:22:26Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

thomcc · 2022-07-01T17:57:35Z

This didn't end up needing any changes, but I rebased it, and wrote a change proposal (bit of a long one, sorry if they should be shorter -- the semantics are just a bit subtle, so I tried to cover everything): rust-lang/libs-team#60

@bors ready

thomcc · 2022-07-01T17:58:13Z

Err, hm. Wrong bot probably (oops). Either way:

@rustbot label -S-waiting-on-author +S-waiting-on-review

RalfJung · 2022-07-02T17:35:36Z

Btw, these operations will also need changes on the Miri side to be supported there.

dtolnay

This looks terrific to me. :) Thanks. I concur with all of the design decisions justified in rust-lang/libs-team#60:

the idea that these should exist at all, rather than doing CAS loops in the downstream code
element-sized offsetting by itself is not sufficient
byte-sized offsetting by itself would be annoying (but I'm sure we'll revisit this possibility when stabilizing)
the specific naming scheme fetch_ptr_add and fetch_byte_add
atomptr.fetch_ptr_add(n, ord).add(n) as the recommendation for non-wrapping semantics

The last would be good to document as an idiom in the part of the documentation that talks about fetch_ptr_add's relationship with wrapping_add, or maybe in the example code. r=me if you get to it before bors does, otherwise separate PR or leave to the stabilization PR.

dtolnay · 2022-07-06T17:17:00Z

@bors r+

bors · 2022-07-06T17:17:02Z

📌 Commit e65ecee has been approved by dtolnay

…laumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#96935 (Allow arithmetic and certain bitwise ops on AtomicPtr) - rust-lang#98519 (Replace some `guess_head_span` with `def_span`) - rust-lang#98911 (rustdoc: filter '_ lifetimes from ty::Generics) - rust-lang#98939 (rustdoc: Add more semantic information to impl IDs) - rust-lang#98971 (Fix typo in file descriptor docs) - rust-lang#98983 (docs: Add overview of `rustc_middle::mir::TerminatorKind`) - rust-lang#98984 (Remove erroneous doc comment) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

RalfJung · 2022-07-07T01:37:23Z

library/core/tests/atomic.rs

+    assert_eq!(atom.load(SeqCst), ptr.map_addr(|a| a | 0b1001));
+
+    assert_eq!(atom.fetch_and(MASK_PTR, SeqCst), ptr.map_addr(|a| a | 0b1001));
+    assert_eq!(atom.load(SeqCst), ptr);


This test just compares the address of the pointer.

Would be good to also load from the pointer obtained via atom.load(), to ensure that it also has the provenance to actually access memory.

rustup; ptr atomics Adds support for the operations added in rust-lang/rust#96935. I made the pointer-binops always return the provenance of the *left* argument; `@thomcc` I hope that is what you intended. I have honestly no idea if it has anything to do with what LLVM does... I also simplified our pointer comparison code while I was at it -- now that *all* comparison operators support wide pointers, we can unify those branches.

jonhoo · 2022-07-07T23:40:52Z

This lists #95228 as the tracking issue. Should that issue be updated to also mention this new feature (strict_provenance_atomic_ptr that is), or should we open a new, different tracking issue specifically for these methods? It feels like there's a decent argument for landing those independently of the other strict provenance mechanisms.

WaffleLapkin · 2022-07-08T01:14:59Z

I think opening a separate tracking issue would be a good idea, the "generic strict_provenance tracking issue" is already overloaded by itself.

RalfJung · 2022-07-08T11:53:04Z

library/core/src/sync/atomic.rs

+        #[cfg(not(bootstrap))]
+        // SAFETY: data races are prevented by atomic intrinsics.
+        unsafe {
+            atomic_add(self.p.get(), core::ptr::invalid_mut(val), order).cast()


If there's any chance that this could end up calling the intrinsic with a pointer type for the LHS and usize for the RHS, that would be great -- it would make it very clear from the types that it's only the left input that carries provenance.

WaffleLapkin · 2022-07-10T10:38:17Z

I've opened a tracking issue for this feature: #99108

rust-highfive assigned kennytm May 11, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2022

rust-highfive assigned dtolnay and unassigned kennytm May 11, 2022

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 May 11, 2022

RalfJung reviewed May 11, 2022

View reviewed changes

library/core/src/sync/atomic.rs Outdated Show resolved Hide resolved

arichardson reviewed May 14, 2022

View reviewed changes

library/core/tests/atomic.rs Show resolved Hide resolved

thomcc force-pushed the atomicptr-strict-prov branch from 9672218 to 02b2629 Compare May 14, 2022 19:37

taiki-e mentioned this pull request May 16, 2022

epoch: Remove ptr-to-int casts crossbeam-rs/crossbeam#796

Merged

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022

m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2022

thomcc added 2 commits July 1, 2022 06:21

Allow arithmetic and certain bitwise ops on AtomicPtr

2f872af

This is mainly to support migrating from AtomicUsize, for the strict provenance experiment. Fixes rust-lang#95492

Rename AtomicPtr::fetch_{add,sub}{,_bytes}

e65ecee

thomcc force-pushed the atomicptr-strict-prov branch from 02b2629 to e65ecee Compare July 1, 2022 13:22

thomcc mentioned this pull request Jul 1, 2022

Arithmetic and bitwise ops on AtomicPtr rust-lang/libs-team#60

Closed

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 1, 2022

RalfJung mentioned this pull request Jul 2, 2022

Miri can't track pointer through atomic bit-flip, which happens as usize rust-lang/miri#1993

Closed

dtolnay approved these changes Jul 6, 2022

View reviewed changes

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 6, 2022

GuillaumeGomez mentioned this pull request Jul 6, 2022

Rollup of 7 pull requests #98987

Merged

bors merged commit 4755173 into rust-lang:master Jul 6, 2022

rustbot added this to the 1.64.0 milestone Jul 6, 2022

RalfJung mentioned this pull request Jul 7, 2022

rustup; ptr atomics rust-lang/miri#2341

Merged

RalfJung reviewed Jul 7, 2022

View reviewed changes

RalfJung reviewed Jul 8, 2022

View reviewed changes

WaffleLapkin mentioned this pull request Jul 10, 2022

Tracking Issue for arithmetic and certain bitwise ops on AtomicPtr #99108

Open

3 tasks

thomcc mentioned this pull request Oct 12, 2022

Remove ptr from AtomicPtr::fetch_{add,sub} method names #102970

Closed

WaffleLapkin mentioned this pull request Oct 24, 2022

Rename AtomicPtr::fetch_ptr_{add,sub} to AtomicPtr::fetch_{add,sub} rust-lang/libs-team#126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow arithmetic and certain bitwise ops on AtomicPtr #96935

Allow arithmetic and certain bitwise ops on AtomicPtr #96935

thomcc commented May 11, 2022 •

edited

Loading

rust-highfive commented May 11, 2022

rust-highfive commented May 11, 2022

thomcc commented May 11, 2022

m-ou-se commented May 11, 2022

WaffleLapkin commented May 11, 2022

m-ou-se commented May 11, 2022 •

edited

Loading

thomcc commented May 14, 2022

m-ou-se commented May 14, 2022

m-ou-se commented May 14, 2022

thomcc commented May 14, 2022

WaffleLapkin commented May 15, 2022

thomcc commented May 15, 2022

thomcc commented May 16, 2022

thomcc commented Jun 30, 2022

rustbot commented Jul 1, 2022

thomcc commented Jul 1, 2022

thomcc commented Jul 1, 2022 •

edited

Loading

RalfJung commented Jul 2, 2022

dtolnay left a comment

dtolnay commented Jul 6, 2022

bors commented Jul 6, 2022

RalfJung Jul 7, 2022 •

edited

Loading

jonhoo commented Jul 7, 2022

WaffleLapkin commented Jul 8, 2022

RalfJung Jul 8, 2022

WaffleLapkin commented Jul 10, 2022

Allow arithmetic and certain bitwise ops on AtomicPtr #96935

Allow arithmetic and certain bitwise ops on AtomicPtr #96935

Conversation

thomcc commented May 11, 2022 • edited Loading

rust-highfive commented May 11, 2022

rust-highfive commented May 11, 2022

thomcc commented May 11, 2022

m-ou-se commented May 11, 2022

WaffleLapkin commented May 11, 2022

m-ou-se commented May 11, 2022 • edited Loading

thomcc commented May 14, 2022

m-ou-se commented May 14, 2022

m-ou-se commented May 14, 2022

thomcc commented May 14, 2022

WaffleLapkin commented May 15, 2022

thomcc commented May 15, 2022

thomcc commented May 16, 2022

thomcc commented Jun 30, 2022

rustbot commented Jul 1, 2022

thomcc commented Jul 1, 2022

thomcc commented Jul 1, 2022 • edited Loading

RalfJung commented Jul 2, 2022

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Jul 6, 2022

bors commented Jul 6, 2022

RalfJung Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

jonhoo commented Jul 7, 2022

WaffleLapkin commented Jul 8, 2022

RalfJung Jul 8, 2022

Choose a reason for hiding this comment

WaffleLapkin commented Jul 10, 2022

thomcc commented May 11, 2022 •

edited

Loading

m-ou-se commented May 11, 2022 •

edited

Loading

thomcc commented Jul 1, 2022 •

edited

Loading

RalfJung Jul 7, 2022 •

edited

Loading