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

Stabilize volatile read and write #1467

Merged
merged 2 commits into from Feb 18, 2016

Conversation

Projects
None yet
9 participants
@Amanieu
Copy link
Contributor

Amanieu commented Jan 18, 2016

This stabilizes the volatile_load and volatile_store intrinsics as ptr::read_volatile and ptr::write_volatile.

Rendered

@nrc nrc added the T-libs label Jan 18, 2016

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jan 19, 2016

I think this RFC proposes to do an obviously right change. There’s no compatibility hazard I can think of and the functions are very useful in certain cases.

👍

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Jan 19, 2016

I'd be happy to see these made stable, but are there any environments where these functions can be used without requiring other unstable things? Said another way, would stabilizing these functions make it so that some project can switch from nightly to stable?

I know that doesn't need to be a requirement, and we have to start somewhere.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 20, 2016

Seems like a good idea to me.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 21, 2016

As a bit of a bikeshed I might prefer to rename these to read_volatile and write_volatile (to group nicely with the existing read and write), but otherwise this seems fine to me. I believe it meshes well with the rest of the functions in the std::ptr module in terms of naming and signature.

I think the biggest question in terms of stabilization is what does volatile mean, and I agree with this RFC that it's well defined in C and other languages, so I would be fine having documentation redirecting to those relevant documents as well. I would perhaps feel best if we could hash out in this RFC exactly what semantics we'd point to (e.g. what documentation we'd point to) just to be extra sure it's ok, but that's not necessarily required in my opinion.

In the long run I think we'll eventually want a wrapper like Volatile<T> or something like that, but I'm more than ok deferring that and only providing the bare bones in the standard library to start out with (e.g. these functions). I think that you're able to build up everything else on top of these functions.

@alexcrichton alexcrichton self-assigned this Jan 21, 2016

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Jan 22, 2016

I renamed the functions to read_volatile/write_volatile and added a link to the LLVM definition of volatile.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 11, 2016

🔔 This RFC is now entering its week-long final comment period 🔔


My own personal opinion is that this is good to go!

@briansmith

This comment has been minimized.

Copy link

briansmith commented Feb 13, 2016

Please do not cite the LLVM documentation as the reference for the semantics. LLVM's documentation doesn't describe standard C semantics, but rather some semantics that are stronger, AFAICT, than what ISO C requires. If Rust wants to be different from ISO C11 then it would be better for Rust to specify what semantics it wants to have, and then verify whether LLVM implements (at least) the wanted semantics. The way this RFC looks now, it will be basically impossible to implement Rust without using LLVM, without reverse engineering LLVM.

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Feb 13, 2016

@briansmith note that this won't be the first Rust feature that is currently defined in terms of LLVM, nor do RFCs always have be complete specifications (i.e. we may want to learn more about how people want to use a feature before nailing down its semantics completely). Of course, having existing un(der)specified features doesn't automatically imply that we should add more. That said, I would expect documentation to not link to LLVM, but rather give some vague description about approximately what the semantics are (e.g. "volatile reads/writes are never elided nor reordered with other volatile reads/writes"), at least until Rust actually has a spec.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 18, 2016

The libs team discussed this RFC during triage yesterday and the conclusion was to merge, so I will do so! Thanks again for the RFC @Amanieu!

@alexcrichton alexcrichton merged commit 7f83957 into rust-lang:master Feb 18, 2016

bors added a commit to rust-lang/rust that referenced this pull request Feb 21, 2016

Auto merge of #31761 - Amanieu:volatile, r=alexcrichton
Tracking issue: #31756
RFC: rust-lang/rfcs#1467

I've made these unstable for now. Should they be stabilized straight away since we've had plenty of experience with people using the unstable intrinsics?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.