From 4b19c80819efbbf48a0efe0aba0e6e2fd3cafbc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Thu, 2 Jan 2020 00:19:29 +0100 Subject: [PATCH 01/13] Don't create strings in the fast path --- src/librustc_typeck/astconv.rs | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/librustc_typeck/astconv.rs b/src/librustc_typeck/astconv.rs index c15bcd81443d6..84b63e986d978 100644 --- a/src/librustc_typeck/astconv.rs +++ b/src/librustc_typeck/astconv.rs @@ -1318,10 +1318,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { // those that do. self.one_bound_for_assoc_type( || traits::supertraits(tcx, trait_ref), - &trait_ref.print_only_trait_path().to_string(), + || trait_ref.print_only_trait_path().to_string(), binding.item_name, path_span, - match binding.kind { + || match binding.kind { ConvertedBindingKind::Equality(ty) => Some(ty.to_string()), _ => None, }, @@ -1878,10 +1878,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { predicates.iter().filter_map(|(p, _)| p.to_opt_poly_trait_ref()), ) }, - ¶m_name.as_str(), + || param_name.to_string(), assoc_name, span, - None, + || None, ) } @@ -1890,10 +1890,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { fn one_bound_for_assoc_type( &self, all_candidates: impl Fn() -> I, - ty_param_name: &str, + ty_param_name: impl Fn() -> String, assoc_name: ast::Ident, span: Span, - is_equality: Option, + is_equality: impl Fn() -> Option, ) -> Result, ErrorReported> where I: Iterator>, @@ -1906,7 +1906,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { None => { self.complain_about_assoc_type_not_found( all_candidates, - ty_param_name, + &ty_param_name(), assoc_name, span, ); @@ -1919,6 +1919,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { if let Some(bound2) = matching_candidates.next() { debug!("one_bound_for_assoc_type: bound2 = {:?}", bound2); + let is_equality = is_equality(); let bounds = iter::once(bound).chain(iter::once(bound2)).chain(matching_candidates); let mut err = if is_equality.is_some() { // More specific Error Index entry. @@ -1928,7 +1929,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { E0222, "ambiguous associated type `{}` in bounds of `{}`", assoc_name, - ty_param_name + ty_param_name() ) } else { struct_span_err!( @@ -1937,7 +1938,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { E0221, "ambiguous associated type `{}` in bounds of `{}`", assoc_name, - ty_param_name + ty_param_name() ) }; err.span_label(span, format!("ambiguous associated type `{}`", assoc_name)); @@ -1975,7 +1976,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { "use fully qualified syntax to disambiguate", format!( "<{} as {}>::{}", - ty_param_name, + ty_param_name(), bound.print_only_trait_path(), assoc_name, ), @@ -1985,7 +1986,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { } else { err.note(&format!( "associated type `{}` could derive from `{}`", - ty_param_name, + ty_param_name(), bound.print_only_trait_path(), )); } @@ -1994,7 +1995,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { err.help(&format!( "consider introducing a new type parameter `T` and adding `where` constraints:\ \n where\n T: {},\n{}", - ty_param_name, + ty_param_name(), where_bounds.join(",\n"), )); } @@ -2108,10 +2109,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { self.one_bound_for_assoc_type( || traits::supertraits(tcx, ty::Binder::bind(trait_ref)), - "Self", + || "Self".to_string(), assoc_ident, span, - None, + || None, )? } (&ty::Param(_), Res::SelfTy(Some(param_did), None)) From edee9c3898bdc3e319d90322259e535affd6ae49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Thu, 2 Jan 2020 00:44:34 +0100 Subject: [PATCH 02/13] Lift using interners instead of in_arena --- src/librustc/ty/context.rs | 39 ++++++++++++++----------- src/librustc_data_structures/sharded.rs | 14 +++++++++ 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index 1b0b5fc4d078d..0c00f06b7b3f5 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -53,7 +53,7 @@ use rustc_hir::{ItemKind, ItemLocalId, ItemLocalMap, ItemLocalSet}; use arena::SyncDroplessArena; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::profiling::SelfProfilerRef; -use rustc_data_structures::sharded::ShardedHashMap; +use rustc_data_structures::sharded::{IntoPointer, ShardedHashMap}; use rustc_data_structures::stable_hasher::{ hash_stable_hashmap, HashStable, StableHasher, StableVec, }; @@ -1560,11 +1560,11 @@ pub trait Lift<'tcx>: fmt::Debug { } macro_rules! nop_lift { - ($ty:ty => $lifted:ty) => { + ($set:ident; $ty:ty => $lifted:ty) => { impl<'a, 'tcx> Lift<'tcx> for $ty { type Lifted = $lifted; fn lift_to_tcx(&self, tcx: TyCtxt<'tcx>) -> Option { - if tcx.interners.arena.in_arena(*self as *const _) { + if tcx.interners.$set.contains_pointer_to(&Interned(*self)) { Some(unsafe { mem::transmute(*self) }) } else { None @@ -1575,14 +1575,14 @@ macro_rules! nop_lift { } macro_rules! nop_list_lift { - ($ty:ty => $lifted:ty) => { + ($set:ident; $ty:ty => $lifted:ty) => { impl<'a, 'tcx> Lift<'tcx> for &'a List<$ty> { type Lifted = &'tcx List<$lifted>; fn lift_to_tcx(&self, tcx: TyCtxt<'tcx>) -> Option { if self.is_empty() { return Some(List::empty()); } - if tcx.interners.arena.in_arena(*self as *const _) { + if tcx.interners.$set.contains_pointer_to(&Interned(*self)) { Some(unsafe { mem::transmute(*self) }) } else { None @@ -1592,21 +1592,21 @@ macro_rules! nop_list_lift { }; } -nop_lift! {Ty<'a> => Ty<'tcx>} -nop_lift! {Region<'a> => Region<'tcx>} -nop_lift! {Goal<'a> => Goal<'tcx>} -nop_lift! {&'a Const<'a> => &'tcx Const<'tcx>} +nop_lift! {type_; Ty<'a> => Ty<'tcx>} +nop_lift! {region; Region<'a> => Region<'tcx>} +nop_lift! {goal; Goal<'a> => Goal<'tcx>} +nop_lift! {const_; &'a Const<'a> => &'tcx Const<'tcx>} -nop_list_lift! {Goal<'a> => Goal<'tcx>} -nop_list_lift! {Clause<'a> => Clause<'tcx>} -nop_list_lift! {Ty<'a> => Ty<'tcx>} -nop_list_lift! {ExistentialPredicate<'a> => ExistentialPredicate<'tcx>} -nop_list_lift! {Predicate<'a> => Predicate<'tcx>} -nop_list_lift! {CanonicalVarInfo => CanonicalVarInfo} -nop_list_lift! {ProjectionKind => ProjectionKind} +nop_list_lift! {goal_list; Goal<'a> => Goal<'tcx>} +nop_list_lift! {clauses; Clause<'a> => Clause<'tcx>} +nop_list_lift! {type_list; Ty<'a> => Ty<'tcx>} +nop_list_lift! {existential_predicates; ExistentialPredicate<'a> => ExistentialPredicate<'tcx>} +nop_list_lift! {predicates; Predicate<'a> => Predicate<'tcx>} +nop_list_lift! {canonical_var_infos; CanonicalVarInfo => CanonicalVarInfo} +nop_list_lift! {projs; ProjectionKind => ProjectionKind} // This is the impl for `&'a InternalSubsts<'a>`. -nop_list_lift! {GenericArg<'a> => GenericArg<'tcx>} +nop_list_lift! {substs; GenericArg<'a> => GenericArg<'tcx>} pub mod tls { use super::{ptr_eq, GlobalCtxt, TyCtxt}; @@ -1930,6 +1930,11 @@ impl<'tcx, T: 'tcx + ?Sized> Clone for Interned<'tcx, T> { } impl<'tcx, T: 'tcx + ?Sized> Copy for Interned<'tcx, T> {} +impl<'tcx, T: 'tcx + ?Sized> IntoPointer for Interned<'tcx, T> { + fn into_pointer(&self) -> *const () { + self.0 as *const _ as *const () + } +} // N.B., an `Interned` compares and hashes as a `TyKind`. impl<'tcx> PartialEq for Interned<'tcx, TyS<'tcx>> { fn eq(&self, other: &Interned<'tcx, TyS<'tcx>>) -> bool { diff --git a/src/librustc_data_structures/sharded.rs b/src/librustc_data_structures/sharded.rs index 8b85d97a1d4fe..ee3f88ff1675f 100644 --- a/src/librustc_data_structures/sharded.rs +++ b/src/librustc_data_structures/sharded.rs @@ -137,6 +137,20 @@ impl ShardedHashMap { } } +pub trait IntoPointer { + /// Returns a pointer which outlives `self`. + fn into_pointer(&self) -> *const (); +} + +impl ShardedHashMap { + pub fn contains_pointer_to(&self, value: &T) -> bool { + let hash = make_hash(&value); + let shard = self.get_shard_by_hash(hash).lock(); + let value = value.into_pointer(); + shard.raw_entry().from_hash(hash, |entry| entry.into_pointer() == value).is_some() + } +} + #[inline] fn make_hash(val: &K) -> u64 { let mut state = FxHasher::default(); From b21c9dddb061faf2d0de0c05aa2c26ad295b470e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Thu, 2 Jan 2020 01:26:18 +0100 Subject: [PATCH 03/13] Use Arena for interning --- src/librustc/arena.rs | 5 ++++- src/librustc/ty/context.rs | 20 ++++---------------- src/librustc/ty/mod.rs | 10 ++++++---- src/librustc_interface/passes.rs | 4 +--- src/librustc_interface/queries.rs | 5 +---- 5 files changed, 16 insertions(+), 28 deletions(-) diff --git a/src/librustc/arena.rs b/src/librustc/arena.rs index cb3fdff53a3b0..15e92d8d84219 100644 --- a/src/librustc/arena.rs +++ b/src/librustc/arena.rs @@ -123,6 +123,9 @@ macro_rules! arena_types { [few] inferred_outlives_crate: rustc::ty::CratePredicatesMap<'tcx>, [] upvars: rustc_data_structures::fx::FxIndexMap, + // Interned types + [] tys: rustc::ty::TyS<$tcx>, + // HIR types [few] hir_forest: rustc::hir::map::Forest<$tcx>, [] arm: rustc_hir::Arm<$tcx>, @@ -176,7 +179,7 @@ macro_rules! declare_arena { ([], [$($a:tt $name:ident: $ty:ty,)*], $tcx:lifetime) => { #[derive(Default)] pub struct Arena<$tcx> { - dropless: DroplessArena, + pub dropless: DroplessArena, drop: DropArena, $($name: arena_for_type!($a[$ty]),)* } diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index 0c00f06b7b3f5..908fd9a529401 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -50,7 +50,6 @@ use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, DefIdSet, DefIndex, LOCAL_CRA use rustc_hir::{HirId, Node, TraitCandidate}; use rustc_hir::{ItemKind, ItemLocalId, ItemLocalMap, ItemLocalSet}; -use arena::SyncDroplessArena; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::profiling::SelfProfilerRef; use rustc_data_structures::sharded::{IntoPointer, ShardedHashMap}; @@ -81,21 +80,11 @@ use syntax::ast; use syntax::attr; use syntax::expand::allocator::AllocatorKind; -pub struct AllArenas { - pub interner: SyncDroplessArena, -} - -impl AllArenas { - pub fn new() -> Self { - AllArenas { interner: SyncDroplessArena::default() } - } -} - type InternedSet<'tcx, T> = ShardedHashMap, ()>; pub struct CtxtInterners<'tcx> { /// The arena that types, regions, etc. are allocated from. - arena: &'tcx SyncDroplessArena, + arena: &'tcx WorkerLocal>, /// Specifically use a speedy hash algorithm for these hash sets, since /// they're accessed quite often. @@ -115,7 +104,7 @@ pub struct CtxtInterners<'tcx> { } impl<'tcx> CtxtInterners<'tcx> { - fn new(arena: &'tcx SyncDroplessArena) -> CtxtInterners<'tcx> { + fn new(arena: &'tcx WorkerLocal>) -> CtxtInterners<'tcx> { CtxtInterners { arena, type_: Default::default(), @@ -1118,7 +1107,6 @@ impl<'tcx> TyCtxt<'tcx> { lint_store: Lrc, local_providers: ty::query::Providers<'tcx>, extern_providers: ty::query::Providers<'tcx>, - arenas: &'tcx AllArenas, arena: &'tcx WorkerLocal>, resolutions: ty::ResolverOutputs, hir: hir_map::Map<'tcx>, @@ -1129,7 +1117,7 @@ impl<'tcx> TyCtxt<'tcx> { let data_layout = TargetDataLayout::parse(&s.target.target).unwrap_or_else(|err| { s.fatal(&err); }); - let interners = CtxtInterners::new(&arenas.interner); + let interners = CtxtInterners::new(arena); let common_types = CommonTypes::new(&interners); let common_lifetimes = CommonLifetimes::new(&interners); let common_consts = CommonConsts::new(&interners, &common_types); @@ -2087,7 +2075,7 @@ macro_rules! slice_interners { $(impl<'tcx> TyCtxt<'tcx> { pub fn $method(self, v: &[$ty]) -> &'tcx List<$ty> { self.interners.$field.intern_ref(v, || { - Interned(List::from_arena(&self.interners.arena, v)) + Interned(List::from_arena(&*self.arena, v)) }).0 } })+ diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index d1e37a4ea1151..78b00f259aa64 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -6,6 +6,7 @@ pub use self::BorrowKind::*; pub use self::IntVarValue::*; pub use self::Variance::*; +use crate::arena::Arena; use crate::hir::exports::ExportMap; use crate::hir::map as hir_map; @@ -26,7 +27,6 @@ use crate::ty::layout::VariantIdx; use crate::ty::subst::{InternalSubsts, Subst, SubstsRef}; use crate::ty::util::{Discr, IntTypeExt}; use crate::ty::walk::TypeWalker; -use arena::SyncDroplessArena; use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::fx::FxIndexMap; @@ -76,7 +76,7 @@ pub use crate::ty::diagnostics::*; pub use self::binding::BindingMode; pub use self::binding::BindingMode::*; -pub use self::context::{keep_local, tls, AllArenas, FreeRegionInfo, TyCtxt}; +pub use self::context::{keep_local, tls, FreeRegionInfo, TyCtxt}; pub use self::context::{ CanonicalUserType, CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations, ResolvedOpaqueTy, UserType, UserTypeAnnotationIndex, @@ -606,7 +606,7 @@ unsafe impl Sync for List {} impl List { #[inline] - fn from_arena<'tcx>(arena: &'tcx SyncDroplessArena, slice: &[T]) -> &'tcx List { + fn from_arena<'tcx>(arena: &'tcx Arena<'tcx>, slice: &[T]) -> &'tcx List { assert!(!mem::needs_drop::()); assert!(mem::size_of::() != 0); assert!(slice.len() != 0); @@ -619,7 +619,9 @@ impl List { let size = offset + slice.len() * mem::size_of::(); - let mem = arena.alloc_raw(size, cmp::max(mem::align_of::(), mem::align_of::())); + let mem = arena + .dropless + .alloc_raw(size, cmp::max(mem::align_of::(), mem::align_of::())); unsafe { let result = &mut *(mem.as_mut_ptr() as *mut List); // Write the length diff --git a/src/librustc_interface/passes.rs b/src/librustc_interface/passes.rs index 9119466cbc048..5567d5bf201a0 100644 --- a/src/librustc_interface/passes.rs +++ b/src/librustc_interface/passes.rs @@ -15,7 +15,7 @@ use rustc::session::search_paths::PathKind; use rustc::session::Session; use rustc::traits; use rustc::ty::steal::Steal; -use rustc::ty::{self, AllArenas, GlobalCtxt, ResolverOutputs, TyCtxt}; +use rustc::ty::{self, GlobalCtxt, ResolverOutputs, TyCtxt}; use rustc::util::common::ErrorReported; use rustc_builtin_macros; use rustc_codegen_ssa::back::link::emit_metadata; @@ -711,7 +711,6 @@ pub fn create_global_ctxt<'tcx>( outputs: OutputFilenames, crate_name: &str, global_ctxt: &'tcx Once>, - all_arenas: &'tcx AllArenas, arena: &'tcx WorkerLocal>, ) -> QueryContext<'tcx> { let sess = &compiler.session(); @@ -742,7 +741,6 @@ pub fn create_global_ctxt<'tcx>( lint_store, local_providers, extern_providers, - &all_arenas, arena, resolver_outputs, hir_map, diff --git a/src/librustc_interface/queries.rs b/src/librustc_interface/queries.rs index 7de1c36ce4b2e..f0d0297d11a1e 100644 --- a/src/librustc_interface/queries.rs +++ b/src/librustc_interface/queries.rs @@ -9,7 +9,7 @@ use rustc::lint::LintStore; use rustc::session::config::{OutputFilenames, OutputType}; use rustc::session::Session; use rustc::ty::steal::Steal; -use rustc::ty::{AllArenas, GlobalCtxt, ResolverOutputs}; +use rustc::ty::{GlobalCtxt, ResolverOutputs}; use rustc::util::common::ErrorReported; use rustc_codegen_utils::codegen_backend::CodegenBackend; use rustc_data_structures::sync::{Lrc, Once, WorkerLocal}; @@ -67,7 +67,6 @@ pub struct Queries<'tcx> { compiler: &'tcx Compiler, gcx: Once>, - all_arenas: AllArenas, arena: WorkerLocal>, dep_graph_future: Query>, @@ -87,7 +86,6 @@ impl<'tcx> Queries<'tcx> { Queries { compiler, gcx: Once::new(), - all_arenas: AllArenas::new(), arena: WorkerLocal::new(|_| Arena::default()), dep_graph_future: Default::default(), parse: Default::default(), @@ -266,7 +264,6 @@ impl<'tcx> Queries<'tcx> { outputs, &crate_name, &self.gcx, - &self.all_arenas, &self.arena, )) }) From f4968c8e00daed7cc4e5658a7abe61557abd4570 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Thu, 2 Jan 2020 01:28:59 +0100 Subject: [PATCH 04/13] Remove SyncTypedArena, SyncDroplessArena and in_arena --- src/libarena/lib.rs | 73 --------------------------------------------- 1 file changed, 73 deletions(-) diff --git a/src/libarena/lib.rs b/src/libarena/lib.rs index beb0bac17d2ea..2a3d92edc4956 100644 --- a/src/libarena/lib.rs +++ b/src/libarena/lib.rs @@ -21,7 +21,6 @@ extern crate alloc; use rustc_data_structures::cold_path; -use rustc_data_structures::sync::MTLock; use smallvec::SmallVec; use std::cell::{Cell, RefCell}; @@ -116,11 +115,6 @@ impl Default for TypedArena { } impl TypedArena { - pub fn in_arena(&self, ptr: *const T) -> bool { - let ptr = ptr as *const T as *mut T; - - self.chunks.borrow().iter().any(|chunk| chunk.start() <= ptr && ptr < chunk.end()) - } /// Allocates an object in the `TypedArena`, returning a reference to it. #[inline] pub fn alloc(&self, object: T) -> &mut T { @@ -334,12 +328,6 @@ impl Default for DroplessArena { } impl DroplessArena { - pub fn in_arena(&self, ptr: *const T) -> bool { - let ptr = ptr as *const u8 as *mut u8; - - self.chunks.borrow().iter().any(|chunk| chunk.start() <= ptr && ptr < chunk.end()) - } - #[inline] fn align(&self, align: usize) { let final_address = ((self.ptr.get() as usize) + align - 1) & !(align - 1); @@ -500,66 +488,5 @@ impl DroplessArena { } } -#[derive(Default)] -// FIXME(@Zoxc): this type is entirely unused in rustc -pub struct SyncTypedArena { - lock: MTLock>, -} - -impl SyncTypedArena { - #[inline(always)] - pub fn alloc(&self, object: T) -> &mut T { - // Extend the lifetime of the result since it's limited to the lock guard - unsafe { &mut *(self.lock.lock().alloc(object) as *mut T) } - } - - #[inline(always)] - pub fn alloc_slice(&self, slice: &[T]) -> &mut [T] - where - T: Copy, - { - // Extend the lifetime of the result since it's limited to the lock guard - unsafe { &mut *(self.lock.lock().alloc_slice(slice) as *mut [T]) } - } - - #[inline(always)] - pub fn clear(&mut self) { - self.lock.get_mut().clear(); - } -} - -#[derive(Default)] -pub struct SyncDroplessArena { - lock: MTLock, -} - -impl SyncDroplessArena { - #[inline(always)] - pub fn in_arena(&self, ptr: *const T) -> bool { - self.lock.lock().in_arena(ptr) - } - - #[inline(always)] - pub fn alloc_raw(&self, bytes: usize, align: usize) -> &mut [u8] { - // Extend the lifetime of the result since it's limited to the lock guard - unsafe { &mut *(self.lock.lock().alloc_raw(bytes, align) as *mut [u8]) } - } - - #[inline(always)] - pub fn alloc(&self, object: T) -> &mut T { - // Extend the lifetime of the result since it's limited to the lock guard - unsafe { &mut *(self.lock.lock().alloc(object) as *mut T) } - } - - #[inline(always)] - pub fn alloc_slice(&self, slice: &[T]) -> &mut [T] - where - T: Copy, - { - // Extend the lifetime of the result since it's limited to the lock guard - unsafe { &mut *(self.lock.lock().alloc_slice(slice) as *mut [T]) } - } -} - #[cfg(test)] mod tests; From 4dbae1e8baf5c42b42aa2ab4d1104ec019b75b83 Mon Sep 17 00:00:00 2001 From: Stein Somers Date: Fri, 17 Jan 2020 01:05:49 +0100 Subject: [PATCH 05/13] Allow added string.insert benchmarks to compile --- src/liballoc/benches/string.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/liballoc/benches/string.rs b/src/liballoc/benches/string.rs index 599c8b1682851..5c95160ba2d14 100644 --- a/src/liballoc/benches/string.rs +++ b/src/liballoc/benches/string.rs @@ -1,5 +1,5 @@ use std::iter::repeat; -use test::Bencher; +use test::{black_box, Bencher}; #[bench] fn bench_with_capacity(b: &mut Bencher) { From 19fdc6e091af973a378d53c13bb1ea42862171ad Mon Sep 17 00:00:00 2001 From: Phoebe Bell Date: Tue, 19 Nov 2019 18:28:32 -0800 Subject: [PATCH 06/13] Document unsafe blocks in core::{cell, str, sync} --- src/libcore/cell.rs | 12 +++++++++-- src/libcore/str/lossy.rs | 6 ++++-- src/libcore/str/mod.rs | 44 ++++++++++++++++++++++++++++++-------- src/libcore/str/pattern.rs | 13 +++++++++-- src/libcore/sync/atomic.rs | 33 ++++++++++++++++++++++++++-- 5 files changed, 91 insertions(+), 17 deletions(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index 4fcd0d9737d56..c530432f802db 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -187,8 +187,6 @@ //! ``` //! -// ignore-tidy-undocumented-unsafe - #![stable(feature = "rust1", since = "1.0.0")] use crate::cmp::Ordering; @@ -368,6 +366,7 @@ impl Cell { if ptr::eq(self, other) { return; } + // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe unsafe { ptr::swap(self.value.get(), other.value.get()); } @@ -387,6 +386,7 @@ impl Cell { /// ``` #[stable(feature = "move_cell", since = "1.17.0")] pub fn replace(&self, val: T) -> T { + // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe mem::replace(unsafe { &mut *self.value.get() }, val) } @@ -423,6 +423,7 @@ impl Cell { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn get(&self) -> T { + // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe unsafe { *self.value.get() } } @@ -491,6 +492,7 @@ impl Cell { #[inline] #[stable(feature = "cell_get_mut", since = "1.11.0")] pub fn get_mut(&mut self) -> &mut T { + // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe unsafe { &mut *self.value.get() } } @@ -510,6 +512,7 @@ impl Cell { #[inline] #[stable(feature = "as_cell", since = "1.37.0")] pub fn from_mut(t: &mut T) -> &Cell { + // SAFETY: `&mut` ensures unique access unsafe { &*(t as *mut T as *const Cell) } } } @@ -553,6 +556,7 @@ impl Cell<[T]> { /// ``` #[stable(feature = "as_cell", since = "1.37.0")] pub fn as_slice_of_cells(&self) -> &[Cell] { + // SAFETY: `Cell` has the same memory layout as `T` unsafe { &*(self as *const Cell<[T]> as *const [Cell]) } } } @@ -816,6 +820,8 @@ impl RefCell { #[inline] pub fn try_borrow(&self) -> Result, BorrowError> { match BorrowRef::new(&self.borrow) { + // SAFETY: `BorrowRef` ensures that there is only immutable access + // to the value while borrowed Some(b) => Ok(Ref { value: unsafe { &*self.value.get() }, borrow: b }), None => Err(BorrowError { _private: () }), } @@ -891,6 +897,7 @@ impl RefCell { #[inline] pub fn try_borrow_mut(&self) -> Result, BorrowMutError> { match BorrowRefMut::new(&self.borrow) { + // SAFETY: `BorrowRef` gurantees unique access Some(b) => Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b }), None => Err(BorrowMutError { _private: () }), } @@ -940,6 +947,7 @@ impl RefCell { #[inline] #[stable(feature = "cell_get_mut", since = "1.11.0")] pub fn get_mut(&mut self) -> &mut T { + // SAFETY: `&mut` guarantees unique access unsafe { &mut *self.value.get() } } diff --git a/src/libcore/str/lossy.rs b/src/libcore/str/lossy.rs index 8a1fb9de54667..9d2e38734ef02 100644 --- a/src/libcore/str/lossy.rs +++ b/src/libcore/str/lossy.rs @@ -3,8 +3,6 @@ use crate::fmt::{self, Write}; use crate::mem; use crate::str as core_str; -// ignore-tidy-undocumented-unsafe - /// Lossy UTF-8 string. #[unstable(feature = "str_internals", issue = "none")] pub struct Utf8Lossy { @@ -17,6 +15,7 @@ impl Utf8Lossy { } pub fn from_bytes(bytes: &[u8]) -> &Utf8Lossy { + // SAFETY: both use the same memory layout, and UTF-8 correctness isn't required unsafe { mem::transmute(bytes) } } @@ -60,6 +59,7 @@ impl<'a> Iterator for Utf8LossyChunksIter<'a> { while i < self.source.len() { let i_ = i; + // SAFETY: 0 <= i < self.source.len() let byte = unsafe { *self.source.get_unchecked(i) }; i += 1; @@ -69,6 +69,7 @@ impl<'a> Iterator for Utf8LossyChunksIter<'a> { macro_rules! error { () => {{ + // SAFETY: we have checked up to `i` that source is valid UTF-8 unsafe { let r = Utf8LossyChunk { valid: core_str::from_utf8_unchecked(&self.source[0..i_]), @@ -130,6 +131,7 @@ impl<'a> Iterator for Utf8LossyChunksIter<'a> { } let r = Utf8LossyChunk { + // SAFETY: we have checked that the entire source is valid UTF-8 valid: unsafe { core_str::from_utf8_unchecked(self.source) }, broken: &[], }; diff --git a/src/libcore/str/mod.rs b/src/libcore/str/mod.rs index ab771b1164bad..3139d6188e27f 100644 --- a/src/libcore/str/mod.rs +++ b/src/libcore/str/mod.rs @@ -1,5 +1,4 @@ // ignore-tidy-filelength -// ignore-tidy-undocumented-unsafe //! String manipulation. //! @@ -341,6 +340,7 @@ impl Utf8Error { #[stable(feature = "rust1", since = "1.0.0")] pub fn from_utf8(v: &[u8]) -> Result<&str, Utf8Error> { run_utf8_validation(v)?; + // SAFETY: just ran validation Ok(unsafe { from_utf8_unchecked(v) }) } @@ -379,6 +379,7 @@ pub fn from_utf8(v: &[u8]) -> Result<&str, Utf8Error> { #[stable(feature = "str_mut_extras", since = "1.20.0")] pub fn from_utf8_mut(v: &mut [u8]) -> Result<&mut str, Utf8Error> { run_utf8_validation(v)?; + // SAFETY: just ran validation Ok(unsafe { from_utf8_unchecked_mut(v) }) } @@ -581,7 +582,7 @@ impl<'a> Iterator for Chars<'a> { #[inline] fn next(&mut self) -> Option { next_code_point(&mut self.iter).map(|ch| { - // str invariant says `ch` is a valid Unicode Scalar Value + // SAFETY: str invariant says `ch` is a valid Unicode Scalar Value unsafe { char::from_u32_unchecked(ch) } }) } @@ -628,7 +629,7 @@ impl<'a> DoubleEndedIterator for Chars<'a> { #[inline] fn next_back(&mut self) -> Option { next_code_point_reverse(&mut self.iter).map(|ch| { - // str invariant says `ch` is a valid Unicode Scalar Value + // SAFETY: str invariant says `ch` is a valid Unicode Scalar Value unsafe { char::from_u32_unchecked(ch) } }) } @@ -658,6 +659,7 @@ impl<'a> Chars<'a> { #[stable(feature = "iter_to_slice", since = "1.4.0")] #[inline] pub fn as_str(&self) -> &'a str { + // SAFETY: Chars is only made from a str, which guarantees the iter is valid utf8 unsafe { from_utf8_unchecked(self.iter.as_slice()) } } } @@ -1102,6 +1104,7 @@ impl<'a, P: Pattern<'a>> SplitInternal<'a, P> { fn get_end(&mut self) -> Option<&'a str> { if !self.finished && (self.allow_trailing_empty || self.end - self.start > 0) { self.finished = true; + // SAFETY: `self.start` and `self.end` always lie on unicode boundaries unsafe { let string = self.matcher.haystack().get_unchecked(self.start..self.end); Some(string) @@ -1119,6 +1122,7 @@ impl<'a, P: Pattern<'a>> SplitInternal<'a, P> { let haystack = self.matcher.haystack(); match self.matcher.next_match() { + // SAFETY: `Searcher` guarantees that `a` and `b` lie on unicode boundaries Some((a, b)) => unsafe { let elt = haystack.get_unchecked(self.start..a); self.start = b; @@ -1151,11 +1155,13 @@ impl<'a, P: Pattern<'a>> SplitInternal<'a, P> { let haystack = self.matcher.haystack(); match self.matcher.next_match_back() { + // SAFETY: `Searcher` guarantees that `a` and `b` lie on unicode boundaries Some((a, b)) => unsafe { let elt = haystack.get_unchecked(b..self.end); self.end = a; Some(elt) }, + // SAFETY: `self.start` and `self.end` always lie on unicode boundaries None => unsafe { self.finished = true; Some(haystack.get_unchecked(self.start..self.end)) @@ -1295,6 +1301,7 @@ where impl<'a, P: Pattern<'a>> MatchIndicesInternal<'a, P> { #[inline] fn next(&mut self) -> Option<(usize, &'a str)> { + // SAFETY: `Searcher` guaratees that `start` and `end` lie on unicode boundaries self.0 .next_match() .map(|(start, end)| unsafe { (start, self.0.haystack().get_unchecked(start..end)) }) @@ -1305,6 +1312,7 @@ impl<'a, P: Pattern<'a>> MatchIndicesInternal<'a, P> { where P::Searcher: ReverseSearcher<'a>, { + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries self.0 .next_match_back() .map(|(start, end)| unsafe { (start, self.0.haystack().get_unchecked(start..end)) }) @@ -1348,6 +1356,7 @@ where impl<'a, P: Pattern<'a>> MatchesInternal<'a, P> { #[inline] fn next(&mut self) -> Option<&'a str> { + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries self.0.next_match().map(|(a, b)| unsafe { // Indices are known to be on utf8 boundaries self.0.haystack().get_unchecked(a..b) @@ -1359,6 +1368,7 @@ impl<'a, P: Pattern<'a>> MatchesInternal<'a, P> { where P::Searcher: ReverseSearcher<'a>, { + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries self.0.next_match_back().map(|(a, b)| unsafe { // Indices are known to be on utf8 boundaries self.0.haystack().get_unchecked(a..b) @@ -1579,6 +1589,9 @@ fn run_utf8_validation(v: &[u8]) -> Result<(), Utf8Error> { if align != usize::max_value() && align.wrapping_sub(index) % usize_bytes == 0 { let ptr = v.as_ptr(); while index < blocks_end { + // SAFETY: since `align - index` and `ascii_block_size` are multiples of + // `usize_bytes`, `ptr.add(index)` is always aligned with a `usize` so we + // may cast directly to a `const` pointer. unsafe { let block = ptr.add(index) as *const usize; // break if there is a nonascii byte @@ -1804,6 +1817,7 @@ mod traits { && slice.is_char_boundary(self.start) && slice.is_char_boundary(self.end) { + // SAFETY: just checked that `start` and `end` are on a char boundary Some(unsafe { self.get_unchecked(slice) }) } else { None @@ -1815,6 +1829,7 @@ mod traits { && slice.is_char_boundary(self.start) && slice.is_char_boundary(self.end) { + // SAFETY: just checked that `start` and `end` are on a char boundary Some(unsafe { self.get_unchecked_mut(slice) }) } else { None @@ -1845,6 +1860,7 @@ mod traits { && slice.is_char_boundary(self.start) && slice.is_char_boundary(self.end) { + // SAFETY: just checked that `start` and `end` are on a char boundary unsafe { self.get_unchecked_mut(slice) } } else { super::slice_error_fail(slice, self.start, self.end) @@ -1873,6 +1889,7 @@ mod traits { #[inline] fn get(self, slice: &str) -> Option<&Self::Output> { if slice.is_char_boundary(self.end) { + // SAFETY: just checked that `end` is on a char boundary Some(unsafe { self.get_unchecked(slice) }) } else { None @@ -1881,6 +1898,7 @@ mod traits { #[inline] fn get_mut(self, slice: &mut str) -> Option<&mut Self::Output> { if slice.is_char_boundary(self.end) { + // SAFETY: just checked that `end` is on a char boundary Some(unsafe { self.get_unchecked_mut(slice) }) } else { None @@ -1903,8 +1921,8 @@ mod traits { } #[inline] fn index_mut(self, slice: &mut str) -> &mut Self::Output { - // is_char_boundary checks that the index is in [0, .len()] if slice.is_char_boundary(self.end) { + // SAFETY: just checked that `end` is on a char boundary unsafe { self.get_unchecked_mut(slice) } } else { super::slice_error_fail(slice, 0, self.end) @@ -1934,6 +1952,7 @@ mod traits { #[inline] fn get(self, slice: &str) -> Option<&Self::Output> { if slice.is_char_boundary(self.start) { + // SAFETY: just checked that `start` is on a char boundary Some(unsafe { self.get_unchecked(slice) }) } else { None @@ -1942,6 +1961,7 @@ mod traits { #[inline] fn get_mut(self, slice: &mut str) -> Option<&mut Self::Output> { if slice.is_char_boundary(self.start) { + // SAFETY: just checked that `start` is on a char boundary Some(unsafe { self.get_unchecked_mut(slice) }) } else { None @@ -1966,8 +1986,8 @@ mod traits { } #[inline] fn index_mut(self, slice: &mut str) -> &mut Self::Output { - // is_char_boundary checks that the index is in [0, .len()] if slice.is_char_boundary(self.start) { + // SAFETY: just checked that `start` is on a char boundary unsafe { self.get_unchecked_mut(slice) } } else { super::slice_error_fail(slice, self.start, slice.len()) @@ -2238,7 +2258,6 @@ impl str { #[stable(feature = "rust1", since = "1.0.0")] #[rustc_const_stable(feature = "str_as_bytes", since = "1.32.0")] #[inline(always)] - // SAFETY: const sound because we transmute two types with the same layout #[allow(unused_attributes)] #[allow_internal_unstable(const_fn_union)] pub const fn as_bytes(&self) -> &[u8] { @@ -2247,6 +2266,7 @@ impl str { str: &'a str, slice: &'a [u8], } + // SAFETY: const sound because we transmute two types with the same layout unsafe { Slices { str: self }.slice } } @@ -2573,6 +2593,7 @@ impl str { pub fn split_at(&self, mid: usize) -> (&str, &str) { // is_char_boundary checks that the index is in [0, .len()] if self.is_char_boundary(mid) { + // SAFETY: just checked that `mid` is on a char boundary unsafe { (self.get_unchecked(0..mid), self.get_unchecked(mid..self.len())) } } else { slice_error_fail(self, 0, mid) @@ -2617,6 +2638,7 @@ impl str { if self.is_char_boundary(mid) { let len = self.len(); let ptr = self.as_mut_ptr(); + // SAFETY: just checked that `mid` is on a char boundary unsafe { ( from_utf8_unchecked_mut(slice::from_raw_parts_mut(ptr, mid)), @@ -3805,8 +3827,8 @@ impl str { if let Some((_, b)) = matcher.next_reject_back() { j = b; } + // SAFETY: `Searcher` is known to return valid indices unsafe { - // Searcher is known to return valid indices self.get_unchecked(i..j) } } @@ -3844,8 +3866,8 @@ impl str { if let Some((a, _)) = matcher.next_reject() { i = a; } + // SAFETY: `Searcher` is known to return valid indices unsafe { - // Searcher is known to return valid indices self.get_unchecked(i..self.len()) } } @@ -3970,8 +3992,8 @@ impl str { if let Some((_, b)) = matcher.next_reject_back() { j = b; } + // SAFETY: `Searcher` is known to return valid indices unsafe { - // Searcher is known to return valid indices self.get_unchecked(0..j) } } @@ -4166,6 +4188,7 @@ impl str { /// ``` #[stable(feature = "ascii_methods_on_intrinsics", since = "1.23.0")] pub fn make_ascii_uppercase(&mut self) { + // SAFETY: safe because we transmute two types with the same layout let me = unsafe { self.as_bytes_mut() }; me.make_ascii_uppercase() } @@ -4191,6 +4214,7 @@ impl str { /// ``` #[stable(feature = "ascii_methods_on_intrinsics", since = "1.23.0")] pub fn make_ascii_lowercase(&mut self) { + // SAFETY: safe because we transmute two types with the same layout let me = unsafe { self.as_bytes_mut() }; me.make_ascii_lowercase() } @@ -4356,6 +4380,7 @@ impl Default for &str { #[stable(feature = "default_mut_str", since = "1.28.0")] impl Default for &mut str { /// Creates an empty mutable str + // SAFETY: `str` is guranteed to be UTF-8 fn default() -> Self { unsafe { from_utf8_unchecked_mut(&mut []) } } @@ -4412,6 +4437,7 @@ impl_fn_for_zst! { #[derive(Clone)] struct UnsafeBytesToStr impl<'a> Fn = |bytes: &'a [u8]| -> &'a str { + // SAFETY: not safe unsafe { from_utf8_unchecked(bytes) } }; } diff --git a/src/libcore/str/pattern.rs b/src/libcore/str/pattern.rs index 46d9499394a38..ef64d8b0fdf88 100644 --- a/src/libcore/str/pattern.rs +++ b/src/libcore/str/pattern.rs @@ -3,8 +3,6 @@ //! For more details, see the traits [`Pattern`], [`Searcher`], //! [`ReverseSearcher`], and [`DoubleEndedSearcher`]. -// ignore-tidy-undocumented-unsafe - #![unstable( feature = "pattern", reason = "API not fully fleshed out and ready to be stabilized", @@ -271,6 +269,14 @@ unsafe impl<'a> Searcher<'a> for CharSearcher<'a> { #[inline] fn next(&mut self) -> SearchStep { let old_finger = self.finger; + // SAFETY: 1-4 guarantee safety of `get_unchecked` + // 1. `self.finger` and `self.finger_back` are kept on unicode boundaries + // (this is invariant) + // 2. `self.finger >= 0` since it starts at 0 and only increases + // 3. `self.finger < self.finger_back` because otherwise the char `iter` + // would return `SearchStep::Done` + // 4. `self.finger` comes before the end of the haystack because `self.finger_back` + // starts at the end and only decreases let slice = unsafe { self.haystack.get_unchecked(old_finger..self.finger_back) }; let mut iter = slice.chars(); let old_len = iter.iter.len(); @@ -293,6 +299,7 @@ unsafe impl<'a> Searcher<'a> for CharSearcher<'a> { // get the haystack after the last character found let bytes = self.haystack.as_bytes().get(self.finger..self.finger_back)?; // the last byte of the utf8 encoded needle + // SAFETY: we have an invariant that `utf8_size < 5` let last_byte = unsafe { *self.utf8_encoded.get_unchecked(self.utf8_size - 1) }; if let Some(index) = memchr::memchr(last_byte, bytes) { // The new finger is the index of the byte we found, @@ -336,6 +343,7 @@ unsafe impl<'a> ReverseSearcher<'a> for CharSearcher<'a> { #[inline] fn next_back(&mut self) -> SearchStep { let old_finger = self.finger_back; + // SAFETY: see the comment for next() above let slice = unsafe { self.haystack.get_unchecked(self.finger..old_finger) }; let mut iter = slice.chars(); let old_len = iter.iter.len(); @@ -363,6 +371,7 @@ unsafe impl<'a> ReverseSearcher<'a> for CharSearcher<'a> { return None; }; // the last byte of the utf8 encoded needle + // SAFETY: we have an invariant that `utf8_size < 5` let last_byte = unsafe { *self.utf8_encoded.get_unchecked(self.utf8_size - 1) }; if let Some(index) = memchr::memrchr(last_byte, bytes) { // we searched a slice that was offset by self.finger, diff --git a/src/libcore/sync/atomic.rs b/src/libcore/sync/atomic.rs index fae95ca5cdb36..889f182561b63 100644 --- a/src/libcore/sync/atomic.rs +++ b/src/libcore/sync/atomic.rs @@ -112,8 +112,6 @@ //! println!("live threads: {}", old_thread_count + 1); //! ``` -// ignore-tidy-undocumented-unsafe - #![stable(feature = "rust1", since = "1.0.0")] #![cfg_attr(not(target_has_atomic_load_store = "8"), allow(dead_code))] #![cfg_attr(not(target_has_atomic_load_store = "8"), allow(unused_imports))] @@ -350,6 +348,7 @@ impl AtomicBool { #[inline] #[stable(feature = "atomic_access", since = "1.15.0")] pub fn get_mut(&mut self) -> &mut bool { + // SAFETY: the mutable reference guarantees unique ownership unsafe { &mut *(self.v.get() as *mut bool) } } @@ -400,6 +399,7 @@ impl AtomicBool { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn load(&self, order: Ordering) -> bool { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_load(self.v.get(), order) != 0 } } @@ -432,6 +432,7 @@ impl AtomicBool { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn store(&self, val: bool, order: Ordering) { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_store(self.v.get(), val as u8, order); } @@ -463,6 +464,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn swap(&self, val: bool, order: Ordering) -> bool { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_swap(self.v.get(), val as u8, order) != 0 } } @@ -558,6 +560,7 @@ impl AtomicBool { success: Ordering, failure: Ordering, ) -> Result { + // SAFETY: data races are prevented by atomic intrinsics match unsafe { atomic_compare_exchange(self.v.get(), current as u8, new as u8, success, failure) } { @@ -615,6 +618,7 @@ impl AtomicBool { success: Ordering, failure: Ordering, ) -> Result { + // SAFETY: data races are prevented by atomic intrinsics match unsafe { atomic_compare_exchange_weak(self.v.get(), current as u8, new as u8, success, failure) } { @@ -661,6 +665,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn fetch_and(&self, val: bool, order: Ordering) -> bool { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_and(self.v.get(), val as u8, order) != 0 } } @@ -756,6 +761,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn fetch_or(&self, val: bool, order: Ordering) -> bool { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_or(self.v.get(), val as u8, order) != 0 } } @@ -797,6 +803,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn fetch_xor(&self, val: bool, order: Ordering) -> bool { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_xor(self.v.get(), val as u8, order) != 0 } } @@ -872,6 +879,7 @@ impl AtomicPtr { #[inline] #[stable(feature = "atomic_access", since = "1.15.0")] pub fn get_mut(&mut self) -> &mut *mut T { + // SAFETY: the mutable reference guarantees unique ownership unsafe { &mut *self.p.get() } } @@ -923,6 +931,7 @@ impl AtomicPtr { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn load(&self, order: Ordering) -> *mut T { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_load(self.p.get() as *mut usize, order) as *mut T } } @@ -957,6 +966,7 @@ impl AtomicPtr { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn store(&self, ptr: *mut T, order: Ordering) { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_store(self.p.get() as *mut usize, ptr as usize, order); } @@ -990,6 +1000,7 @@ impl AtomicPtr { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "ptr")] pub fn swap(&self, ptr: *mut T, order: Ordering) -> *mut T { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_swap(self.p.get() as *mut usize, ptr as usize, order) as *mut T } } @@ -1074,6 +1085,7 @@ impl AtomicPtr { success: Ordering, failure: Ordering, ) -> Result<*mut T, *mut T> { + // SAFETY: data races are prevented by atomic intrinsics unsafe { let res = atomic_compare_exchange( self.p.get() as *mut usize, @@ -1137,6 +1149,7 @@ impl AtomicPtr { success: Ordering, failure: Ordering, ) -> Result<*mut T, *mut T> { + // SAFETY: data races are prevented by atomic intrinsics unsafe { let res = atomic_compare_exchange_weak( self.p.get() as *mut usize, @@ -1290,6 +1303,7 @@ assert_eq!(some_var.load(Ordering::SeqCst), 5); #[inline] #[$stable_access] pub fn get_mut(&mut self) -> &mut $int_type { + // SAFETY: the mutable reference guarantees unique ownership unsafe { &mut *self.v.get() } } } @@ -1344,6 +1358,7 @@ assert_eq!(some_var.load(Ordering::Relaxed), 5); #[inline] #[$stable] pub fn load(&self, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_load(self.v.get(), order) } } } @@ -1378,6 +1393,7 @@ assert_eq!(some_var.load(Ordering::Relaxed), 10); #[inline] #[$stable] pub fn store(&self, val: $int_type, order: Ordering) { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_store(self.v.get(), val, order); } } } @@ -1408,6 +1424,7 @@ assert_eq!(some_var.swap(10, Ordering::Relaxed), 5); #[$stable] #[$cfg_cas] pub fn swap(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_swap(self.v.get(), val, order) } } } @@ -1510,6 +1527,7 @@ assert_eq!(some_var.load(Ordering::Relaxed), 10); new: $int_type, success: Ordering, failure: Ordering) -> Result<$int_type, $int_type> { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_compare_exchange(self.v.get(), current, new, success, failure) } } } @@ -1562,6 +1580,7 @@ loop { new: $int_type, success: Ordering, failure: Ordering) -> Result<$int_type, $int_type> { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_compare_exchange_weak(self.v.get(), current, new, success, failure) } @@ -1596,6 +1615,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 10); #[$stable] #[$cfg_cas] pub fn fetch_add(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_add(self.v.get(), val, order) } } } @@ -1628,6 +1648,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 10); #[$stable] #[$cfg_cas] pub fn fetch_sub(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_sub(self.v.get(), val, order) } } } @@ -1663,6 +1684,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 0b100001); #[$stable] #[$cfg_cas] pub fn fetch_and(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_and(self.v.get(), val, order) } } } @@ -1699,6 +1721,7 @@ assert_eq!(foo.load(Ordering::SeqCst), !(0x13 & 0x31)); #[$stable_nand] #[$cfg_cas] pub fn fetch_nand(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_nand(self.v.get(), val, order) } } } @@ -1734,6 +1757,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 0b111111); #[$stable] #[$cfg_cas] pub fn fetch_or(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_or(self.v.get(), val, order) } } } @@ -1769,6 +1793,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 0b011110); #[$stable] #[$cfg_cas] pub fn fetch_xor(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { atomic_xor(self.v.get(), val, order) } } } @@ -1880,6 +1905,7 @@ assert!(max_foo == 42); issue = "48655")] #[$cfg_cas] pub fn fetch_max(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { $max_fn(self.v.get(), val, order) } } } @@ -1932,6 +1958,7 @@ assert_eq!(min_foo, 12); issue = "48655")] #[$cfg_cas] pub fn fetch_min(&self, val: $int_type, order: Ordering) -> $int_type { + // SAFETY: data races are prevented by atomic intrinsics unsafe { $min_fn(self.v.get(), val, order) } } } @@ -2526,6 +2553,7 @@ pub fn fence(order: Ordering) { // https://github.com/WebAssembly/tool-conventions/issues/59. We should // follow that discussion and implement a solution when one comes about! #[cfg(not(target_arch = "wasm32"))] + // SAFETY: using an atomic fence is safe unsafe { match order { Acquire => intrinsics::atomic_fence_acq(), @@ -2613,6 +2641,7 @@ pub fn fence(order: Ordering) { #[inline] #[stable(feature = "compiler_fences", since = "1.21.0")] pub fn compiler_fence(order: Ordering) { + // SAFETY: doesn't compile to machine code unsafe { match order { Acquire => intrinsics::atomic_singlethreadfence_acq(), From a93e99cae7c1e6352304f65be93f28f1b5713231 Mon Sep 17 00:00:00 2001 From: Phoebe Bell Date: Tue, 19 Nov 2019 18:39:36 -0800 Subject: [PATCH 07/13] Fix typo "gurantees -> guarantees" --- src/libcore/cell.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index c530432f802db..f351735bef240 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -897,7 +897,7 @@ impl RefCell { #[inline] pub fn try_borrow_mut(&self) -> Result, BorrowMutError> { match BorrowRefMut::new(&self.borrow) { - // SAFETY: `BorrowRef` gurantees unique access + // SAFETY: `BorrowRef` guarantees unique access Some(b) => Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b }), None => Err(BorrowMutError { _private: () }), } From e0140ffeb0a7378f6568f5b8efebcfc58278f03a Mon Sep 17 00:00:00 2001 From: Phoebe Bell Date: Thu, 26 Dec 2019 11:57:57 -0800 Subject: [PATCH 08/13] Apply suggestions from code review Co-Authored-By: Ralf Jung --- src/libcore/str/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcore/str/mod.rs b/src/libcore/str/mod.rs index 3139d6188e27f..f89edf1910fac 100644 --- a/src/libcore/str/mod.rs +++ b/src/libcore/str/mod.rs @@ -659,7 +659,7 @@ impl<'a> Chars<'a> { #[stable(feature = "iter_to_slice", since = "1.4.0")] #[inline] pub fn as_str(&self) -> &'a str { - // SAFETY: Chars is only made from a str, which guarantees the iter is valid utf8 + // SAFETY: `Chars` is only made from a str, which guarantees the iter is valid utf8 unsafe { from_utf8_unchecked(self.iter.as_slice()) } } } From ca2fae8edbf0fce88e8e780d69745b26c856f4fc Mon Sep 17 00:00:00 2001 From: Phoebe Bell Date: Thu, 26 Dec 2019 12:56:34 -0800 Subject: [PATCH 09/13] Elaborate on SAFETY comments --- src/libcore/cell.rs | 25 +++++++++----- src/libcore/str/lossy.rs | 9 ++--- src/libcore/str/mod.rs | 67 +++++++++++++++++++------------------- src/libcore/sync/atomic.rs | 64 ++++++++++++++++++------------------ 4 files changed, 88 insertions(+), 77 deletions(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index f351735bef240..e7eecf7540ad7 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -366,7 +366,10 @@ impl Cell { if ptr::eq(self, other) { return; } - // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe + // SAFETY: This can be risky if called from separate threads, but `Cell` + // is `!Sync` so this won't happen. This also won't invalidate any + // pointers since `Cell` makes sure nothing else will be pointing into + // either of these `Cell`s. unsafe { ptr::swap(self.value.get(), other.value.get()); } @@ -386,7 +389,8 @@ impl Cell { /// ``` #[stable(feature = "move_cell", since = "1.17.0")] pub fn replace(&self, val: T) -> T { - // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe + // SAFETY: This can cause data races if called from a separate thread, + // but `Cell` is `!Sync` so this won't happen. mem::replace(unsafe { &mut *self.value.get() }, val) } @@ -423,7 +427,8 @@ impl Cell { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn get(&self) -> T { - // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe + // SAFETY: This can cause data races if called from a separate thread, + // but `Cell` is `!Sync` so this won't happen. unsafe { *self.value.get() } } @@ -492,7 +497,9 @@ impl Cell { #[inline] #[stable(feature = "cell_get_mut", since = "1.11.0")] pub fn get_mut(&mut self) -> &mut T { - // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe + // SAFETY: This can cause data races if called from a separate thread, + // but `Cell` is `!Sync` so this won't happen, and `&mut` guarantees + // unique access. unsafe { &mut *self.value.get() } } @@ -512,7 +519,7 @@ impl Cell { #[inline] #[stable(feature = "as_cell", since = "1.37.0")] pub fn from_mut(t: &mut T) -> &Cell { - // SAFETY: `&mut` ensures unique access + // SAFETY: `&mut` ensures unique access. unsafe { &*(t as *mut T as *const Cell) } } } @@ -556,7 +563,7 @@ impl Cell<[T]> { /// ``` #[stable(feature = "as_cell", since = "1.37.0")] pub fn as_slice_of_cells(&self) -> &[Cell] { - // SAFETY: `Cell` has the same memory layout as `T` + // SAFETY: `Cell` has the same memory layout as `T`. unsafe { &*(self as *const Cell<[T]> as *const [Cell]) } } } @@ -821,7 +828,7 @@ impl RefCell { pub fn try_borrow(&self) -> Result, BorrowError> { match BorrowRef::new(&self.borrow) { // SAFETY: `BorrowRef` ensures that there is only immutable access - // to the value while borrowed + // to the value while borrowed. Some(b) => Ok(Ref { value: unsafe { &*self.value.get() }, borrow: b }), None => Err(BorrowError { _private: () }), } @@ -897,7 +904,7 @@ impl RefCell { #[inline] pub fn try_borrow_mut(&self) -> Result, BorrowMutError> { match BorrowRefMut::new(&self.borrow) { - // SAFETY: `BorrowRef` guarantees unique access + // SAFETY: `BorrowRef` guarantees unique access. Some(b) => Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b }), None => Err(BorrowMutError { _private: () }), } @@ -947,7 +954,7 @@ impl RefCell { #[inline] #[stable(feature = "cell_get_mut", since = "1.11.0")] pub fn get_mut(&mut self) -> &mut T { - // SAFETY: `&mut` guarantees unique access + // SAFETY: `&mut` guarantees unique access. unsafe { &mut *self.value.get() } } diff --git a/src/libcore/str/lossy.rs b/src/libcore/str/lossy.rs index 9d2e38734ef02..88b2bc551b7d1 100644 --- a/src/libcore/str/lossy.rs +++ b/src/libcore/str/lossy.rs @@ -15,7 +15,7 @@ impl Utf8Lossy { } pub fn from_bytes(bytes: &[u8]) -> &Utf8Lossy { - // SAFETY: both use the same memory layout, and UTF-8 correctness isn't required + // SAFETY: Both use the same memory layout, and UTF-8 correctness isn't required. unsafe { mem::transmute(bytes) } } @@ -59,7 +59,8 @@ impl<'a> Iterator for Utf8LossyChunksIter<'a> { while i < self.source.len() { let i_ = i; - // SAFETY: 0 <= i < self.source.len() + // SAFETY: `i` starts at `0`, is less than `self.source.len()`, and + // only increases, so `0 <= i < self.source.len()`. let byte = unsafe { *self.source.get_unchecked(i) }; i += 1; @@ -69,7 +70,7 @@ impl<'a> Iterator for Utf8LossyChunksIter<'a> { macro_rules! error { () => {{ - // SAFETY: we have checked up to `i` that source is valid UTF-8 + // SAFETY: We have checked up to `i` that source is valid UTF-8. unsafe { let r = Utf8LossyChunk { valid: core_str::from_utf8_unchecked(&self.source[0..i_]), @@ -131,7 +132,7 @@ impl<'a> Iterator for Utf8LossyChunksIter<'a> { } let r = Utf8LossyChunk { - // SAFETY: we have checked that the entire source is valid UTF-8 + // SAFETY: We have checked that the entire source is valid UTF-8. valid: unsafe { core_str::from_utf8_unchecked(self.source) }, broken: &[], }; diff --git a/src/libcore/str/mod.rs b/src/libcore/str/mod.rs index f89edf1910fac..ec1713d1d4baa 100644 --- a/src/libcore/str/mod.rs +++ b/src/libcore/str/mod.rs @@ -340,7 +340,7 @@ impl Utf8Error { #[stable(feature = "rust1", since = "1.0.0")] pub fn from_utf8(v: &[u8]) -> Result<&str, Utf8Error> { run_utf8_validation(v)?; - // SAFETY: just ran validation + // SAFETY: Just ran validation. Ok(unsafe { from_utf8_unchecked(v) }) } @@ -379,7 +379,7 @@ pub fn from_utf8(v: &[u8]) -> Result<&str, Utf8Error> { #[stable(feature = "str_mut_extras", since = "1.20.0")] pub fn from_utf8_mut(v: &mut [u8]) -> Result<&mut str, Utf8Error> { run_utf8_validation(v)?; - // SAFETY: just ran validation + // SAFETY: Just ran validation. Ok(unsafe { from_utf8_unchecked_mut(v) }) } @@ -582,7 +582,7 @@ impl<'a> Iterator for Chars<'a> { #[inline] fn next(&mut self) -> Option { next_code_point(&mut self.iter).map(|ch| { - // SAFETY: str invariant says `ch` is a valid Unicode Scalar Value + // SAFETY: `str` invariant says `ch` is a valid Unicode Scalar Value. unsafe { char::from_u32_unchecked(ch) } }) } @@ -629,7 +629,7 @@ impl<'a> DoubleEndedIterator for Chars<'a> { #[inline] fn next_back(&mut self) -> Option { next_code_point_reverse(&mut self.iter).map(|ch| { - // SAFETY: str invariant says `ch` is a valid Unicode Scalar Value + // SAFETY: `str` invariant says `ch` is a valid Unicode Scalar Value. unsafe { char::from_u32_unchecked(ch) } }) } @@ -659,7 +659,7 @@ impl<'a> Chars<'a> { #[stable(feature = "iter_to_slice", since = "1.4.0")] #[inline] pub fn as_str(&self) -> &'a str { - // SAFETY: `Chars` is only made from a str, which guarantees the iter is valid utf8 + // SAFETY: `Chars` is only made from a str, which guarantees the iter is valid UTF-8. unsafe { from_utf8_unchecked(self.iter.as_slice()) } } } @@ -1104,7 +1104,7 @@ impl<'a, P: Pattern<'a>> SplitInternal<'a, P> { fn get_end(&mut self) -> Option<&'a str> { if !self.finished && (self.allow_trailing_empty || self.end - self.start > 0) { self.finished = true; - // SAFETY: `self.start` and `self.end` always lie on unicode boundaries + // SAFETY: `self.start` and `self.end` always lie on unicode boundaries. unsafe { let string = self.matcher.haystack().get_unchecked(self.start..self.end); Some(string) @@ -1122,7 +1122,7 @@ impl<'a, P: Pattern<'a>> SplitInternal<'a, P> { let haystack = self.matcher.haystack(); match self.matcher.next_match() { - // SAFETY: `Searcher` guarantees that `a` and `b` lie on unicode boundaries + // SAFETY: `Searcher` guarantees that `a` and `b` lie on unicode boundaries. Some((a, b)) => unsafe { let elt = haystack.get_unchecked(self.start..a); self.start = b; @@ -1155,13 +1155,13 @@ impl<'a, P: Pattern<'a>> SplitInternal<'a, P> { let haystack = self.matcher.haystack(); match self.matcher.next_match_back() { - // SAFETY: `Searcher` guarantees that `a` and `b` lie on unicode boundaries + // SAFETY: `Searcher` guarantees that `a` and `b` lie on unicode boundaries. Some((a, b)) => unsafe { let elt = haystack.get_unchecked(b..self.end); self.end = a; Some(elt) }, - // SAFETY: `self.start` and `self.end` always lie on unicode boundaries + // SAFETY: `self.start` and `self.end` always lie on unicode boundaries. None => unsafe { self.finished = true; Some(haystack.get_unchecked(self.start..self.end)) @@ -1301,7 +1301,7 @@ where impl<'a, P: Pattern<'a>> MatchIndicesInternal<'a, P> { #[inline] fn next(&mut self) -> Option<(usize, &'a str)> { - // SAFETY: `Searcher` guaratees that `start` and `end` lie on unicode boundaries + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. self.0 .next_match() .map(|(start, end)| unsafe { (start, self.0.haystack().get_unchecked(start..end)) }) @@ -1312,7 +1312,7 @@ impl<'a, P: Pattern<'a>> MatchIndicesInternal<'a, P> { where P::Searcher: ReverseSearcher<'a>, { - // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. self.0 .next_match_back() .map(|(start, end)| unsafe { (start, self.0.haystack().get_unchecked(start..end)) }) @@ -1356,7 +1356,7 @@ where impl<'a, P: Pattern<'a>> MatchesInternal<'a, P> { #[inline] fn next(&mut self) -> Option<&'a str> { - // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. self.0.next_match().map(|(a, b)| unsafe { // Indices are known to be on utf8 boundaries self.0.haystack().get_unchecked(a..b) @@ -1368,7 +1368,7 @@ impl<'a, P: Pattern<'a>> MatchesInternal<'a, P> { where P::Searcher: ReverseSearcher<'a>, { - // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. self.0.next_match_back().map(|(a, b)| unsafe { // Indices are known to be on utf8 boundaries self.0.haystack().get_unchecked(a..b) @@ -1589,9 +1589,10 @@ fn run_utf8_validation(v: &[u8]) -> Result<(), Utf8Error> { if align != usize::max_value() && align.wrapping_sub(index) % usize_bytes == 0 { let ptr = v.as_ptr(); while index < blocks_end { - // SAFETY: since `align - index` and `ascii_block_size` are multiples of - // `usize_bytes`, `ptr.add(index)` is always aligned with a `usize` so we - // may cast directly to a `const` pointer. + // SAFETY: since `align - index` and `ascii_block_size` are + // multiples of `usize_bytes`, `block = ptr.add(index)` is + // always aligned with a `usize` so it's safe to dereference + // both `block` and `block.offset(1)`. unsafe { let block = ptr.add(index) as *const usize; // break if there is a nonascii byte @@ -1817,7 +1818,7 @@ mod traits { && slice.is_char_boundary(self.start) && slice.is_char_boundary(self.end) { - // SAFETY: just checked that `start` and `end` are on a char boundary + // SAFETY: just checked that `start` and `end` are on a char boundary. Some(unsafe { self.get_unchecked(slice) }) } else { None @@ -1829,7 +1830,7 @@ mod traits { && slice.is_char_boundary(self.start) && slice.is_char_boundary(self.end) { - // SAFETY: just checked that `start` and `end` are on a char boundary + // SAFETY: just checked that `start` and `end` are on a char boundary. Some(unsafe { self.get_unchecked_mut(slice) }) } else { None @@ -1860,7 +1861,7 @@ mod traits { && slice.is_char_boundary(self.start) && slice.is_char_boundary(self.end) { - // SAFETY: just checked that `start` and `end` are on a char boundary + // SAFETY: just checked that `start` and `end` are on a char boundary. unsafe { self.get_unchecked_mut(slice) } } else { super::slice_error_fail(slice, self.start, self.end) @@ -1889,7 +1890,7 @@ mod traits { #[inline] fn get(self, slice: &str) -> Option<&Self::Output> { if slice.is_char_boundary(self.end) { - // SAFETY: just checked that `end` is on a char boundary + // SAFETY: just checked that `end` is on a char boundary. Some(unsafe { self.get_unchecked(slice) }) } else { None @@ -1898,7 +1899,7 @@ mod traits { #[inline] fn get_mut(self, slice: &mut str) -> Option<&mut Self::Output> { if slice.is_char_boundary(self.end) { - // SAFETY: just checked that `end` is on a char boundary + // SAFETY: just checked that `end` is on a char boundary. Some(unsafe { self.get_unchecked_mut(slice) }) } else { None @@ -1922,7 +1923,7 @@ mod traits { #[inline] fn index_mut(self, slice: &mut str) -> &mut Self::Output { if slice.is_char_boundary(self.end) { - // SAFETY: just checked that `end` is on a char boundary + // SAFETY: just checked that `end` is on a char boundary. unsafe { self.get_unchecked_mut(slice) } } else { super::slice_error_fail(slice, 0, self.end) @@ -1952,7 +1953,7 @@ mod traits { #[inline] fn get(self, slice: &str) -> Option<&Self::Output> { if slice.is_char_boundary(self.start) { - // SAFETY: just checked that `start` is on a char boundary + // SAFETY: just checked that `start` is on a char boundary. Some(unsafe { self.get_unchecked(slice) }) } else { None @@ -1961,7 +1962,7 @@ mod traits { #[inline] fn get_mut(self, slice: &mut str) -> Option<&mut Self::Output> { if slice.is_char_boundary(self.start) { - // SAFETY: just checked that `start` is on a char boundary + // SAFETY: just checked that `start` is on a char boundary. Some(unsafe { self.get_unchecked_mut(slice) }) } else { None @@ -1987,7 +1988,7 @@ mod traits { #[inline] fn index_mut(self, slice: &mut str) -> &mut Self::Output { if slice.is_char_boundary(self.start) { - // SAFETY: just checked that `start` is on a char boundary + // SAFETY: just checked that `start` is on a char boundary. unsafe { self.get_unchecked_mut(slice) } } else { super::slice_error_fail(slice, self.start, slice.len()) @@ -2593,7 +2594,7 @@ impl str { pub fn split_at(&self, mid: usize) -> (&str, &str) { // is_char_boundary checks that the index is in [0, .len()] if self.is_char_boundary(mid) { - // SAFETY: just checked that `mid` is on a char boundary + // SAFETY: just checked that `mid` is on a char boundary. unsafe { (self.get_unchecked(0..mid), self.get_unchecked(mid..self.len())) } } else { slice_error_fail(self, 0, mid) @@ -2638,7 +2639,7 @@ impl str { if self.is_char_boundary(mid) { let len = self.len(); let ptr = self.as_mut_ptr(); - // SAFETY: just checked that `mid` is on a char boundary + // SAFETY: just checked that `mid` is on a char boundary. unsafe { ( from_utf8_unchecked_mut(slice::from_raw_parts_mut(ptr, mid)), @@ -3827,7 +3828,7 @@ impl str { if let Some((_, b)) = matcher.next_reject_back() { j = b; } - // SAFETY: `Searcher` is known to return valid indices + // SAFETY: `Searcher` is known to return valid indices. unsafe { self.get_unchecked(i..j) } @@ -3866,7 +3867,7 @@ impl str { if let Some((a, _)) = matcher.next_reject() { i = a; } - // SAFETY: `Searcher` is known to return valid indices + // SAFETY: `Searcher` is known to return valid indices. unsafe { self.get_unchecked(i..self.len()) } @@ -3992,7 +3993,7 @@ impl str { if let Some((_, b)) = matcher.next_reject_back() { j = b; } - // SAFETY: `Searcher` is known to return valid indices + // SAFETY: `Searcher` is known to return valid indices. unsafe { self.get_unchecked(0..j) } @@ -4188,7 +4189,7 @@ impl str { /// ``` #[stable(feature = "ascii_methods_on_intrinsics", since = "1.23.0")] pub fn make_ascii_uppercase(&mut self) { - // SAFETY: safe because we transmute two types with the same layout + // SAFETY: safe because we transmute two types with the same layout. let me = unsafe { self.as_bytes_mut() }; me.make_ascii_uppercase() } @@ -4214,7 +4215,7 @@ impl str { /// ``` #[stable(feature = "ascii_methods_on_intrinsics", since = "1.23.0")] pub fn make_ascii_lowercase(&mut self) { - // SAFETY: safe because we transmute two types with the same layout + // SAFETY: safe because we transmute two types with the same layout. let me = unsafe { self.as_bytes_mut() }; me.make_ascii_lowercase() } @@ -4380,7 +4381,7 @@ impl Default for &str { #[stable(feature = "default_mut_str", since = "1.28.0")] impl Default for &mut str { /// Creates an empty mutable str - // SAFETY: `str` is guranteed to be UTF-8 + // SAFETY: `str` is guranteed to be UTF-8. fn default() -> Self { unsafe { from_utf8_unchecked_mut(&mut []) } } diff --git a/src/libcore/sync/atomic.rs b/src/libcore/sync/atomic.rs index 889f182561b63..8d55dc55ef2d6 100644 --- a/src/libcore/sync/atomic.rs +++ b/src/libcore/sync/atomic.rs @@ -348,7 +348,7 @@ impl AtomicBool { #[inline] #[stable(feature = "atomic_access", since = "1.15.0")] pub fn get_mut(&mut self) -> &mut bool { - // SAFETY: the mutable reference guarantees unique ownership + // SAFETY: the mutable reference guarantees unique ownership. unsafe { &mut *(self.v.get() as *mut bool) } } @@ -399,7 +399,8 @@ impl AtomicBool { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn load(&self, order: Ordering) -> bool { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: any data races are prevented by atomic intrinsics and the raw + // pointer passed in is valid because we got it from a reference. unsafe { atomic_load(self.v.get(), order) != 0 } } @@ -432,7 +433,8 @@ impl AtomicBool { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn store(&self, val: bool, order: Ordering) { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: any data races are prevented by atomic intrinsics and the raw + // pointer passed in is valid because we got it from a reference. unsafe { atomic_store(self.v.get(), val as u8, order); } @@ -464,7 +466,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn swap(&self, val: bool, order: Ordering) -> bool { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_swap(self.v.get(), val as u8, order) != 0 } } @@ -560,7 +562,7 @@ impl AtomicBool { success: Ordering, failure: Ordering, ) -> Result { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. match unsafe { atomic_compare_exchange(self.v.get(), current as u8, new as u8, success, failure) } { @@ -618,7 +620,7 @@ impl AtomicBool { success: Ordering, failure: Ordering, ) -> Result { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. match unsafe { atomic_compare_exchange_weak(self.v.get(), current as u8, new as u8, success, failure) } { @@ -665,7 +667,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn fetch_and(&self, val: bool, order: Ordering) -> bool { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_and(self.v.get(), val as u8, order) != 0 } } @@ -761,7 +763,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn fetch_or(&self, val: bool, order: Ordering) -> bool { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_or(self.v.get(), val as u8, order) != 0 } } @@ -803,7 +805,7 @@ impl AtomicBool { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "8")] pub fn fetch_xor(&self, val: bool, order: Ordering) -> bool { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_xor(self.v.get(), val as u8, order) != 0 } } @@ -879,7 +881,7 @@ impl AtomicPtr { #[inline] #[stable(feature = "atomic_access", since = "1.15.0")] pub fn get_mut(&mut self) -> &mut *mut T { - // SAFETY: the mutable reference guarantees unique ownership + // SAFETY: the mutable reference guarantees unique ownership. unsafe { &mut *self.p.get() } } @@ -931,7 +933,7 @@ impl AtomicPtr { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn load(&self, order: Ordering) -> *mut T { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_load(self.p.get() as *mut usize, order) as *mut T } } @@ -966,7 +968,7 @@ impl AtomicPtr { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn store(&self, ptr: *mut T, order: Ordering) { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_store(self.p.get() as *mut usize, ptr as usize, order); } @@ -1000,7 +1002,7 @@ impl AtomicPtr { #[stable(feature = "rust1", since = "1.0.0")] #[cfg(target_has_atomic = "ptr")] pub fn swap(&self, ptr: *mut T, order: Ordering) -> *mut T { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_swap(self.p.get() as *mut usize, ptr as usize, order) as *mut T } } @@ -1085,7 +1087,7 @@ impl AtomicPtr { success: Ordering, failure: Ordering, ) -> Result<*mut T, *mut T> { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { let res = atomic_compare_exchange( self.p.get() as *mut usize, @@ -1149,7 +1151,7 @@ impl AtomicPtr { success: Ordering, failure: Ordering, ) -> Result<*mut T, *mut T> { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { let res = atomic_compare_exchange_weak( self.p.get() as *mut usize, @@ -1303,7 +1305,7 @@ assert_eq!(some_var.load(Ordering::SeqCst), 5); #[inline] #[$stable_access] pub fn get_mut(&mut self) -> &mut $int_type { - // SAFETY: the mutable reference guarantees unique ownership + // SAFETY: the mutable reference guarantees unique ownership. unsafe { &mut *self.v.get() } } } @@ -1358,7 +1360,7 @@ assert_eq!(some_var.load(Ordering::Relaxed), 5); #[inline] #[$stable] pub fn load(&self, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_load(self.v.get(), order) } } } @@ -1393,7 +1395,7 @@ assert_eq!(some_var.load(Ordering::Relaxed), 10); #[inline] #[$stable] pub fn store(&self, val: $int_type, order: Ordering) { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_store(self.v.get(), val, order); } } } @@ -1424,7 +1426,7 @@ assert_eq!(some_var.swap(10, Ordering::Relaxed), 5); #[$stable] #[$cfg_cas] pub fn swap(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_swap(self.v.get(), val, order) } } } @@ -1527,7 +1529,7 @@ assert_eq!(some_var.load(Ordering::Relaxed), 10); new: $int_type, success: Ordering, failure: Ordering) -> Result<$int_type, $int_type> { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_compare_exchange(self.v.get(), current, new, success, failure) } } } @@ -1580,7 +1582,7 @@ loop { new: $int_type, success: Ordering, failure: Ordering) -> Result<$int_type, $int_type> { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_compare_exchange_weak(self.v.get(), current, new, success, failure) } @@ -1615,7 +1617,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 10); #[$stable] #[$cfg_cas] pub fn fetch_add(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_add(self.v.get(), val, order) } } } @@ -1648,7 +1650,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 10); #[$stable] #[$cfg_cas] pub fn fetch_sub(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_sub(self.v.get(), val, order) } } } @@ -1684,7 +1686,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 0b100001); #[$stable] #[$cfg_cas] pub fn fetch_and(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_and(self.v.get(), val, order) } } } @@ -1721,7 +1723,7 @@ assert_eq!(foo.load(Ordering::SeqCst), !(0x13 & 0x31)); #[$stable_nand] #[$cfg_cas] pub fn fetch_nand(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_nand(self.v.get(), val, order) } } } @@ -1757,7 +1759,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 0b111111); #[$stable] #[$cfg_cas] pub fn fetch_or(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_or(self.v.get(), val, order) } } } @@ -1793,7 +1795,7 @@ assert_eq!(foo.load(Ordering::SeqCst), 0b011110); #[$stable] #[$cfg_cas] pub fn fetch_xor(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { atomic_xor(self.v.get(), val, order) } } } @@ -1905,7 +1907,7 @@ assert!(max_foo == 42); issue = "48655")] #[$cfg_cas] pub fn fetch_max(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { $max_fn(self.v.get(), val, order) } } } @@ -1958,7 +1960,7 @@ assert_eq!(min_foo, 12); issue = "48655")] #[$cfg_cas] pub fn fetch_min(&self, val: $int_type, order: Ordering) -> $int_type { - // SAFETY: data races are prevented by atomic intrinsics + // SAFETY: data races are prevented by atomic intrinsics. unsafe { $min_fn(self.v.get(), val, order) } } } @@ -2553,7 +2555,7 @@ pub fn fence(order: Ordering) { // https://github.com/WebAssembly/tool-conventions/issues/59. We should // follow that discussion and implement a solution when one comes about! #[cfg(not(target_arch = "wasm32"))] - // SAFETY: using an atomic fence is safe + // SAFETY: using an atomic fence is safe. unsafe { match order { Acquire => intrinsics::atomic_fence_acq(), @@ -2641,7 +2643,7 @@ pub fn fence(order: Ordering) { #[inline] #[stable(feature = "compiler_fences", since = "1.21.0")] pub fn compiler_fence(order: Ordering) { - // SAFETY: doesn't compile to machine code + // SAFETY: using an atomic fence is safe. unsafe { match order { Acquire => intrinsics::atomic_singlethreadfence_acq(), From c103c284b93039149f0ec2a4f3d20ca89f34c0ad Mon Sep 17 00:00:00 2001 From: Phoebe Bell Date: Thu, 16 Jan 2020 18:38:04 -0800 Subject: [PATCH 10/13] Move comments for tidy --- src/libcore/str/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libcore/str/mod.rs b/src/libcore/str/mod.rs index ec1713d1d4baa..3bfeee87cc013 100644 --- a/src/libcore/str/mod.rs +++ b/src/libcore/str/mod.rs @@ -1301,9 +1301,9 @@ where impl<'a, P: Pattern<'a>> MatchIndicesInternal<'a, P> { #[inline] fn next(&mut self) -> Option<(usize, &'a str)> { - // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. self.0 .next_match() + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. .map(|(start, end)| unsafe { (start, self.0.haystack().get_unchecked(start..end)) }) } @@ -1312,9 +1312,9 @@ impl<'a, P: Pattern<'a>> MatchIndicesInternal<'a, P> { where P::Searcher: ReverseSearcher<'a>, { - // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. self.0 .next_match_back() + // SAFETY: `Searcher` guarantees that `start` and `end` lie on unicode boundaries. .map(|(start, end)| unsafe { (start, self.0.haystack().get_unchecked(start..end)) }) } } @@ -3901,8 +3901,8 @@ impl str { "The first search step from Searcher \ must include the first character" ); + // SAFETY: `Searcher` is known to return valid indices. unsafe { - // Searcher is known to return valid indices. Some(self.get_unchecked(len..)) } } else { @@ -3942,8 +3942,8 @@ impl str { "The first search step from ReverseSearcher \ must include the last character" ); + // SAFETY: `Searcher` is known to return valid indices. unsafe { - // Searcher is known to return valid indices. Some(self.get_unchecked(..start)) } } else { @@ -4381,8 +4381,8 @@ impl Default for &str { #[stable(feature = "default_mut_str", since = "1.28.0")] impl Default for &mut str { /// Creates an empty mutable str - // SAFETY: `str` is guranteed to be UTF-8. fn default() -> Self { + // SAFETY: The empty string is valid UTF-8. unsafe { from_utf8_unchecked_mut(&mut []) } } } From 0f2ee495e9a9d677466ce0c1827ba20919a5ca1f Mon Sep 17 00:00:00 2001 From: Phoebe Bell Date: Thu, 16 Jan 2020 18:50:53 -0800 Subject: [PATCH 11/13] Fix formatting: ./x.py fmt --- src/libcore/str/mod.rs | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/libcore/str/mod.rs b/src/libcore/str/mod.rs index 3bfeee87cc013..5a7cddd4041d5 100644 --- a/src/libcore/str/mod.rs +++ b/src/libcore/str/mod.rs @@ -3829,9 +3829,7 @@ impl str { j = b; } // SAFETY: `Searcher` is known to return valid indices. - unsafe { - self.get_unchecked(i..j) - } + unsafe { self.get_unchecked(i..j) } } /// Returns a string slice with all prefixes that match a pattern @@ -3868,9 +3866,7 @@ impl str { i = a; } // SAFETY: `Searcher` is known to return valid indices. - unsafe { - self.get_unchecked(i..self.len()) - } + unsafe { self.get_unchecked(i..self.len()) } } /// Returns a string slice with the prefix removed. @@ -3902,9 +3898,7 @@ impl str { must include the first character" ); // SAFETY: `Searcher` is known to return valid indices. - unsafe { - Some(self.get_unchecked(len..)) - } + unsafe { Some(self.get_unchecked(len..)) } } else { None } @@ -3943,9 +3937,7 @@ impl str { must include the last character" ); // SAFETY: `Searcher` is known to return valid indices. - unsafe { - Some(self.get_unchecked(..start)) - } + unsafe { Some(self.get_unchecked(..start)) } } else { None } @@ -3994,9 +3986,7 @@ impl str { j = b; } // SAFETY: `Searcher` is known to return valid indices. - unsafe { - self.get_unchecked(0..j) - } + unsafe { self.get_unchecked(0..j) } } /// Returns a string slice with all prefixes that match a pattern From 022a7de26f5cb386ec853eec3b7f44dcd516a6d1 Mon Sep 17 00:00:00 2001 From: Phoebe Bell Date: Thu, 16 Jan 2020 19:26:02 -0800 Subject: [PATCH 12/13] Add SAFETY comment for atomic example --- src/libcore/sync/atomic.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libcore/sync/atomic.rs b/src/libcore/sync/atomic.rs index 8d55dc55ef2d6..9d449bb991507 100644 --- a/src/libcore/sync/atomic.rs +++ b/src/libcore/sync/atomic.rs @@ -1989,7 +1989,9 @@ extern { } let mut atomic = ", stringify!($atomic_type), "::new(1); -unsafe { +", +// SAFETY: Safe as long as `my_atomic_op` is atomic. +"unsafe { my_atomic_op(atomic.as_mut_ptr()); } # } From b1d0c118ff726fc8c5ff2a5a19840a9e36e8940e Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Thu, 16 Jan 2020 08:24:34 -0500 Subject: [PATCH 13/13] [self-profiler] Add example to `-Z help` to turn on query key recording Also add the `default` option so that it's easy to add query key recording to the default. --- src/librustc_data_structures/profiling.rs | 2 ++ src/librustc_session/options.rs | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/librustc_data_structures/profiling.rs b/src/librustc_data_structures/profiling.rs index 004db0a79a880..44cef727f034b 100644 --- a/src/librustc_data_structures/profiling.rs +++ b/src/librustc_data_structures/profiling.rs @@ -136,9 +136,11 @@ bitflags::bitflags! { } } +// keep this in sync with the `-Z self-profile-events` help message in librustc_session/options.rs const EVENT_FILTERS_BY_NAME: &[(&str, EventFilter)] = &[ ("none", EventFilter::NONE), ("all", EventFilter::ALL), + ("default", EventFilter::DEFAULT), ("generic-activity", EventFilter::GENERIC_ACTIVITIES), ("query-provider", EventFilter::QUERY_PROVIDERS), ("query-cache-hit", EventFilter::QUERY_CACHE_HITS), diff --git a/src/librustc_session/options.rs b/src/librustc_session/options.rs index 4b5736adc17c3..2a0ed27b63b08 100644 --- a/src/librustc_session/options.rs +++ b/src/librustc_session/options.rs @@ -923,8 +923,12 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, self_profile: SwitchWithOptPath = (SwitchWithOptPath::Disabled, parse_switch_with_opt_path, [UNTRACKED], "run the self profiler and output the raw event data"), + // keep this in sync with the event filter names in librustc_data_structures/profiling.rs self_profile_events: Option> = (None, parse_opt_comma_list, [UNTRACKED], - "specifies which kinds of events get recorded by the self profiler"), + "specifies which kinds of events get recorded by the self profiler; + for example: `-Z self-profile-events=default,query-keys` + all options: none, all, default, generic-activity, query-provider, query-cache-hit + query-blocked, incr-cache-load, query-keys"), emit_stack_sizes: bool = (false, parse_bool, [UNTRACKED], "emits a section containing stack size metadata"), plt: Option = (None, parse_opt_bool, [TRACKED],