Added Volatile Module. #12079

thehydroimpulse · 2014-02-07T00:32:29Z

This is the second attempt at submitted the PR (wrecked the last one #12063 trying to squash my commits).

As per the discussion on the previous PR:

VolatilePtr's store method will achieve ownership of the current data first. This will zero-out the memory; after which the new pointer will be written. The old data is then dropped (through an explicit drop call).
Added comments for the module definition describing what volatile is, with references to LLVM's docs and wikipedia.
Added comments describing the semantics of VolatilePtr.take.

huonw · 2014-02-07T00:47:14Z

src/libstd/unstable/volatile.rs

+    }
+}
+
+pub struct VolatilePtr<T> {


Can we just have Volatile<T> with priv data: T, using the same technique as VolatileInt VolatileBool? (And then they would just be Volatile<int> and Volatile<bool>, etc.)

I guess having VolatilePtr<T> too would be ok for external pointers, but...

I pushed a commit that changes it to that format.

alexcrichton · 2014-02-07T00:50:58Z

I agree with @huonw that I'm second-guessing the layout of this module. I think that we should have struct Volatile<T: Pod> { priv t: T } to support all copyable use cases (it has load and store), but I'm not sure about the pointer case.

@huonw has a good point in that if a major use case is interoping with something like memory-mapped I/O, the VolatilePtr as-is doesn't support that.

thehydroimpulse · 2014-02-07T01:03:59Z

@alexcrichton @huonw Good points. I definitely think Volatile<T> would be more appropriate.

Would you want a single wrapping type Volatile<T>? Or Volatile<T> and VolatilePtr<T>?

thehydroimpulse · 2014-02-07T02:33:51Z

@alexcrichton @huonw I removed the specific implementations such VolatileInt and replaced it with a more generic Volatile<T: Pod>.

I still need to address @huonw last point.

alexcrichton · 2014-02-07T17:58:38Z

src/libstd/unstable/volatile.rs

+ *
+ *  * `VolatileInt`: Provides semantics to work with valued integers.
+ *  * `VolatileBool`: Provides semantics to work with valued booleans.
+ e  * `VolatilePtr`: Provides semantics to work with pointers.


A stray e here.

thehydroimpulse · 2014-02-07T23:43:27Z

@alexcrichton Ok, I removed VolatilePtr and now everything is working under Volatile<T>. If a user wants to have a pointer to volatile data, they'll have to manage it yourself.

alexcrichton · 2014-02-07T23:50:32Z

src/libstd/unstable/volatile.rs

+ *         // Do something...
+ *     }
+ * }
+ * ```


I think this example is a little too misleading. This makes it sound like all loop variables need to be volatile, but that's not true.

Volatile memory accesses are normally required when memory can change where the compiler has no way of knowing it can change. For example, if i was a shared variable the compiler may assume that you were the only thread (and hence no one else could change it). Another case is memory-mapped I/O where the hardware may change the value each time you read it.

@alexcrichton Well, I should probably clarify that in the example above, nothing in the program/loop was changing i, thus, a compiler could assume it to be constant and factor the condition out, resulting in a static while true. However, in a situation where volatile variables are useful (like you stated), something else might of changed i.

Does that make sense?

flaper87 · 2014-02-07T23:50:40Z

@thehydroimpulse could you squash these commits?

alexcrichton · 2014-02-07T23:51:22Z

src/libstd/unstable/volatile.rs

+///
+/// ```
+/// let v: Volatile<int> = Volatile::new(10);
+/// static s: Volatile<bool> = Volatile { data: true };


This won't actually work because data is private. Additionally, can you put this in a rust code fence so it gets tested?

Ok. Added the rust code fence.

thehydroimpulse · 2014-02-08T00:02:22Z

@flaper87 Yes, I'll squash my commits after I make those changes.

thehydroimpulse · 2014-02-08T02:24:29Z

@flaper87 Squashed the commits.

huonw · 2014-02-08T02:31:18Z

src/libstd/unstable/volatile.rs

