From 01c0723ef247fec4b85af203c7247b66e3817e1b Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Sat, 5 Mar 2016 14:51:24 -0500 Subject: [PATCH 1/6] add #[derive(Hash)] test for #21714 --- src/test/run-pass/deriving-hash.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/test/run-pass/deriving-hash.rs b/src/test/run-pass/deriving-hash.rs index a98cfa2393f1e..91bfc2f9201b7 100644 --- a/src/test/run-pass/deriving-hash.rs +++ b/src/test/run-pass/deriving-hash.rs @@ -12,6 +12,7 @@ #![feature(hash_default)] use std::hash::{Hash, SipHasher, Hasher}; +use std::mem::size_of; #[derive(Hash)] struct Person { @@ -24,12 +25,30 @@ struct Person { #[derive(Hash)] struct __H__H; #[derive(Hash)] enum Collision<__H> { __H { __H__H: __H } } +#[derive(Hash)] +enum E { A=1, B } + fn hash(t: &T) -> u64 { let mut s = SipHasher::new_with_keys(0, 0); t.hash(&mut s); s.finish() } +struct FakeHasher<'a>(&'a mut Vec); +impl<'a> Hasher for FakeHasher<'a> { + fn finish(&self) -> u64 { + unimplemented!() + } + + fn write(&mut self, bytes: &[u8]) { + self.0.extend(bytes); + } +} + +fn fake_hash(v: &mut Vec, e: E) { + e.hash(&mut FakeHasher(v)); +} + fn main() { let person1 = Person { id: 5, @@ -43,4 +62,11 @@ fn main() { }; assert_eq!(hash(&person1), hash(&person1)); assert!(hash(&person1) != hash(&person2)); + + // test #21714 + let mut va = vec![]; + let mut vb = vec![]; + fake_hash(&mut va, E::A); + fake_hash(&mut vb, E::B); + assert!(va != vb); } From c480b6a75df08cef48190e3c18eab26e99bae58c Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Mon, 7 Mar 2016 13:05:12 -0500 Subject: [PATCH 2/6] fix #21714 by using discriminant_value in #[derive(Hash)] This is the same approach taken in #24270, except that this should not be a breaking change because it only changes the output of hash functions, which nobody should be relying on. --- src/libsyntax_ext/deriving/hash.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/libsyntax_ext/deriving/hash.rs b/src/libsyntax_ext/deriving/hash.rs index ba38ebc860713..9368658648910 100644 --- a/src/libsyntax_ext/deriving/hash.rs +++ b/src/libsyntax_ext/deriving/hash.rs @@ -12,7 +12,7 @@ use deriving; use deriving::generic::*; use deriving::generic::ty::*; -use syntax::ast::{MetaItem, Expr, Mutability}; +use syntax::ast::{self, MetaItem, Expr, Mutability}; use syntax::codemap::Span; use syntax::ext::base::{ExtCtxt, Annotatable}; use syntax::ext::build::AstBuilder; @@ -81,15 +81,18 @@ fn hash_substructure(cx: &mut ExtCtxt, trait_span: Span, substr: &Substructure) let fields = match *substr.fields { Struct(_, ref fs) => fs, - EnumMatching(index, variant, ref fs) => { - // Determine the discriminant. We will feed this value to the byte - // iteration function. - let discriminant = match variant.node.disr_expr { - Some(ref d) => d.clone(), - None => cx.expr_usize(trait_span, index) - }; + EnumMatching(_, _, ref fs) => { + let path = cx.std_path(&["intrinsics", "discriminant_value"]); + let call = cx.expr_call_global( + trait_span, path, vec![cx.expr_self(trait_span)]); + let variant_value = cx.expr_block(P(ast::Block { + stmts: vec![], + expr: Some(call), + id: ast::DUMMY_NODE_ID, + rules: ast::BlockCheckMode::Unsafe(ast::CompilerGenerated), + span: trait_span })); - stmts.push(call_hash(trait_span, discriminant)); + stmts.push(call_hash(trait_span, variant_value)); fs } From b20e748ad88daee81488c1033971eec6b139d291 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Tue, 22 Mar 2016 18:17:57 -0400 Subject: [PATCH 3/6] mk: point target LD_LIBRARY_PATH at current stage --- mk/main.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mk/main.mk b/mk/main.mk index 9df04a6d43eac..10743ef2e257e 100644 --- a/mk/main.mk +++ b/mk/main.mk @@ -493,7 +493,7 @@ endif LD_LIBRARY_PATH_ENV_HOSTDIR$(1)_T_$(2)_H_$(3) := \ $$(CURDIR)/$$(HLIB$(1)_H_$(3)):$$(CFG_LLVM_INST_DIR_$(3))/lib LD_LIBRARY_PATH_ENV_TARGETDIR$(1)_T_$(2)_H_$(3) := \ - $$(CURDIR)/$$(TLIB1_T_$(2)_H_$(CFG_BUILD)) + $$(CURDIR)/$$(TLIB$(1)_T_$(2)_H_$(CFG_BUILD)) HOST_RPATH_VAR$(1)_T_$(2)_H_$(3) := \ $$(LD_LIBRARY_PATH_ENV_NAME$(1)_T_$(2)_H_$(3))=$$(LD_LIBRARY_PATH_ENV_HOSTDIR$(1)_T_$(2)_H_$(3)):$$$$$$(LD_LIBRARY_PATH_ENV_NAME$(1)_T_$(2)_H_$(3)) From dd5972ee354c3b76c1d34df8eb0cbd5e9a6b48e7 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Tue, 22 Mar 2016 18:18:30 -0400 Subject: [PATCH 4/6] mk: add missing dep compiletest=>log --- mk/crates.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mk/crates.mk b/mk/crates.mk index ae40a8e7f70dc..05018d2a94062 100644 --- a/mk/crates.mk +++ b/mk/crates.mk @@ -123,7 +123,7 @@ DEPS_rustdoc := rustc rustc_driver native:hoedown serialize getopts \ test rustc_lint rustc_front -TOOL_DEPS_compiletest := test getopts +TOOL_DEPS_compiletest := test getopts log TOOL_DEPS_rustdoc := rustdoc TOOL_DEPS_rustc := rustc_driver TOOL_DEPS_rustbook := std rustdoc From 1e67d8a57099eb9d286b5a4adfa798c1b3d437b9 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Thu, 10 Mar 2016 00:31:19 -0500 Subject: [PATCH 5/6] deriving: factor out discriminant_value construction --- src/libsyntax_ext/deriving/generic/mod.rs | 38 +++++++---------------- src/libsyntax_ext/deriving/hash.rs | 15 +++------ src/libsyntax_ext/deriving/mod.rs | 17 ++++++++++ 3 files changed, 33 insertions(+), 37 deletions(-) diff --git a/src/libsyntax_ext/deriving/generic/mod.rs b/src/libsyntax_ext/deriving/generic/mod.rs index 6dac2f5f9e515..7d452b14dae84 100644 --- a/src/libsyntax_ext/deriving/generic/mod.rs +++ b/src/libsyntax_ext/deriving/generic/mod.rs @@ -209,6 +209,8 @@ use syntax::ptr::P; use self::ty::{LifetimeBounds, Path, Ptr, PtrTy, Self_, Ty}; +use deriving; + pub mod ty; pub struct TraitDef<'a> { @@ -381,22 +383,6 @@ fn find_type_parameters(ty: &ast::Ty, ty_param_names: &[ast::Name]) -> Vec P { - let path = cx.std_path(&["intrinsics", "unreachable"]); - let call = cx.expr_call_global( - sp, path, vec![]); - let unreachable = cx.expr_block(P(ast::Block { - stmts: vec![], - expr: Some(call), - id: ast::DUMMY_NODE_ID, - rules: ast::BlockCheckMode::Unsafe(ast::CompilerGenerated), - span: sp })); - - unreachable -} - impl<'a> TraitDef<'a> { pub fn expand(&self, cx: &mut ExtCtxt, @@ -1279,15 +1265,11 @@ impl<'a> MethodDef<'a> { let mut first_ident = None; for (&ident, self_arg) in vi_idents.iter().zip(&self_args) { - let path = cx.std_path(&["intrinsics", "discriminant_value"]); - let call = cx.expr_call_global( - sp, path, vec![cx.expr_addr_of(sp, self_arg.clone())]); - let variant_value = cx.expr_block(P(ast::Block { - stmts: vec![], - expr: Some(call), - id: ast::DUMMY_NODE_ID, - rules: ast::BlockCheckMode::Unsafe(ast::CompilerGenerated), - span: sp })); + let self_addr = cx.expr_addr_of(sp, self_arg.clone()); + let variant_value = deriving::call_intrinsic(cx, + sp, + "discriminant_value", + vec![self_addr]); let target_ty = cx.ty_ident(sp, cx.ident_of(target_type_name)); let variant_disr = cx.expr_cast(sp, variant_value, target_ty); @@ -1315,7 +1297,9 @@ impl<'a> MethodDef<'a> { //Since we know that all the arguments will match if we reach the match expression we //add the unreachable intrinsics as the result of the catch all which should help llvm //in optimizing it - match_arms.push(cx.arm(sp, vec![cx.pat_wild(sp)], expr_unreachable_intrinsic(cx, sp))); + match_arms.push(cx.arm(sp, + vec![cx.pat_wild(sp)], + deriving::call_intrinsic(cx, sp, "unreachable", vec![]))); // Final wrinkle: the self_args are expressions that deref // down to desired l-values, but we cannot actually deref @@ -1391,7 +1375,7 @@ impl<'a> MethodDef<'a> { // derive Debug on such a type could here generate code // that needs the feature gate enabled.) - expr_unreachable_intrinsic(cx, sp) + deriving::call_intrinsic(cx, sp, "unreachable", vec![]) } else { diff --git a/src/libsyntax_ext/deriving/hash.rs b/src/libsyntax_ext/deriving/hash.rs index 9368658648910..c37ae116d379b 100644 --- a/src/libsyntax_ext/deriving/hash.rs +++ b/src/libsyntax_ext/deriving/hash.rs @@ -12,7 +12,7 @@ use deriving; use deriving::generic::*; use deriving::generic::ty::*; -use syntax::ast::{self, MetaItem, Expr, Mutability}; +use syntax::ast::{MetaItem, Expr, Mutability}; use syntax::codemap::Span; use syntax::ext::base::{ExtCtxt, Annotatable}; use syntax::ext::build::AstBuilder; @@ -82,15 +82,10 @@ fn hash_substructure(cx: &mut ExtCtxt, trait_span: Span, substr: &Substructure) let fields = match *substr.fields { Struct(_, ref fs) => fs, EnumMatching(_, _, ref fs) => { - let path = cx.std_path(&["intrinsics", "discriminant_value"]); - let call = cx.expr_call_global( - trait_span, path, vec![cx.expr_self(trait_span)]); - let variant_value = cx.expr_block(P(ast::Block { - stmts: vec![], - expr: Some(call), - id: ast::DUMMY_NODE_ID, - rules: ast::BlockCheckMode::Unsafe(ast::CompilerGenerated), - span: trait_span })); + let variant_value = deriving::call_intrinsic(cx, + trait_span, + "discriminant_value", + vec![cx.expr_self(trait_span)]); stmts.push(call_hash(trait_span, variant_value)); diff --git a/src/libsyntax_ext/deriving/mod.rs b/src/libsyntax_ext/deriving/mod.rs index 1774167e83000..92a141fb4ec86 100644 --- a/src/libsyntax_ext/deriving/mod.rs +++ b/src/libsyntax_ext/deriving/mod.rs @@ -18,6 +18,7 @@ use syntax::ext::build::AstBuilder; use syntax::feature_gate; use syntax::codemap::Span; use syntax::parse::token::{intern, intern_and_get_ident}; +use syntax::ptr::P; macro_rules! pathvec { ($($x:ident)::+) => ( @@ -271,3 +272,19 @@ fn hygienic_type_parameter(item: &Annotatable, base: &str) -> String { typaram } +/// Constructs an expression that calls an intrinsic +fn call_intrinsic(cx: &ExtCtxt, + span: Span, + intrinsic: &str, + args: Vec>) -> P { + let path = cx.std_path(&["intrinsics", intrinsic]); + let call = cx.expr_call_global(span, path, args); + + cx.expr_block(P(ast::Block { + stmts: vec![], + expr: Some(call), + id: ast::DUMMY_NODE_ID, + rules: ast::BlockCheckMode::Unsafe(ast::CompilerGenerated), + span: span })) +} + From 5954fce848a0105cdf2009278cd1b50daffb7c61 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sun, 27 Mar 2016 22:05:58 +0530 Subject: [PATCH 6/6] Improve concurrency chapter --- src/doc/book/concurrency.md | 142 +++++++++++++++++++++++++++++++----- 1 file changed, 124 insertions(+), 18 deletions(-) diff --git a/src/doc/book/concurrency.md b/src/doc/book/concurrency.md index 30e4ad7ba5b12..991654b2b8d43 100644 --- a/src/doc/book/concurrency.md +++ b/src/doc/book/concurrency.md @@ -94,6 +94,54 @@ fn main() { } ``` +As closures can capture variables from their environment, we can also try to +bring some data into the other thread: + +```rust,ignore +use std::thread; + +fn main() { + let x = 1; + thread::spawn(|| { + println!("x is {}", x); + }); +} +``` + +However, this gives us an error: + +```text +5:19: 7:6 error: closure may outlive the current function, but it + borrows `x`, which is owned by the current function +... +5:19: 7:6 help: to force the closure to take ownership of `x` (and any other referenced variables), + use the `move` keyword, as shown: + thread::spawn(move || { + println!("x is {}", x); + }); +``` + +This is because by default closures capture variables by reference, and thus the +closure only captures a _reference to `x`_. This is a problem, because the +thread may outlive the scope of `x`, leading to a dangling pointer. + +To fix this, we use a `move` closure as mentioned in the error message. `move` +closures are explained in depth [here](closures.html#move-closures); basically +they move variables from their environment into themselves. This means that `x` +is now owned by the closure, and cannot be used in `main()` after the call to +`spawn()`. + +```rust +use std::thread; + +fn main() { + let x = 1; + thread::spawn(move || { + println!("x is {}", x); + }); +} +``` + Many languages have the ability to execute threads, but it's wildly unsafe. There are entire books about how to prevent errors that occur from shared mutable state. Rust helps out with its type system here as well, by preventing @@ -145,23 +193,64 @@ This gives us an error: ``` Rust knows this wouldn't be safe! If we had a reference to `data` in each -thread, and the thread takes ownership of the reference, we'd have three -owners! +thread, and the thread takes ownership of the reference, we'd have three owners! +`data` gets moved out of `main` in the first call to `spawn()`, so subsequent +calls in the loop cannot use this variable. + +So, we need some type that lets us have more than one owning reference to a +value. Usually, we'd use `Rc` for this, which is a reference counted type +that provides shared ownership. It has some runtime bookkeeping that keeps track +of the number of references to it, hence the "reference count" part of its name. + +Calling `clone()` on an `Rc` will return a new owned reference and bump the +internal reference count. We create one of these for each thread: -So, we need some type that lets us have more than one reference to a value and -that we can share between threads, that is it must implement `Sync`. -We'll use `Arc`, Rust's standard atomic reference count type, which -wraps a value up with some extra runtime bookkeeping which allows us to -share the ownership of the value between multiple references at the same time. +```ignore +use std::thread; +use std::time::Duration; +use std::rc::Rc; -The bookkeeping consists of a count of how many of these references exist to -the value, hence the reference count part of the name. +fn main() { + let mut data = Rc::new(vec![1, 2, 3]); + + for i in 0..3 { + // create a new owned reference + let data_ref = data.clone(); + + // use it in a thread + thread::spawn(move || { + data_ref[i] += 1; + }); + } + + thread::sleep(Duration::from_millis(50)); +} +``` + +This won't work, however, and will give us the error: + +```text +13:9: 13:22 error: the trait `core::marker::Send` is not + implemented for the type `alloc::rc::Rc>` +... +13:9: 13:22 note: `alloc::rc::Rc>` + cannot be sent between threads safely +``` + +As the error message mentions, `Rc` cannot be sent between threads safely. This +is because the internal reference count is not maintained in a thread safe +matter and can have a data race. + +To solve this, we'll use `Arc`, Rust's standard atomic reference count type. The Atomic part means `Arc` can safely be accessed from multiple threads. To do this the compiler guarantees that mutations of the internal count use indivisible operations which can't have data races. +In essence, `Arc` is a type that lets us share ownership of data _across +threads_. + ```ignore use std::thread; @@ -182,7 +271,7 @@ fn main() { } ``` -We now call `clone()` on our `Arc`, which increases the internal count. +Similarly to las time, we use `clone()` to create a new owned handle. This handle is then moved into the new thread. And... still gives us an error. @@ -193,14 +282,21 @@ And... still gives us an error. ^~~~ ``` -`Arc` assumes one more property about its contents to ensure that it is safe -to share across threads: it assumes its contents are `Sync`. This is true for -our value if it's immutable, but we want to be able to mutate it, so we need -something else to persuade the borrow checker we know what we're doing. +`Arc by default has immutable contents. It allows the _sharing_ of data +between threads, but shared mutable data is unsafe and when threads are +involved can cause data races! + -It looks like we need some type that allows us to safely mutate a shared value, -for example a type that can ensure only one thread at a time is able to -mutate the value inside it at any one time. +Usually when we wish to make something in an immutable position mutable, we use +`Cell` or `RefCell` which allow safe mutation via runtime checks or +otherwise (see also: [Choosing Your Guarantees](choosing-your-guarantees.html)). +However, similar to `Rc`, these are not thread safe. If we try using these, we +will get an error about these types not being `Sync`, and the code will fail to +compile. + +It looks like we need some type that allows us to safely mutate a shared value +across threads, for example a type that can ensure only one thread at a time is +able to mutate the value inside it at any one time. For that, we can use the `Mutex` type! @@ -229,7 +325,17 @@ fn main() { Note that the value of `i` is bound (copied) to the closure and not shared among the threads. -Also note that [`lock`](../std/sync/struct.Mutex.html#method.lock) method of +We're "locking" the mutex here. A mutex (short for "mutual exclusion"), as +mentioned, only allows one thread at a time to access a value. When we wish to +access the value, we use `lock()` on it. This will "lock" the mutex, and no +other thread will be able to lock it (and hence, do anything with the value) +until we're done with it. If a thread attempts to lock a mutex which is already +locked, it will wait until the other thread releases the lock. + +The lock "release" here is implicit; when the result of the lock (in this case, +`data`) goes out of scope, the lock is automatically released. + +Note that [`lock`](../std/sync/struct.Mutex.html#method.lock) method of [`Mutex`](../std/sync/struct.Mutex.html) has this signature: ```ignore