+/*!
+ * Volatile Module.
+ *
+ * The order of variables, and their execution isn't guaranteed. The compiler may


"The order of execution of code is not guaranteed: the compiler may reorder statements and instructions to make things more efficient."

thehydroimpulse · 2014-02-08T02:54:12Z

@huonw Fixed, and squashed my commits.

thehydroimpulse · 2014-02-09T01:34:33Z

@alexcrichton review?

alexcrichton · 2014-02-09T05:02:52Z

I was thinking this through recently, and I was curious about how useful this type will be. Sorry I don't think I thought this through when this was initially opened.

I would imagine that the purpose of this is to provide a form of safe wrapper around the intrinsics. You normally need volatile things when you have a form of memory which is changing beyond the knowledge of the compiler (I've only ever experienced volatile loads/stores with memory mapped I/O).

With that in mind, and especially with rust's linear type system, I'm not sure what exactly you'd use a Volatile<T> for. The compiler will prevent you from safely sharing this memory so something else could access it, and you can't safely cast a memory location to Volatile<T>.

I've been thinking that something which may be useful is:

fn volatile_load<T: Pod>(location: &T) -> T { ... }
fn volatile_store<T: Pod>(location: &mut T, t: T) { ... }

But I think that I'd need to think this through more to make sure that this is a useful interface that we'd want.

huonw · 2014-02-09T05:23:31Z

With that in mind, and especially with rust's linear type system, I'm not sure what exactly you'd use a Volatile for

Making sure you don't accidentally access something non-volatile-ly. e.g. in C

volatile int i; 
// all accesses are volatile automatically

but with only the wrappers for the intrinsics... the user has to remember to use them each time (i.e. remember to write volatile_store(&mut i, 1) instead of i = 1). However, I imagine hand written high-level wrappers would be used a lot too, due to custom requirements.

thehydroimpulse · 2014-02-09T05:23:59Z

@alexcrichton No worries. I think my initial goal was to allow an easier way to work with volatile variables. Previously, well, currently, you'd have to manually use the intrinsics whenever you touch the volatile variables. This is kind of a pain, especially compared to C. It's fairly easy to forget to call the intrinsics. Whereas if you'd have a type Volatile<T>, you can distinctly know that it's volatile from here on out.

That's why I thought wrapping around the intrinsics would be somewhat useful.

alexcrichton · 2014-02-09T06:32:16Z

@huonw, I agree, but I'm having trouble thinking of a use case of a safely-shared Volatile<T>. From my understanding, our linear type system makes it such that a volatile variable in safe circumstances isn't necessary.

I suppose I'm approaching this volatile type as a safe interface to the volatile intrinsics, but perhaps the interface can't be truly safe in all situations?

thehydroimpulse · 2014-02-11T02:12:16Z

@alexcrichton mmm. I'm trying to think of situations where we could have a safe volatile type (or where it'd be useful), but can't. Most of the volatile stuff I've done is with memory mapped I/O, and most of the stuff down there is inherently unsafe (mapping physical devices to memory, etc...).

Thoughts?

alexcrichton · 2014-02-11T02:24:38Z

That was my reasoning behind a safe volatile_load and volatile_store. The user will unsafely create &T or &mut T for memory-mapped I/O slots, and then they can use the safe wrappers to load/store from that memory location.

thehydroimpulse · 2014-02-11T02:54:41Z

@alexcrichton So, the only real benefit Volatile<T> would have is the strictness of working with volatile variables. Such that you don't need to remember to call volatile_load and volatile_store when accessing and storing variables and that you can easily identify when working with volatile variables or not.

Is that an appropriate benefit compared to the raw intrinsics? Or should we also include a safe volatile_load and volatile_store?

alexcrichton · 2014-02-11T17:06:16Z

The problem with Volatile<T> is that I don't believe that there's any situation with safe code in which you need a volatile variable. Something unsafe is probably happening if you need to use a volatile load/store, in which case having a safe wrapper won't buy you much.

nikomatsakis · 2014-02-11T19:05:37Z

On Tue, Feb 11, 2014 at 09:06:20AM -0800, Alex Crichton wrote:

The problem with Volatile<T> is that I don't believe that there's
any situation with safe code in which you need a volatile variable.

I'm not sure that's true. There are some simple cases where I can
imagine it being useful, such as having a bunch of threads exploring
some solution space (think traveling salesman problem or something
like that) and tracking a lower bound on solutions, used purely for
optimization purposes. In these kinds of cases, it's often ok to have
the threads write an integer to a shared variable that other threads
will read. These reads must be very cheap in order to be worthwhile,
since the whole goal of this is optimization. I guess one could use an
ARC of a Volatile to achieve this.

nikomatsakis · 2014-02-11T22:00:10Z

On Tue, Feb 11, 2014 at 11:59:13AM -0800, Alex Crichton wrote:

We can leave volatile_{load,store} as unsafe intrinsics, but we
can also provide safe wrappers around them for T: Pod when loading
from &T and storing into &mut T

I'm not sure I see the point. References like &T and &mut T should
always be thread-local and hence volatile reads are just overhead.

thehydroimpulse · 2014-02-14T07:37:26Z

Any updates on where this is headed? @nikomatsakis @alexcrichton

flaper87 · 2014-02-28T09:50:00Z

@alexcrichton @nikomatsakis Any final comment here?

@thehydroimpulse could you rebase this PR ?

thehydroimpulse · 2014-02-28T10:32:40Z

@flaper87 Yep, I'll rebase. I'm also assuming it'd be best to remove it from the unstable module just like the other ones in there? Then just mark the module as unstable.

nikomatsakis · 2014-02-28T10:58:37Z

I guess my willingness to merge depends on what happens if you try to do Volatile<Foo> where Foo is some multiword struct.

nikomatsakis · 2014-02-28T10:58:53Z

If it just results in a confusing error message, that's relatively ok and we can file a follow up bug. :)

thehydroimpulse · 2014-02-28T11:32:28Z

Error message, just for the sake of it:

Assertion failed: (getOperand(0)->getType() == 
cast<PointerType>(getOperand(1)->getType())->getElementType() && 
"Ptr must be a pointer to Val type!"), function AssertOK, file 
/Users/daniel/Projects/rust/src/llvm/lib/IR/Instructions.cpp, line 1085.

flaper87 · 2014-03-13T21:22:36Z

@thehydroimpulse Did you figure out what's happening here? Also, this patch may need a rebase. If you're running short in time let us know. Probably someone can move this work forward.

bill-myers · 2014-03-13T22:07:59Z

I think this should be implemented like Cell, using an Unsafe and taking &self for all operations, but using volatile_load/store instead of non-volatile (and restricted to single-word types, which however should include floating point and machine-supported SIMD types).

And perhaps there should be a read-only and a write-only version, for read-only and write-only hardware registers.

The use case is of course uncacheable memory-mapped devices; for cacheable memory, one should always use atomics instead.

thehydroimpulse · 2014-03-14T07:49:23Z

@flaper87 Yes. I'll be pushing my changes shortly.

nikomatsakis · 2014-03-17T19:59:33Z

Yes this should definitely use Unsafe<T>, once @flaper87's work lands.

thehydroimpulse · 2014-03-18T07:47:16Z

@nikomatsakis Should I hold off until @flaper87's work lands?

flaper87 · 2014-03-18T17:12:32Z

@thehydroimpulse I'd wait, yeah! FWIW, the patch will hopefully and this week. it's ready and I'm addressing some review feedback as we speak. In case you're interested / curious, this is the PR #12686

flaper87 · 2014-03-21T08:32:52Z

@thehydroimpulse the patch landed! You can now go ahead and rebase this one. Also, remember to make Volatile use Unsafe<T>

thehydroimpulse · 2014-03-23T07:02:03Z

@flaper87 Awesome. Thanks!

thehydroimpulse · 2014-03-28T09:43:18Z

@nikomatsakis I've addressed your previous comments (I guess they got deleted with a force push). I added the more constrained bounds (for i32 and u32 right now), used Unsafe<T>, and added a compile-fail test. I made the bound VolatileSafe + Pod to have the ability to copy the value (because of Unsafe<T>)

flaper87 · 2014-03-28T10:48:06Z

src/test/compile-fail/volatile-invalid-type.rs

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+


You need to add #[allow(experimental)];

UPDATE: could you pls add a description about what this test does?

flaper87 · 2014-03-28T11:03:06Z

@thehydroimpulse thanks for the update here. I added few comments. Could you also squash these commits when you're done? Thanks a lot again for the great work here!

* T is currently bound by the private VolatileSafe trait, which is only implemented for `i32` and `u32`. * Uses the volatile_load/volatile_store intrinsics. * User define-able types are not currently supported.

thehydroimpulse · 2014-03-28T11:46:30Z

@flaper87 Addressed the comments and squashed my commits.

nikomatsakis · 2014-03-28T18:01:33Z

Huh. So I realize I was thinking of something different when I thought of volatile, in that I was expecting we were targeting something more like Java's volatile, but I realize now that is basically what we call Atomic.

One thing is that this type should not be Share, as volatile loads and stores do not imply barriers, and it should use Unsafe<T>. As far as I know, volatile loads are needed precisely when the write isn't happening from within the current program but rather due to hardware DMA or some other such thing.

I'd like to see a realistic example that needs Volatile so I understand better how it is going to be used. The example in the doc comment isn't realistic because it doesn't show what is actually mutating the volatile variable. I expect we'll need some way to export a pointer to the contents of the volatile so that you can instruct the other process where to write to?

This has unfortunate interactions with our move semantics, in that if you export this pointer, and then move the value, weird things happen. It's kind of a borrow. Maybe we can better model it using an & borrow, in fact.

So long story short I'd like to understand better how this is to be used.

alexcrichton · 2014-04-07T23:07:00Z

Closing due to inactivity. I agree with @nikomatsakis, I think we need a compelling use case to guide this to make some more progress.

fix: `signature_help`: use corresponding param list for methods Close rust-lang#12079

Fix some typos changelog: none

huonw reviewed Feb 7, 2014
View reviewed changes

alexcrichton reviewed Feb 7, 2014
View reviewed changes

huonw reviewed Feb 8, 2014
View reviewed changes

flaper87 reviewed Mar 28, 2014
View reviewed changes

Added a Volatile<T> unstable (experimental) type.

2f0d8dc

* T is currently bound by the private VolatileSafe trait, which is only implemented for `i32` and `u32`. * Uses the volatile_load/volatile_store intrinsics. * User define-able types are not currently supported.

alexcrichton closed this Apr 7, 2014

huonw mentioned this pull request Apr 28, 2014

No way to emit volatile loads/stores #11172

Closed

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022

Auto merge of rust-lang#12082 - iDawer:ide.signature_help, r=lnicola

a8bc625

fix: `signature_help`: use corresponding param list for methods Close rust-lang#12079

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 11, 2024

Auto merge of rust-lang#12079 - yuxqiu:fix_typos, r=Manishearth

9eb2b22

Fix some typos changelog: none

Added Volatile Module. #12079

Added Volatile Module. #12079

Conversation

thehydroimpulse commented Feb 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 7, 2014

thehydroimpulse commented Feb 7, 2014

thehydroimpulse commented Feb 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehydroimpulse commented Feb 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaper87 commented Feb 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehydroimpulse commented Feb 8, 2014

thehydroimpulse commented Feb 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehydroimpulse commented Feb 8, 2014

thehydroimpulse commented Feb 9, 2014

alexcrichton commented Feb 9, 2014

huonw commented Feb 9, 2014

thehydroimpulse commented Feb 9, 2014

alexcrichton commented Feb 9, 2014

thehydroimpulse commented Feb 11, 2014

alexcrichton commented Feb 11, 2014

thehydroimpulse commented Feb 11, 2014

alexcrichton commented Feb 11, 2014

nikomatsakis commented Feb 11, 2014

nikomatsakis commented Feb 11, 2014

thehydroimpulse commented Feb 14, 2014

flaper87 commented Feb 28, 2014

thehydroimpulse commented Feb 28, 2014

nikomatsakis commented Feb 28, 2014

nikomatsakis commented Feb 28, 2014

thehydroimpulse commented Feb 28, 2014

flaper87 commented Mar 13, 2014

bill-myers commented Mar 13, 2014

thehydroimpulse commented Mar 14, 2014

nikomatsakis commented Mar 17, 2014

thehydroimpulse commented Mar 18, 2014

flaper87 commented Mar 18, 2014

flaper87 commented Mar 21, 2014

thehydroimpulse commented Mar 23, 2014

thehydroimpulse commented Mar 28, 2014

Choose a reason for hiding this comment

flaper87 commented Mar 28, 2014

thehydroimpulse commented Mar 28, 2014

nikomatsakis commented Mar 28, 2014

alexcrichton commented Apr 7, 2014