From f1a40410ecce3c1b115e244c7e189e019e226c13 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Wed, 6 Apr 2022 16:30:49 +0200 Subject: [PATCH 01/25] Return status from futex_wake(). --- library/std/src/sys/unix/futex.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/library/std/src/sys/unix/futex.rs b/library/std/src/sys/unix/futex.rs index c61d948fb601d..4231241a14224 100644 --- a/library/std/src/sys/unix/futex.rs +++ b/library/std/src/sys/unix/futex.rs @@ -69,14 +69,14 @@ pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option) { } #[cfg(any(target_os = "linux", target_os = "android"))] -pub fn futex_wake(futex: &AtomicI32) { +pub fn futex_wake(futex: &AtomicI32) -> bool { unsafe { libc::syscall( libc::SYS_futex, futex as *const AtomicI32, libc::FUTEX_WAKE | libc::FUTEX_PRIVATE_FLAG, 1, - ); + ) > 0 } } @@ -93,12 +93,10 @@ pub fn futex_wake_all(futex: &AtomicI32) { } #[cfg(target_os = "emscripten")] -pub fn futex_wake(futex: &AtomicI32) { +pub fn futex_wake(futex: &AtomicI32) -> bool { extern "C" { fn emscripten_futex_wake(addr: *const AtomicI32, count: libc::c_int) -> libc::c_int; } - unsafe { - emscripten_futex_wake(futex as *const AtomicI32, 1); - } + unsafe { emscripten_futex_wake(futex as *const AtomicI32, 1) > 0 } } From 4f28344bb4af3e50acd02db40c4b30e913c5a9ef Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Thu, 24 Mar 2022 18:25:03 -0400 Subject: [PATCH 02/25] Improve documentation of `Place` and `Operand` --- compiler/rustc_middle/src/mir/mod.rs | 134 ++++++++++++++++++++++++--- 1 file changed, 121 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 0a4f84558fee4..d99e4dd815e2e 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1775,8 +1775,98 @@ pub struct CopyNonOverlapping<'tcx> { /////////////////////////////////////////////////////////////////////////// // Places -/// A path to a value; something that can be evaluated without -/// changing or disturbing program state. +/// Places roughly correspond to a "location in memory." Places in MIR are the same mathematical +/// object as places in Rust. This of course means that what exactly they are is undecided and part +/// of the Rust memory model. However, they will likely contain at least the following three pieces +/// of information in some form: +/// +/// 1. The part of memory that is referred to (see discussion below for details). +/// 2. The type of the place and an optional variant index. See [`PlaceTy`][tcx::PlaceTy] +/// 3. The provenance with which the place is being accessed. +/// +/// We'll give a description below of how the first two of these three properties are computed for a +/// place. We cannot give a description of the provenance, because that is part of the undecided +/// aliasing model - we only include it here at all to acknowledge its existence. +/// +/// For a place that has no projections, ie `Place { local, projection: [] }`, the part of memory is +/// the local's full allocation and the type is the type of the local. For any other place, we +/// define the values as a function of the parent place, that is the place with its last +/// [`ProjectionElem`] stripped. The way this is computed of course depends on the kind of that last +/// projection element: +/// +/// - [`Downcast`](ProjectionElem::Downcast): This projection sets the place's variant index to the +/// given one, and makes no other changes. A `Downcast` projection on a place with its variant +/// index already set is not well-formed. +/// - [`Field`](ProjectionElem::Field): `Field` projections take their parent place and create a +/// place referring to one of the fields of the type. The referred to place in memory is where +/// the layout places the field. The type becomes the type of the field. +/// +/// These projections are only legal for tuples, ADTs, closures, and generators. If the ADT or +/// generator has more than one variant, the parent place's variant index must be set, indicating +/// which variant is being used. If it has just one variant, the variant index may or may not be +/// included - the single possible variant is inferred if it is not included. +/// - [`ConstantIndex`](ProjectionElem::ConstantIndex): Computes an offset in units of `T` into the +/// place as described in the documentation for the `ProjectionElem`. The resulting part of +/// memory is the location of that element of the array/slice, and the type is `T`. This is only +/// legal if the parent place has type `[T; N]` or `[T]` (*not* `&[T]`). +/// - [`Subslice`](ProjectionElem::Subslice): Much like `ConstantIndex`. It is also only legal on +/// `[T; N]` and `[T]`. However, this yields a `Place` of type `[T]`, and may refer to more than +/// one element in the parent place. +/// - [`Index`](ProjectionElem::Index): Like `ConstantIndex`, only legal on `[T; N]` or `[T]`. +/// However, `Index` additionally takes a local from which the value of the index is computed at +/// runtime. Computing the value of the index involves interpreting the `Local` as a +/// `Place { local, projection: [] }`, and then computing its value as if done via +/// [`Operand::Copy`]. The array/slice is then indexed with the resulting value. The local must +/// have type `usize`. +/// - [`Deref`](ProjectionElem::Deref): Derefs are the last type of projection, and the most +/// complicated. They are only legal on parent places that are references, pointers, or `Box`. A +/// `Deref` projection begins by creating a value from the parent place, as if by +/// [`Operand::Copy`]. It then dereferences the resulting pointer, creating a place of the +/// pointed to type. +/// +/// **Needs clarification**: What about metadata resulting from dereferencing wide pointers (and +/// possibly from accessing unsized locals - not sure how those work)? That probably deserves to go +/// on the list above and be discussed too. It is also probably necessary for making the indexing +/// stuff lass hand-wavey. +/// +/// **Needs clarification**: When it says "part of memory" what does that mean precisely, and how +/// does it interact with the metadata? +/// +/// One possible model that I believe makes sense is that "part of memory" is actually just the +/// address of the beginning of the referred to range of bytes. For sized types, the size of the +/// range is then stored in the type, and for unsized types it's stored (possibly indirectly, +/// through a vtable) in the metadata. +/// +/// Alternatively, the "part of memory" could be a whole range of bytes. Initially seemed more +/// natural to me, but seems like it falls apart after a little bit. +/// +/// More likely though, we should call this detail a part of the Rust memory model and let that deal +/// with the precise definition of this part of a place. If we feel strongly, I don't think we *have +/// to* though. MIR places are more flexible than Rust places, and we might be able to make a +/// decision on the flexible parts without semi-stabilizing the source language. (end NC) +/// +/// Computing a place may be UB - this is certainly the case with dereferencing, which requires +/// sufficient provenance, but it may additionally be the case for some of the other field +/// projections. +/// +/// It is undecided when this UB kicks in. As best I can tell that is the question being discussed +/// in [UCG#319]. Summarizing from that thread, I believe the options are: +/// +/// [UCG#319]: https://github.com/rust-lang/unsafe-code-guidelines/issues/319 +/// +/// 1. Each intermediate place must have provenance for the whole part of memory it refers to. This +/// is the status quo. +/// 2. Only for intermediate place where the last projection was *not* a deref. This corresponds to +/// "Check inbounds on place projection". +/// 3. Only on place to value conversions, assignments, and referencing operation. This corresponds +/// to "remove the restrictions from `*` entirely." +/// 4. On each intermediate place if the place is used for a place to value conversion as part of +/// an assignment assignment or it is used for a referencing operation. For a raw pointer +/// computation, never. This corresponds to "magic?". +/// +/// Hopefully I am not misrepresenting anyone's opinions - please let me know if I am. Currently, +/// Rust chooses option 1. This is checked by MIRI and taken advantage of by codegen (via `gep +/// inbounds`). That is possibly subject to change. #[derive(Copy, Clone, PartialEq, Eq, Hash, TyEncodable, HashStable)] pub struct Place<'tcx> { pub local: Local, @@ -2145,24 +2235,42 @@ pub struct SourceScopeLocalData { /////////////////////////////////////////////////////////////////////////// // Operands -/// These are values that can appear inside an rvalue. They are intentionally -/// limited to prevent rvalues from being nested in one another. +/// An operand in MIR represents a "value" in Rust, the definition of which is undecided and part of +/// the memory model. One proposal for a definition of values can be found [on UCG][value-def]. +/// +/// [value-def]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/value-domain.md +/// +/// The most common way to create values is via a place to value conversion. A place to value +/// conversion is an operation which reads the memory of the place and converts it to a value. This +/// is a fundamentally *typed* operation. Different types will do different things. These are some +/// possible examples of what Rust may - but will not necessarily - decide to do on place to value +/// conversions: +/// +/// 1. Types with validity constraints cause UB if the validity constraint is not met +/// 2. References/pointers may have their provenance change or cause other provenance related +/// side-effects. +/// +/// A place to value conversion on a place that has its variant index set is not well-formed. +/// However, note that this rule only applies to places appearing in MIR bodies. Many functions, +/// such as [`Place::ty`], still accept such a place. If you write a function for which it might be +/// ambiguous whether such a thing is accepted, make sure to document your choice clearly. #[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)] pub enum Operand<'tcx> { - /// Copy: The value must be available for use afterwards. - /// - /// This implies that the type of the place must be `Copy`; this is true - /// by construction during build, but also checked by the MIR type checker. + /// Creates a value by performing a place to value conversion at the given place. The type of + /// the place must be `Copy` Copy(Place<'tcx>), - /// Move: The value (including old borrows of it) will not be used again. + /// Creates a value by performing a place to value conversion for the place, just like the + /// `Copy` operand. + /// + /// This *may* additionally overwrite the place with `uninit` bytes, depending on how we decide + /// in [UCG#188]. You should not emit MIR that may attempt a subsequent second place to value + /// conversion on this place without first re-initializing it. /// - /// Safe for values of all types (modulo future developments towards `?Move`). - /// Correct usage patterns are enforced by the borrow checker for safe code. - /// `Copy` may be converted to `Move` to enable "last-use" optimizations. + /// [UCG#188]: https://github.com/rust-lang/unsafe-code-guidelines/issues/188 Move(Place<'tcx>), - /// Synthesizes a constant value. + /// Constants are already semantically values, and remain unchanged. Constant(Box>), } From 83685906ada21e4e13379c539afa319db065a561 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Thu, 24 Mar 2022 18:25:37 -0400 Subject: [PATCH 03/25] Adjust computation of place types to detect more invalid places --- compiler/rustc_const_eval/src/transform/validate.rs | 10 +++++++++- compiler/rustc_middle/src/mir/tcx.rs | 3 +++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 263959f3cb376..cbad2510749d4 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -3,8 +3,8 @@ use rustc_index::bit_set::BitSet; use rustc_infer::infer::TyCtxtInferExt; use rustc_middle::mir::interpret::Scalar; -use rustc_middle::mir::traversal; use rustc_middle::mir::visit::{PlaceContext, Visitor}; +use rustc_middle::mir::{traversal, Place}; use rustc_middle::mir::{ AggregateKind, BasicBlock, Body, BorrowKind, Local, Location, MirPass, MirPhase, Operand, PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, StatementKind, Terminator, @@ -240,6 +240,14 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { self.super_projection_elem(local, proj_base, elem, context, location); } + fn visit_place(&mut self, place: &Place<'tcx>, _: PlaceContext, location: Location) { + // Set off any `bug!`s in the type computation code + let ty = place.ty(&self.body.local_decls, self.tcx); + if ty.variant_index.is_some() { + self.fail(location, "Top level places may not have their variant index set!"); + } + } + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { match &statement.kind { StatementKind::Assign(box (dest, rvalue)) => { diff --git a/compiler/rustc_middle/src/mir/tcx.rs b/compiler/rustc_middle/src/mir/tcx.rs index 51d8113840a93..597ade4223684 100644 --- a/compiler/rustc_middle/src/mir/tcx.rs +++ b/compiler/rustc_middle/src/mir/tcx.rs @@ -76,6 +76,9 @@ impl<'tcx> PlaceTy<'tcx> { V: ::std::fmt::Debug, T: ::std::fmt::Debug + Copy, { + if self.variant_index.is_some() && !matches!(elem, ProjectionElem::Field(..)) { + bug!("cannot use non field projection on downcasted place") + } let answer = match *elem { ProjectionElem::Deref => { let ty = self From e000179e423ce1586e2624afe29f1eac1346f851 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Thu, 24 Mar 2022 22:29:33 -0400 Subject: [PATCH 04/25] Add documentation for the semantics of MIR rvalues --- compiler/rustc_middle/src/lib.rs | 1 + compiler/rustc_middle/src/mir/mod.rs | 123 ++++++++++++++++++++++----- 2 files changed, 101 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_middle/src/lib.rs b/compiler/rustc_middle/src/lib.rs index fa2dad5ce25f0..fd2b5f5335f69 100644 --- a/compiler/rustc_middle/src/lib.rs +++ b/compiler/rustc_middle/src/lib.rs @@ -59,6 +59,7 @@ #![feature(unwrap_infallible)] #![feature(decl_macro)] #![feature(drain_filter)] +#![feature(intra_doc_pointers)] #![recursion_limit = "512"] #![allow(rustc::potential_query_instability)] diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index d99e4dd815e2e..129439ae82188 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -2378,57 +2378,134 @@ impl<'tcx> Operand<'tcx> { #[derive(Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq)] /// The various kinds of rvalues that can appear in MIR. /// -/// Not all of these are allowed at every [`MirPhase`]. Check the documentation there to see which -/// ones you do not have to worry about. The MIR validator will generally enforce such restrictions, -/// causing an ICE if they are violated. +/// Not all of these are allowed at every [`MirPhase`] - when this is the case, it's stated below. +/// +/// Computing any rvalue begins by evaluating the places and operands in the rvalue in the order in +/// which they appear. These are then used to produce a "value" - the same kind of value that an +/// [`Operand`] is. pub enum Rvalue<'tcx> { - /// x (either a move or copy, depending on type of x) + /// Yields the operand unchanged Use(Operand<'tcx>), - /// [x; 32] + /// Creates an array where each element is the value of the operand. This currently does not + /// drop the value even if the number of repetitions is zero, see [#74836]. + /// + /// Corresponds to source code like `[x; 32]`. + /// + /// [#74836]: https://github.com/rust-lang/rust/issues/74836 Repeat(Operand<'tcx>, ty::Const<'tcx>), - /// &x or &mut x + /// Creates a reference of the indicated kind to the place. + /// + /// There is not much to document here, because besides the obvious parts the semantics of this + /// are essentially entirely a part of the aliasing model. There are many UCG issues discussing + /// exactly what the behavior of this operation should be. + /// + /// `Shallow` borrows are disallowed after drop lowering. Ref(Region<'tcx>, BorrowKind, Place<'tcx>), - /// Accessing a thread local static. This is inherently a runtime operation, even if llvm - /// treats it as an access to a static. This `Rvalue` yields a reference to the thread local - /// static. + /// Returns a pointer/reference to the given thread local. + /// + /// The yielded type is a `*mut T` if the static is mutable, otherwise if the static is extern a + /// `*const T`, and if neither of those apply a `&T`. + /// + /// **Note:** This is a runtime operation that actually executes code and is in this sense more + /// like a function call. Also, DSEing these causes `fn main() {}` to SIGILL for some reason + /// that I never got a chance to look into. + /// + /// **Needs clarification**: Are there weird additional semantics here related to the runtime + /// nature of this operation? ThreadLocalRef(DefId), - /// Create a raw pointer to the given place - /// Can be generated by raw address of expressions (`&raw const x`), - /// or when casting a reference to a raw pointer. + /// Creates a pointer with the indicated mutability to the place. + /// + /// This is generated by pointer casts like `&v as *const _` or raw address of expressions like + /// `&raw v` or `addr_of!(v)`. + /// + /// Like with references, the semantics of this operation are heavily dependent on the aliasing + /// model. AddressOf(Mutability, Place<'tcx>), - /// length of a `[X]` or `[X;n]` value + /// Yields the length of the place, as a `usize`. + /// + /// If the type of the place is an array, this is the array length. This also works for slices + /// (`[T]`, not `&[T]`) through some mechanism that depends on how exactly places work (see + /// there for more details). Len(Place<'tcx>), + /// Performs essentially all of the casts that can be performed via `as`. + /// + /// This allows for casts from/to a variety of types. + /// + /// **FIXME**: Document exactly which `CastKind`s allow which types of casts. Figure out why + /// `ArrayToPointer` and `MutToConstPointer` are special. Cast(CastKind, Operand<'tcx>, Ty<'tcx>), + /// * `Offset` has the same semantics as [`offset`](pointer::offset), except that the second + /// paramter may be a `usize` as well. + /// * The comparison operations accept `bool`s, `char`s, signed or unsigned integers, floats, + /// raw pointers, or function pointers and return a `bool`. + /// * Left and right shift operations accept signed or unsigned integers not necessarily of the + /// same type and return a value of the same type as their LHS. For all other operations, the + /// types of the operands must match. + /// * The `Bit*` operations accept signed integers, unsigned integers, or bools and return a + /// value of that type. + /// * The remaining operations accept signed integers, unsigned integers, or floats of any + /// matching type and return a value of that type. BinaryOp(BinOp, Box<(Operand<'tcx>, Operand<'tcx>)>), + + /// Same as `BinaryOp`, but yields `(T, bool)` instead of `T`. In addition to performing the + /// same computation as the matching `BinaryOp`, checks if the infinite precison result would be + /// unequal to the actual result and sets the `bool` if this is the case. `BinOp::Offset` is not + /// allowed here. + /// + /// **FIXME**: What about division/modulo? Are they allowed here at all? Are zero divisors still + /// UB? Also, which other combinations of types are disallowed? CheckedBinaryOp(BinOp, Box<(Operand<'tcx>, Operand<'tcx>)>), + /// Yields the size or alignment of the type as a `usize`. NullaryOp(NullOp, Ty<'tcx>), + + /// Exactly like `BinaryOp`, but less operands. + /// + /// Also does two's-complement arithmetic. Negation requires a signed integer or a float; binary + /// not requires a signed integer, unsigned integer, or bool. Both operation kinds return a + /// value with the same type as their operand. UnaryOp(UnOp, Operand<'tcx>), - /// Read the discriminant of an ADT. + /// Computes the discriminant of the place, returning it as an integer of type + /// [`discriminant_ty`]. + /// + /// The validity requirements for the underlying value are undecided for this rvalue, see + /// [#91095]. Note too that the value of the discriminant is not the same thing as the + /// variant index; use [`discriminant_for_variant`] to convert. + /// + /// For types defined in the source code as enums, this is well behaved. This is also well + /// formed for other types, but yields no particular value - there is no reason it couldn't be + /// defined to yield eg zero though. /// - /// Undefined (i.e., no effort is made to make it defined, but there’s no reason why it cannot - /// be defined to return, say, a 0) if ADT is not an enum. + /// [`discriminant_ty`]: crate::ty::Ty::discriminant_ty + /// [#91095]: https://github.com/rust-lang/rust/issues/91095 + /// [`discriminant_for_variant`]: crate::ty::Ty::discriminant_for_variant Discriminant(Place<'tcx>), - /// Creates an aggregate value, like a tuple or struct. This is - /// only needed because we want to distinguish `dest = Foo { x: - /// ..., y: ... }` from `dest.x = ...; dest.y = ...;` in the case - /// that `Foo` has a destructor. These rvalues can be optimized - /// away after type-checking and before lowering. + /// Creates an aggregate value, like a tuple or struct. + /// + /// This is needed because dataflow analysis needs to distinguish + /// `dest = Foo { x: ..., y: ... }` from `dest.x = ...; dest.y = ...;` in the case that `Foo` + /// has a destructor. + /// + /// Disallowed after deaggregation for all aggregate kinds except `Array` and `Generator`. After + /// generator lowering, `Generator` aggregate kinds are disallowed too. Aggregate(Box>, Vec>), /// Transmutes a `*mut u8` into shallow-initialized `Box`. /// - /// This is different a normal transmute because dataflow analysis will treat the box - /// as initialized but its content as uninitialized. + /// This is different a normal transmute because dataflow analysis will treat the box as + /// initialized but its content as uninitialized. Like other pointer casts, this in general + /// affects alias analysis. + /// + /// Disallowed after drop elaboration. ShallowInitBox(Operand<'tcx>, Ty<'tcx>), } From 5fc86769e65687d5993802bab04d34d77d7629c0 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Thu, 24 Mar 2022 22:30:23 -0400 Subject: [PATCH 05/25] Extend the MIR validator to check many more things around rvalues. --- .../src/transform/validate.rs | 200 ++++++++++++++---- src/test/mir-opt/lower_intrinsics.rs | 2 +- ...r_intrinsics.wrapping.LowerIntrinsics.diff | 40 ++-- 3 files changed, 182 insertions(+), 60 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index cbad2510749d4..552af58b033e2 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -4,14 +4,13 @@ use rustc_index::bit_set::BitSet; use rustc_infer::infer::TyCtxtInferExt; use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::visit::{PlaceContext, Visitor}; -use rustc_middle::mir::{traversal, Place}; use rustc_middle::mir::{ - AggregateKind, BasicBlock, Body, BorrowKind, Local, Location, MirPass, MirPhase, Operand, - PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, StatementKind, Terminator, - TerminatorKind, START_BLOCK, + traversal, AggregateKind, BasicBlock, BinOp, Body, BorrowKind, Local, Location, MirPass, + MirPhase, Operand, Place, PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, + StatementKind, Terminator, TerminatorKind, UnOp, START_BLOCK, }; use rustc_middle::ty::fold::BottomUpFolder; -use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt, TypeFoldable}; +use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable}; use rustc_mir_dataflow::impls::MaybeStorageLive; use rustc_mir_dataflow::storage::AlwaysLiveLocals; use rustc_mir_dataflow::{Analysis, ResultsCursor}; @@ -36,6 +35,13 @@ pub struct Validator { impl<'tcx> MirPass<'tcx> for Validator { fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { + // FIXME(JakobDegen): These bodies never instantiated in codegend anyway, so it's not + // terribly important that they pass the validator. However, I think other passes might + // still see them, in which case they might be surprised. It would probably be better if we + // didn't put this through the MIR pipeline at all. + if matches!(body.source.instance, InstanceDef::Intrinsic(..) | InstanceDef::Virtual(..)) { + return; + } let def_id = body.source.def_id(); let param_env = tcx.param_env(def_id); let mir_phase = self.mir_phase; @@ -248,58 +254,174 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } } - fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { - match &statement.kind { - StatementKind::Assign(box (dest, rvalue)) => { - // LHS and RHS of the assignment must have the same type. - let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; - let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); - if !self.mir_assign_valid_types(right_ty, left_ty) { + fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { + macro_rules! check_kinds { + ($t:expr, $text:literal, $($patterns:tt)*) => { + if !matches!(($t).kind(), $($patterns)*) { + self.fail(location, format!($text, $t)); + } + }; + } + match rvalue { + Rvalue::Use(_) => {} + Rvalue::Aggregate(agg_kind, _) => { + let disallowed = match **agg_kind { + AggregateKind::Array(..) => false, + AggregateKind::Generator(..) => self.mir_phase >= MirPhase::GeneratorsLowered, + _ => self.mir_phase >= MirPhase::Deaggregated, + }; + if disallowed { self.fail( location, - format!( - "encountered `{:?}` with incompatible types:\n\ - left-hand side has type: {}\n\ - right-hand side has type: {}", - statement.kind, left_ty, right_ty, - ), + format!("{:?} have been lowered to field assignments", rvalue), + ) + } + } + Rvalue::Ref(_, BorrowKind::Shallow, _) => { + if self.mir_phase >= MirPhase::DropsLowered { + self.fail( + location, + "`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase", ); } - match rvalue { - // The sides of an assignment must not alias. Currently this just checks whether the places - // are identical. - Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => { - if dest == src { + } + Rvalue::Len(p) => { + let pty = p.ty(&self.body.local_decls, self.tcx).ty; + check_kinds!( + pty, + "Cannot compute length of non-array type {:?}", + ty::Array(..) | ty::Slice(..) + ); + } + Rvalue::BinaryOp(op, vals) | Rvalue::CheckedBinaryOp(op, vals) => { + use BinOp::*; + let a = vals.0.ty(&self.body.local_decls, self.tcx); + let b = vals.1.ty(&self.body.local_decls, self.tcx); + match op { + Offset => { + check_kinds!(a, "Cannot offset non-pointer type {:?}", ty::RawPtr(..)); + if b != self.tcx.types.isize && b != self.tcx.types.usize { + self.fail(location, format!("Cannot offset by non-isize type {:?}", b)); + } + } + Eq | Lt | Le | Ne | Ge | Gt => { + for x in [a, b] { + check_kinds!( + x, + "Cannot compare type {:?}", + ty::Bool + | ty::Char + | ty::Int(..) + | ty::Uint(..) + | ty::Float(..) + | ty::RawPtr(..) + | ty::FnPtr(..) + ) + } + // None of the possible types have lifetimes, so we can just compare + // directly + if a != b { self.fail( location, - "encountered `Assign` statement with overlapping memory", + format!("Cannot compare unequal types {:?} and {:?}", a, b), ); } } - Rvalue::Aggregate(agg_kind, _) => { - let disallowed = match **agg_kind { - AggregateKind::Array(..) => false, - AggregateKind::Generator(..) => { - self.mir_phase >= MirPhase::GeneratorsLowered - } - _ => self.mir_phase >= MirPhase::Deaggregated, - }; - if disallowed { + Shl | Shr => { + for x in [a, b] { + check_kinds!( + x, + "Cannot shift non-integer type {:?}", + ty::Uint(..) | ty::Int(..) + ) + } + } + BitAnd | BitOr | BitXor => { + for x in [a, b] { + check_kinds!( + x, + "Cannot perform bitwise op on type {:?}", + ty::Uint(..) | ty::Int(..) | ty::Bool + ) + } + if a != b { self.fail( location, - format!("{:?} have been lowered to field assignments", rvalue), - ) + format!( + "Cannot perform bitwise op on unequal types {:?} and {:?}", + a, b + ), + ); } } - Rvalue::Ref(_, BorrowKind::Shallow, _) => { - if self.mir_phase >= MirPhase::DropsLowered { + Add | Sub | Mul | Div | Rem => { + for x in [a, b] { + check_kinds!( + x, + "Cannot perform op on type {:?}", + ty::Uint(..) | ty::Int(..) | ty::Float(..) + ) + } + if a != b { self.fail( location, - "`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase", + format!("Cannot perform op on unequal types {:?} and {:?}", a, b), ); } } - _ => {} + } + } + Rvalue::UnaryOp(op, operand) => { + let a = operand.ty(&self.body.local_decls, self.tcx); + match op { + UnOp::Neg => { + check_kinds!(a, "Cannot negate type {:?}", ty::Int(..) | ty::Float(..)) + } + UnOp::Not => { + check_kinds!( + a, + "Cannot binary not type {:?}", + ty::Int(..) | ty::Uint(..) | ty::Bool + ); + } + } + } + Rvalue::ShallowInitBox(operand, _) => { + let a = operand.ty(&self.body.local_decls, self.tcx); + check_kinds!(a, "Cannot shallow init type {:?}", ty::RawPtr(..)); + } + _ => {} + } + self.super_rvalue(rvalue, location); + } + + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + match &statement.kind { + StatementKind::Assign(box (dest, rvalue)) => { + // LHS and RHS of the assignment must have the same type. + let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; + let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); + if !self.mir_assign_valid_types(right_ty, left_ty) { + self.fail( + location, + format!( + "encountered `{:?}` with incompatible types:\n\ + left-hand side has type: {}\n\ + right-hand side has type: {}", + statement.kind, left_ty, right_ty, + ), + ); + } + // FIXME(JakobDegen): Check this for all rvalues, not just this one. + if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue { + // The sides of an assignment must not alias. Currently this just checks whether + // the places are identical. + if dest == src { + self.fail( + location, + "encountered `Assign` statement with overlapping memory", + ); + } } } StatementKind::AscribeUserType(..) => { diff --git a/src/test/mir-opt/lower_intrinsics.rs b/src/test/mir-opt/lower_intrinsics.rs index 8a8880dad02e5..eab51b65f1a19 100644 --- a/src/test/mir-opt/lower_intrinsics.rs +++ b/src/test/mir-opt/lower_intrinsics.rs @@ -3,7 +3,7 @@ #![crate_type = "lib"] // EMIT_MIR lower_intrinsics.wrapping.LowerIntrinsics.diff -pub fn wrapping(a: T, b: T) { +pub fn wrapping(a: i32, b: i32) { let _x = core::intrinsics::wrapping_add(a, b); let _y = core::intrinsics::wrapping_sub(a, b); let _z = core::intrinsics::wrapping_mul(a, b); diff --git a/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff b/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff index a531a19bd7820..5a0286bad2fb7 100644 --- a/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff +++ b/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff @@ -1,23 +1,23 @@ - // MIR for `wrapping` before LowerIntrinsics + // MIR for `wrapping` after LowerIntrinsics - fn wrapping(_1: T, _2: T) -> () { - debug a => _1; // in scope 0 at $DIR/lower_intrinsics.rs:6:26: 6:27 - debug b => _2; // in scope 0 at $DIR/lower_intrinsics.rs:6:32: 6:33 - let mut _0: (); // return place in scope 0 at $DIR/lower_intrinsics.rs:6:38: 6:38 - let _3: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11 - let mut _4: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46 - let mut _5: T; // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 - let mut _7: T; // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46 - let mut _8: T; // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49 - let mut _10: T; // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46 - let mut _11: T; // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49 + fn wrapping(_1: i32, _2: i32) -> () { + debug a => _1; // in scope 0 at $DIR/lower_intrinsics.rs:6:17: 6:18 + debug b => _2; // in scope 0 at $DIR/lower_intrinsics.rs:6:25: 6:26 + let mut _0: (); // return place in scope 0 at $DIR/lower_intrinsics.rs:6:33: 6:33 + let _3: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11 + let mut _4: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46 + let mut _5: i32; // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 + let mut _7: i32; // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46 + let mut _8: i32; // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49 + let mut _10: i32; // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46 + let mut _11: i32; // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49 scope 1 { debug _x => _3; // in scope 1 at $DIR/lower_intrinsics.rs:7:9: 7:11 - let _6: T; // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11 + let _6: i32; // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11 scope 2 { debug _y => _6; // in scope 2 at $DIR/lower_intrinsics.rs:8:9: 8:11 - let _9: T; // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11 + let _9: i32; // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11 scope 3 { debug _z => _9; // in scope 3 at $DIR/lower_intrinsics.rs:9:9: 9:11 } @@ -30,10 +30,10 @@ _4 = _1; // scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46 StorageLive(_5); // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 _5 = _2; // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49 -- _3 = wrapping_add::(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 +- _3 = wrapping_add::(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:7:14: 7:44 -- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_add::}, val: Value(Scalar()) } +- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_add::}, val: Value(Scalar()) } + _3 = Add(move _4, move _5); // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 + goto -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50 } @@ -46,10 +46,10 @@ _7 = _1; // scope 1 at $DIR/lower_intrinsics.rs:8:45: 8:46 StorageLive(_8); // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49 _8 = _2; // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49 -- _6 = wrapping_sub::(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 +- _6 = wrapping_sub::(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:8:14: 8:44 -- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_sub::}, val: Value(Scalar()) } +- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_sub::}, val: Value(Scalar()) } + _6 = Sub(move _7, move _8); // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 + goto -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50 } @@ -62,10 +62,10 @@ _10 = _1; // scope 2 at $DIR/lower_intrinsics.rs:9:45: 9:46 StorageLive(_11); // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49 _11 = _2; // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49 -- _9 = wrapping_mul::(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 +- _9 = wrapping_mul::(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:9:14: 9:44 -- // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_mul::}, val: Value(Scalar()) } +- // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_mul::}, val: Value(Scalar()) } + _9 = Mul(move _10, move _11); // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 + goto -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50 } @@ -73,7 +73,7 @@ bb3: { StorageDead(_11); // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50 StorageDead(_10); // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50 - _0 = const (); // scope 0 at $DIR/lower_intrinsics.rs:6:38: 10:2 + _0 = const (); // scope 0 at $DIR/lower_intrinsics.rs:6:33: 10:2 StorageDead(_9); // scope 2 at $DIR/lower_intrinsics.rs:10:1: 10:2 StorageDead(_6); // scope 1 at $DIR/lower_intrinsics.rs:10:1: 10:2 StorageDead(_3); // scope 0 at $DIR/lower_intrinsics.rs:10:1: 10:2 From 148beafc5f480f08f4e376a6b1fdae61d0e7f4df Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 25 Mar 2022 02:17:05 -0400 Subject: [PATCH 06/25] Improve documentation for MIR statement kinds. --- compiler/rustc_middle/src/mir/mod.rs | 92 +++++++++++++++++++++++----- 1 file changed, 76 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 129439ae82188..68b0074d7f889 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1573,34 +1573,79 @@ impl Statement<'_> { /// causing an ICE if they are violated. #[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable)] pub enum StatementKind<'tcx> { - /// Write the RHS Rvalue to the LHS Place. + /// Assign statements roughly correspond to an assignment in Rust proper (`x = ...`) except + /// without the possibility of dropping the previous value (that must be done separately, if at + /// all). The *exact* way this works is undecided. It probably does something like evaluating + /// the LHS and RHS, and then doing the inverse of a place to value conversion to write the + /// resulting value into memory. Various parts of this may do type specific things that are more + /// complicated than simply copying over the bytes depending on the types. /// - /// The LHS place may not overlap with any memory accessed on the RHS. + /// **Needs clarification**: The implication of the above idea would be that assignment implies + /// that the resulting value is initialized. I believe we could commit to this separately from + /// committing to whatever part of the memory model we would need to decide on to make the above + /// paragragh precise. Do we want to? + /// + /// Assignments in which the types of the place and rvalue differ are not well-formed. + /// + /// **Needs clarification**: Do we ever want to worry about non-free (in the body) lifetimes for + /// the typing requirement in post drop-elaboration MIR? I think probably not - I'm not sure we + /// could meaningfully require this anyway. How about free lifetimes? Is ignoring this + /// interesting for optimizations? Do we want to allow such optimizations? + /// + /// **Needs clarification**: We currently require that the LHS place not overlap with any place + /// read as part of computation of the RHS. This requirement is under discussion in [#68364]. As + /// a part of this discussion, it is also unclear in what order the components are evaluated. + /// + /// [#68364]: https://github.com/rust-lang/rust/issues/68364 + /// + /// See [`Rvalue`] documentation for details on each of those. Assign(Box<(Place<'tcx>, Rvalue<'tcx>)>), - /// This represents all the reading that a pattern match may do - /// (e.g., inspecting constants and discriminant values), and the - /// kind of pattern it comes from. This is in order to adapt potential - /// error messages to these specific patterns. + /// This represents all the reading that a pattern match may do (e.g., inspecting constants and + /// discriminant values), and the kind of pattern it comes from. This is in order to adapt + /// potential error messages to these specific patterns. /// /// Note that this also is emitted for regular `let` bindings to ensure that locals that are /// never accessed still get some sanity checks for, e.g., `let x: ! = ..;` + /// + /// When executed at runtime this is a nop. + /// + /// Disallowed after drop elaboration. FakeRead(Box<(FakeReadCause, Place<'tcx>)>), /// Write the discriminant for a variant to the enum Place. SetDiscriminant { place: Box>, variant_index: VariantIdx }, - /// Start a live range for the storage of the local. + /// `StorageLive` and `StorageDead` statements mark the live range of a local. + /// + /// Using a local before a `StorageLive` or after a `StorageDead` is not well-formed. These + /// statements are not required. If the entire MIR body contains no `StorageLive`/`StorageDead` + /// statements for a particular local, the local is always considered live. + /// + /// More precisely, the MIR validator currently does a `MaybeLiveLocals` analysis to check + /// validity of each use of a local. I believe this is equivalent to requiring for every use of + /// a local, there exist at least one path from the root to that use that contains a + /// `StorageLive` more recently than a `StorageDead`. + /// + /// **Needs clarification**: Is it permitted to `StorageLive` a local for which we previously + /// executed `StorageDead`? How about two `StorageLive`s without an intervening `StorageDead`? + /// Two `StorageDead`s without an intervening `StorageLive`? LLVM says yes, poison, yes. If the + /// answer to any of these is "no," is breaking that rule UB or is it an error to have a path in + /// the CFG that might do this? StorageLive(Local), - /// End the current live range for the storage of the local. + /// See `StorageLive` above. StorageDead(Local), - /// Retag references in the given place, ensuring they got fresh tags. This is - /// part of the Stacked Borrows model. These statements are currently only interpreted - /// by miri and only generated when "-Z mir-emit-retag" is passed. - /// See - /// for more details. + /// Retag references in the given place, ensuring they got fresh tags. + /// + /// This is part of the Stacked Borrows model. These statements are currently only interpreted + /// by miri and only generated when `-Z mir-emit-retag` is passed. See + /// for + /// more details. + /// + /// For code that is not specific to stacked borrows, you should consider statements to read + /// and modify the place in an opaque way. Retag(RetagKind, Box>), /// Encodes a user's type ascription. These need to be preserved @@ -1615,6 +1660,10 @@ pub enum StatementKind<'tcx> { /// - `Contravariant` -- requires that `T_y :> T` /// - `Invariant` -- requires that `T_y == T` /// - `Bivariant` -- no effect + /// + /// When executed at runtime this is a nop. + /// + /// Disallowed after drop elaboration. AscribeUserType(Box<(Place<'tcx>, UserTypeProjection)>, ty::Variance), /// Marks the start of a "coverage region", injected with '-Cinstrument-coverage'. A @@ -1624,9 +1673,20 @@ pub enum StatementKind<'tcx> { /// executed. Coverage(Box), - /// Denotes a call to the intrinsic function copy_overlapping, where `src_dst` denotes the - /// memory being read from and written to(one field to save memory), and size - /// indicates how many bytes are being copied over. + /// Denotes a call to the intrinsic function `copy_overlapping`. + /// + /// First, all three operands are evaluated. `src` and `dest` must each be a reference, pointer, + /// or `Box` pointing to the same type `T`. `count` must evaluate to a `usize`. Then, `src` and + /// `dest` are dereferenced, and `count * size_of::()` bytes beginning with the first byte of + /// the `src` place are copied to the continguous range of bytes beginning with the first byte + /// of `dest`. + /// + /// **Needs clarification**: In what order are operands computed and dereferenced? It should + /// probably match the order for assignment, but that is also undecided. + /// + /// **Needs clarification**: Is this typed or not, ie is there a place to value and back + /// conversion involved? I vaguely remember Ralf saying somewhere that he thought it should not + /// be. CopyNonOverlapping(Box>), /// No-op. Useful for deleting instructions without affecting statement indices. From c996bc0f48bb8602369f9aca58f22ccb7200ed05 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 25 Mar 2022 20:00:16 -0400 Subject: [PATCH 07/25] Improve documentation for MIR terminators --- compiler/rustc_middle/src/mir/terminator.rs | 148 ++++++++++++++++---- 1 file changed, 121 insertions(+), 27 deletions(-) diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs index ae94bd121f953..bf68835235da6 100644 --- a/compiler/rustc_middle/src/mir/terminator.rs +++ b/compiler/rustc_middle/src/mir/terminator.rs @@ -105,13 +105,34 @@ impl<'a> Iterator for SwitchTargetsIter<'a> { impl<'a> ExactSizeIterator for SwitchTargetsIter<'a> {} +/// A note on unwinding: Panics may occur during the execution of some terminators. Depending on the +/// `-C panic` flag, this may either cause the program to abort or the call stack to unwind. Such +/// terminators have a `cleanup: Option` field on them. If stack unwinding occurs, then +/// once the current function is reached, execution continues at the given basic block, if any. If +/// `cleanup` is `None` then no cleanup is performed, and the stack continues unwinding. This is +/// equivalent to the execution of a `Resume` terminator. +/// +/// The basic block pointed to by a `cleanup` field must have its `cleanup` flag set. `cleanup` +/// basic blocks have a couple restrictions: +/// 1. All `cleanup` fields in them must be `None`. +/// 2. `Return` terminators are not allowed in them. `Abort` and `Unwind` terminators are. +/// 3. All other basic blocks (in the current body) that are reachable from `cleanup` basic blocks +/// must also be `cleanup`. This is a part of the type system and checked statically, so it is +/// still an error to have such an edge in the CFG even if it's known that it won't be taken at +/// runtime. #[derive(Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq)] pub enum TerminatorKind<'tcx> { - /// Block should have one successor in the graph; we jump there. + /// Block has one successor; we continue execution there. Goto { target: BasicBlock }, - /// Operand evaluates to an integer; jump depending on its value - /// to one of the targets, and otherwise fallback to `otherwise`. + /// Switches based on the computed value. + /// + /// First, evaluates the `discr` operand. The type of the operand must be a signed or unsigned + /// integer, char, or bool, and must match the given type. Then, if the list of switch targets + /// contains the computed value, continues execution at the associated basic block. Otherwise, + /// continues execution at the "otherwise" basic block. + /// + /// Target values may not appear more than once. SwitchInt { /// The discriminant value being tested. discr: Operand<'tcx>, @@ -124,29 +145,62 @@ pub enum TerminatorKind<'tcx> { targets: SwitchTargets, }, - /// Indicates that the landing pad is finished and unwinding should - /// continue. Emitted by `build::scope::diverge_cleanup`. + /// Indicates that the landing pad is finished and that the process should continue unwinding. + /// + /// Like a return, this marks the end of this invocation of the function. + /// + /// Only permitted in cleanup blocks. `Resume` is not permitted with `-C unwind=abort` after + /// deaggregation runs. Resume, - /// Indicates that the landing pad is finished and that the process - /// should abort. Used to prevent unwinding for foreign items. + /// Indicates that the landing pad is finished and that the process should abort. + /// + /// Used to prevent unwinding for foreign items or with `-C unwind=abort`. Only permitted in + /// cleanup blocks. Abort, - /// Indicates a normal return. The return place should have - /// been filled in before this executes. This can occur multiple times - /// in different basic blocks. + /// Returns from the function. + /// + /// Like function calls, the exact semantics of returns in Rust are unclear. Returning very + /// likely at least assigns the value currently in the return place (`_0`) to the place + /// specified in the associated `Call` terminator in the calling function, as if assigned via + /// `dest = move _0`. It might additionally do other things, like have side-effects in the + /// aliasing model. + /// + /// If the body is a generator body, this has slightly different semantics; it instead causes a + /// `GeneratorState::Returned(_0)` to be created (as if by an `Aggregate` rvalue) and assigned + /// to the return place. Return, /// Indicates a terminator that can never be reached. + /// + /// Executing this terminator is UB. Unreachable, - /// Drop the `Place`. + /// The behavior of this statement differs significantly before and after drop elaboration. + /// After drop elaboration, `Drop` executes the drop glue for the specified place, after which + /// it continues execution/unwinds at the given basic blocks. It is possible that executing drop + /// glue is special - this would be part of Rust's memory model. (**FIXME**: due we have an + /// issue tracking if drop glue has any interesting semantics in addition to those of a function + /// call?) + /// + /// `Drop` before drop elaboration is a *conditional* execution of the drop glue. Specifically, the + /// `Drop` will be executed if... + /// + /// **Needs clarification**: End of that sentence. This in effect should document the exact + /// behavior of drop elaboration. The following sounds vaguely right, but I'm not quite sure: + /// + /// > The drop glue is executed if, among all statements executed within this `Body`, an assignment to + /// > the place or one of its "parents" occurred more recently than a move out of it. This does not + /// > consider indirect assignments. Drop { place: Place<'tcx>, target: BasicBlock, unwind: Option }, - /// Drop the `Place` and assign the new value over it. This ensures - /// that the assignment to `P` occurs *even if* the destructor for - /// place unwinds. Its semantics are best explained by the - /// elaboration: + /// Drops the place and assigns a new value to it. + /// + /// This first performs the exact same operation as the pre drop-elaboration `Drop` terminator; + /// it then additionally assigns the `value` to the `place` as if by an assignment statement. + /// This assignment occurs both in the unwind and the regular code paths. The semantics are best + /// explained by the elaboration: /// /// ``` /// BB0 { @@ -170,7 +224,7 @@ pub enum TerminatorKind<'tcx> { /// } /// ``` /// - /// Note that DropAndReplace is eliminated as part of the `ElaborateDrops` pass. + /// Disallowed after drop elaboration. DropAndReplace { place: Place<'tcx>, value: Operand<'tcx>, @@ -178,7 +232,14 @@ pub enum TerminatorKind<'tcx> { unwind: Option, }, - /// Block ends with a call of a function. + /// Roughly speaking, evaluates the `func` operand and the arguments, and starts execution of + /// the referred to function. The operand types must match the argument types of the function. + /// The return place type must exactly match the return type. The type of the `func` operand + /// must be callable, meaning either a function pointer, a function type, or a closure type. + /// + /// **Needs clarification**: The exact semantics of this, see [#71117]. + /// + /// [#71117]: https://github.com/rust-lang/rust/issues/71117 Call { /// The function that’s being called. func: Operand<'tcx>, @@ -187,7 +248,7 @@ pub enum TerminatorKind<'tcx> { /// This allows the memory occupied by "by-value" arguments to be /// reused across function calls without duplicating the contents. args: Vec>, - /// Destination for the return value. If some, the call is converging. + /// Destination for the return value. If none, the call necessarily diverges. destination: Option<(Place<'tcx>, BasicBlock)>, /// Cleanups to be done if the call unwinds. cleanup: Option, @@ -199,8 +260,12 @@ pub enum TerminatorKind<'tcx> { fn_span: Span, }, - /// Jump to the target if the condition has the expected value, - /// otherwise panic with a message and a cleanup target. + /// Evaluates the operand, which must have type `bool`. If it is not equal to `expected`, + /// initiates a panic. Initiating a panic corresponds to a `Call` terminator with some + /// unspecified constant as the function to call, all the operands stored in the `AssertMessage` + /// as parameters, and `None` for the destination. Keep in mind that the `cleanup` path is not + /// necessarily executed even in the case of a panic, for example in `-C panic=abort`. If the + /// assertion does not fail, execution continues at the specified basic block. Assert { cond: Operand<'tcx>, expected: bool, @@ -209,7 +274,18 @@ pub enum TerminatorKind<'tcx> { cleanup: Option, }, - /// A suspend point. + /// Marks a suspend point. + /// + /// Like `Return` terminators in generator bodies, this computes `value` and then a + /// `GeneratorState::Yielded(value)` as if by `Aggregate` rvalue. That value is then assigned to + /// the return place of the function calling this one, and execution continues in the calling + /// function. When next invoked with the same first argument, execution of this function + /// continues at the `resume` basic block, with the second argument written to the `resume_arg` + /// place. If the generator is dropped before then, the `drop` basic block is invoked. + /// + /// Not permitted in bodies that are not generator bodies, or after generator lowering. + /// + /// **Needs clarification**: What about the evaluation order of the `resume_arg` and `value`? Yield { /// The value to return. value: Operand<'tcx>, @@ -221,11 +297,24 @@ pub enum TerminatorKind<'tcx> { drop: Option, }, - /// Indicates the end of the dropping of a generator. + /// Indicates the end of dropping a generator. + /// + /// Semantically just a `return` (from the generators drop glue). Only permitted in the same situations + /// as `yield`. + /// + /// **Needs clarification**: Is that even correct? The generator drop code is always confusing + /// to me, because it's not even really in the current body. + /// + /// **Needs clarification**: Are there type system constraints on these terminators? Should + /// there be a "block type" like `cleanup` blocks for them? GeneratorDrop, - /// A block where control flow only ever takes one real path, but borrowck - /// needs to be more conservative. + /// A block where control flow only ever takes one real path, but borrowck needs to be more + /// conservative. + /// + /// At runtime this is semantically just a goto. + /// + /// Disallowed after drop elaboration. FalseEdge { /// The target normal control flow will take. real_target: BasicBlock, @@ -233,9 +322,14 @@ pub enum TerminatorKind<'tcx> { /// practice. imaginary_target: BasicBlock, }, - /// A terminator for blocks that only take one path in reality, but where we - /// reserve the right to unwind in borrowck, even if it won't happen in practice. - /// This can arise in infinite loops with no function calls for example. + + /// A terminator for blocks that only take one path in reality, but where we reserve the right + /// to unwind in borrowck, even if it won't happen in practice. This can arise in infinite loops + /// with no function calls for example. + /// + /// At runtime this is semantically just a goto. + /// + /// Disallowed after drop elaboration. FalseUnwind { /// The target normal control flow will take. real_target: BasicBlock, From 3c169f301cc8af7406d4fb132e9e9bf243a930ae Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 25 Mar 2022 20:00:33 -0400 Subject: [PATCH 08/25] Adjust MIR validator to check a few more things for terminators --- .../src/transform/validate.rs | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 552af58b033e2..45e71d3388d93 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -627,6 +627,9 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } } TerminatorKind::Yield { resume, drop, .. } => { + if self.body.generator.is_none() { + self.fail(location, "`Yield` cannot appear outside generator bodies"); + } if self.mir_phase >= MirPhase::GeneratorsLowered { self.fail(location, "`Yield` should have been replaced by generator lowering"); } @@ -666,6 +669,9 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } } TerminatorKind::GeneratorDrop => { + if self.body.generator.is_none() { + self.fail(location, "`GeneratorDrop` cannot appear outside generator bodies"); + } if self.mir_phase >= MirPhase::GeneratorsLowered { self.fail( location, @@ -673,11 +679,19 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ); } } - // Nothing to validate for these. - TerminatorKind::Resume - | TerminatorKind::Abort - | TerminatorKind::Return - | TerminatorKind::Unreachable => {} + TerminatorKind::Resume | TerminatorKind::Abort => { + let bb = location.block; + if !self.body.basic_blocks()[bb].is_cleanup { + self.fail(location, "Cannot `Resume` from non-cleanup basic block") + } + } + TerminatorKind::Return => { + let bb = location.block; + if self.body.basic_blocks()[bb].is_cleanup { + self.fail(location, "Cannot `Return` from cleanup basic block") + } + } + TerminatorKind::Unreachable => {} } self.super_terminator(terminator, location); From 8ef4af780c7078c65c194b19fff0019784b00566 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 25 Mar 2022 20:08:35 -0400 Subject: [PATCH 09/25] Improve MIR phases documentation with summaries of changes --- compiler/rustc_middle/src/mir/mod.rs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 68b0074d7f889..4ff07dd70ba40 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -127,12 +127,24 @@ pub trait MirPass<'tcx> { /// The various "big phases" that MIR goes through. /// /// These phases all describe dialects of MIR. Since all MIR uses the same datastructures, the -/// dialects forbid certain variants or values in certain phases. +/// dialects forbid certain variants or values in certain phases. The sections below summarize the +/// changes, but do not document them thoroughly. The full documentation is found in the appropriate +/// documentation for the thing the change is affecting. /// /// Warning: ordering of variants is significant. #[derive(Copy, Clone, TyEncodable, TyDecodable, Debug, PartialEq, Eq, PartialOrd, Ord)] #[derive(HashStable)] pub enum MirPhase { + /// The dialect of MIR used during all phases before `DropsLowered` is the same. This is also + /// the MIR that analysis such as borrowck uses. + /// + /// One important thing to remember about the behavior of this section of MIR is that drop terminators + /// (including drop and replace) are *conditional*. The elaborate drops pass will then replace each + /// instance of a drop terminator with a nop, an unconditional drop, or a drop conditioned on a drop + /// flag. Of course, this means that it is important that the drop elaboration can accurately recognize + /// when things are initialized and when things are de-initialized. That means any code running on this + /// version of MIR must be sure to produce output that drop elaboration can reason about. See the + /// section on the drop terminatorss for more details. Built = 0, // FIXME(oli-obk): it's unclear whether we still need this phase (and its corresponding query). // We used to have this for pre-miri MIR based const eval. @@ -162,6 +174,16 @@ pub enum MirPhase { /// And the following variant is allowed: /// * [`StatementKind::SetDiscriminant`] Deaggregated = 4, + /// Before this phase, generators are in the "source code" form, featuring `yield` statements + /// and such. With this phase change, they are transformed into a proper state machine. Running + /// optimizations before this change can be potentially dangerous because the source code is to + /// some extent a "lie." In particular, `yield` terminators effectively make the value of all + /// locals visible to the caller. This means that dead store elimination before them, or code + /// motion across them, is not correct in general. This is also exasperated by type checking + /// having pre-computed a list of the types that it thinks are ok to be live across a yield + /// point - this is necessary to decide eg whether autotraits are implemented. Introducing new + /// types across a yield point will lead to ICEs becaues of this. + /// /// Beginning with this phase, the following variants are disallowed: /// * [`TerminatorKind::Yield`](terminator::TerminatorKind::Yield) /// * [`TerminatorKind::GeneratorDrop](terminator::TerminatorKind::GeneratorDrop) From 14fb427e7cd2f51da30a692492e597b79322990f Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Sat, 26 Mar 2022 20:46:56 -0400 Subject: [PATCH 10/25] Address various comments and change some details around place to value conversions --- .../src/transform/validate.rs | 2 +- compiler/rustc_middle/src/mir/mod.rs | 63 +++++++++---------- compiler/rustc_middle/src/mir/terminator.rs | 8 ++- 3 files changed, 37 insertions(+), 36 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 45e71d3388d93..a01e4512e9a1e 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -682,7 +682,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { TerminatorKind::Resume | TerminatorKind::Abort => { let bb = location.block; if !self.body.basic_blocks()[bb].is_cleanup { - self.fail(location, "Cannot `Resume` from non-cleanup basic block") + self.fail(location, "Cannot `Resume` or `Abort` from non-cleanup basic block") } } TerminatorKind::Return => { diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 4ff07dd70ba40..7701fd0a71a3b 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1644,16 +1644,15 @@ pub enum StatementKind<'tcx> { /// statements are not required. If the entire MIR body contains no `StorageLive`/`StorageDead` /// statements for a particular local, the local is always considered live. /// - /// More precisely, the MIR validator currently does a `MaybeLiveLocals` analysis to check - /// validity of each use of a local. I believe this is equivalent to requiring for every use of - /// a local, there exist at least one path from the root to that use that contains a + /// More precisely, the MIR validator currently does a `MaybeStorageLiveLocals` analysis to + /// check validity of each use of a local. I believe this is equivalent to requiring for every + /// use of a local, there exist at least one path from the root to that use that contains a /// `StorageLive` more recently than a `StorageDead`. /// - /// **Needs clarification**: Is it permitted to `StorageLive` a local for which we previously - /// executed `StorageDead`? How about two `StorageLive`s without an intervening `StorageDead`? - /// Two `StorageDead`s without an intervening `StorageLive`? LLVM says yes, poison, yes. If the - /// answer to any of these is "no," is breaking that rule UB or is it an error to have a path in - /// the CFG that might do this? + /// **Needs clarification**: Is it permitted to have two `StorageLive`s without an intervening + /// `StorageDead`? Two `StorageDead`s without an intervening `StorageLive`? LLVM says poison, + /// yes. If the answer to any of these is "no," is breaking that rule UB or is it an error to + /// have a path in the CFG that might do this? StorageLive(Local), /// See `StorageLive` above. @@ -1666,7 +1665,7 @@ pub enum StatementKind<'tcx> { /// for /// more details. /// - /// For code that is not specific to stacked borrows, you should consider statements to read + /// For code that is not specific to stacked borrows, you should consider retags to read /// and modify the place in an opaque way. Retag(RetagKind, Box>), @@ -1695,7 +1694,7 @@ pub enum StatementKind<'tcx> { /// executed. Coverage(Box), - /// Denotes a call to the intrinsic function `copy_overlapping`. + /// Denotes a call to the intrinsic function `copy_nonoverlapping`. /// /// First, all three operands are evaluated. `src` and `dest` must each be a reference, pointer, /// or `Box` pointing to the same type `T`. `count` must evaluate to a `usize`. Then, `src` and @@ -1909,7 +1908,7 @@ pub struct CopyNonOverlapping<'tcx> { /// **Needs clarification**: What about metadata resulting from dereferencing wide pointers (and /// possibly from accessing unsized locals - not sure how those work)? That probably deserves to go /// on the list above and be discussed too. It is also probably necessary for making the indexing -/// stuff lass hand-wavey. +/// stuff less hand-wavey. /// /// **Needs clarification**: When it says "part of memory" what does that mean precisely, and how /// does it interact with the metadata? @@ -2324,13 +2323,13 @@ pub struct SourceScopeLocalData { /// /// The most common way to create values is via a place to value conversion. A place to value /// conversion is an operation which reads the memory of the place and converts it to a value. This -/// is a fundamentally *typed* operation. Different types will do different things. These are some -/// possible examples of what Rust may - but will not necessarily - decide to do on place to value -/// conversions: +/// is a fundamentally *typed* operation. The nature of the value produced depends on the type of +/// the conversion. Furthermore, there may be other effects: if the type has a validity constraint +/// the place to value conversion might be UB if the validity constraint is not met. /// -/// 1. Types with validity constraints cause UB if the validity constraint is not met -/// 2. References/pointers may have their provenance change or cause other provenance related -/// side-effects. +/// **Needs clarification:** Ralf proposes that place to value conversions not have side-effects. +/// This is what is implemented in miri today. Are these the semantics we want for MIR? Is this +/// something we can even decide without knowing more about Rust's memory model? /// /// A place to value conversion on a place that has its variant index set is not well-formed. /// However, note that this rule only applies to places appearing in MIR bodies. Many functions, @@ -2462,15 +2461,17 @@ impl<'tcx> Operand<'tcx> { /// /// Not all of these are allowed at every [`MirPhase`] - when this is the case, it's stated below. /// -/// Computing any rvalue begins by evaluating the places and operands in the rvalue in the order in -/// which they appear. These are then used to produce a "value" - the same kind of value that an -/// [`Operand`] is. +/// Computing any rvalue begins by evaluating the places and operands in some order (**Needs +/// clarification**: Which order?). These are then used to produce a "value" - the same kind of +/// value that an [`Operand`] is. pub enum Rvalue<'tcx> { /// Yields the operand unchanged Use(Operand<'tcx>), - /// Creates an array where each element is the value of the operand. This currently does not - /// drop the value even if the number of repetitions is zero, see [#74836]. + /// Creates an array where each element is the value of the operand. + /// + /// This is the cause of a bug in the case where the repetition count is zero because the value + /// is not dropped, see [#74836]. /// /// Corresponds to source code like `[x; 32]`. /// @@ -2524,12 +2525,12 @@ pub enum Rvalue<'tcx> { Cast(CastKind, Operand<'tcx>, Ty<'tcx>), /// * `Offset` has the same semantics as [`offset`](pointer::offset), except that the second - /// paramter may be a `usize` as well. + /// parameter may be a `usize` as well. /// * The comparison operations accept `bool`s, `char`s, signed or unsigned integers, floats, /// raw pointers, or function pointers and return a `bool`. /// * Left and right shift operations accept signed or unsigned integers not necessarily of the /// same type and return a value of the same type as their LHS. For all other operations, the - /// types of the operands must match. + /// types of the operands must match. Like in Rust, the RHS is truncated as needed. /// * The `Bit*` operations accept signed integers, unsigned integers, or bools and return a /// value of that type. /// * The remaining operations accept signed integers, unsigned integers, or floats of any @@ -2538,21 +2539,19 @@ pub enum Rvalue<'tcx> { /// Same as `BinaryOp`, but yields `(T, bool)` instead of `T`. In addition to performing the /// same computation as the matching `BinaryOp`, checks if the infinite precison result would be - /// unequal to the actual result and sets the `bool` if this is the case. `BinOp::Offset` is not - /// allowed here. + /// unequal to the actual result and sets the `bool` if this is the case. /// - /// **FIXME**: What about division/modulo? Are they allowed here at all? Are zero divisors still - /// UB? Also, which other combinations of types are disallowed? + /// This only supports addition, subtraction, multiplication, and shift operations. CheckedBinaryOp(BinOp, Box<(Operand<'tcx>, Operand<'tcx>)>), - /// Yields the size or alignment of the type as a `usize`. + /// Computes a value as described by the operation. NullaryOp(NullOp, Ty<'tcx>), /// Exactly like `BinaryOp`, but less operands. /// - /// Also does two's-complement arithmetic. Negation requires a signed integer or a float; binary - /// not requires a signed integer, unsigned integer, or bool. Both operation kinds return a - /// value with the same type as their operand. + /// Also does two's-complement arithmetic. Negation requires a signed integer or a float; + /// bitwise not requires a signed integer, unsigned integer, or bool. Both operation kinds + /// return a value with the same type as their operand. UnaryOp(UnOp, Operand<'tcx>), /// Computes the discriminant of the place, returning it as an integer of type diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs index bf68835235da6..cc08857463d58 100644 --- a/compiler/rustc_middle/src/mir/terminator.rs +++ b/compiler/rustc_middle/src/mir/terminator.rs @@ -234,10 +234,12 @@ pub enum TerminatorKind<'tcx> { /// Roughly speaking, evaluates the `func` operand and the arguments, and starts execution of /// the referred to function. The operand types must match the argument types of the function. - /// The return place type must exactly match the return type. The type of the `func` operand - /// must be callable, meaning either a function pointer, a function type, or a closure type. + /// The return place type must match the return type. The type of the `func` operand must be + /// callable, meaning either a function pointer, a function type, or a closure type. /// - /// **Needs clarification**: The exact semantics of this, see [#71117]. + /// **Needs clarification**: The exact semantics of this. Current backends rely on `move` + /// operands not aliasing the return place. It is unclear how this is justified in MIR, see + /// [#71117]. /// /// [#71117]: https://github.com/rust-lang/rust/issues/71117 Call { From 6cb463cb112a05298ce2467085b64dfb4a3d63b3 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Wed, 6 Apr 2022 16:31:11 +0200 Subject: [PATCH 11/25] Add futex-based RwLock on Linux. --- .../std/src/sys/unix/locks/futex_rwlock.rs | 293 ++++++++++++++++++ library/std/src/sys/unix/locks/mod.rs | 4 +- 2 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 library/std/src/sys/unix/locks/futex_rwlock.rs diff --git a/library/std/src/sys/unix/locks/futex_rwlock.rs b/library/std/src/sys/unix/locks/futex_rwlock.rs new file mode 100644 index 0000000000000..0665d7b3bfdc5 --- /dev/null +++ b/library/std/src/sys/unix/locks/futex_rwlock.rs @@ -0,0 +1,293 @@ +use crate::sync::atomic::{ + AtomicI32, + Ordering::{Acquire, Relaxed, Release}, +}; +use crate::sys::futex::{futex_wait, futex_wake, futex_wake_all}; + +pub type MovableRwLock = RwLock; + +pub struct RwLock { + // The state consists of a 30-bit reader counter, a 'readers waiting' flag, and a 'writers waiting' flag. + // Bits 0..30: + // 0: Unlocked + // 1..=0x3FFF_FFFE: Locked by N readers + // 0x3FFF_FFFF: Write locked + // Bit 30: Readers are waiting on this futex. + // Bit 31: Writers are waiting on the writer_notify futex. + state: AtomicI32, + // The 'condition variable' to notify writers through. + // Incremented on every signal. + writer_notify: AtomicI32, +} + +const READ_LOCKED: i32 = 1; +const MASK: i32 = (1 << 30) - 1; +const WRITE_LOCKED: i32 = MASK; +const MAX_READERS: i32 = MASK - 1; +const READERS_WAITING: i32 = 1 << 30; +const WRITERS_WAITING: i32 = 1 << 31; + +fn unlocked(state: i32) -> bool { + state & MASK == 0 +} + +fn write_locked(state: i32) -> bool { + state & MASK == WRITE_LOCKED +} + +fn readers_waiting(state: i32) -> bool { + state & READERS_WAITING != 0 +} + +fn writers_waiting(state: i32) -> bool { + state & WRITERS_WAITING != 0 +} + +fn read_lockable(state: i32) -> bool { + // This also returns false if the counter could overflow if we tried to read lock it. + state & MASK < MAX_READERS && !readers_waiting(state) && !writers_waiting(state) +} + +fn reached_max_readers(state: i32) -> bool { + state & MASK == MAX_READERS +} + +impl RwLock { + #[inline] + pub const fn new() -> Self { + Self { state: AtomicI32::new(0), writer_notify: AtomicI32::new(0) } + } + + #[inline] + pub unsafe fn destroy(&self) {} + + #[inline] + pub unsafe fn try_read(&self) -> bool { + self.state + .fetch_update(Acquire, Relaxed, |s| read_lockable(s).then(|| s + READ_LOCKED)) + .is_ok() + } + + #[inline] + pub unsafe fn read(&self) { + if !self.try_read() { + self.read_contended(); + } + } + + #[inline] + pub unsafe fn read_unlock(&self) { + let state = self.state.fetch_sub(READ_LOCKED, Release) - 1; + + // It's impossible for a reader to be waiting on a read-locked RwLock, + // except if there is also a writer waiting. + debug_assert!(!readers_waiting(state) || writers_waiting(state)); + + // Wake up a writer if we were the last reader and there's a writer waiting. + if unlocked(state) && writers_waiting(state) { + self.wake_writer_or_readers(state); + } + } + + #[cold] + fn read_contended(&self) { + let mut state = self.spin_read(); + + loop { + // If we can lock it, lock it. + if read_lockable(state) { + match self.state.compare_exchange(state, state + READ_LOCKED, Acquire, Relaxed) { + Ok(_) => return, // Locked! + Err(s) => { + state = s; + continue; + } + } + } + + // Check for overflow. + if reached_max_readers(state) { + panic!("too many active read locks on RwLock"); + } + + // Make sure the readers waiting bit is set before we go to sleep. + if !readers_waiting(state) { + if let Err(s) = + self.state.compare_exchange(state, state | READERS_WAITING, Relaxed, Relaxed) + { + state = s; + continue; + } + } + + // Wait for the state to change. + futex_wait(&self.state, state | READERS_WAITING, None); + + // Spin again after waking up. + state = self.spin_read(); + } + } + + #[inline] + pub unsafe fn try_write(&self) -> bool { + self.state.fetch_update(Acquire, Relaxed, |s| unlocked(s).then(|| s + WRITE_LOCKED)).is_ok() + } + + #[inline] + pub unsafe fn write(&self) { + if !self.try_write() { + self.write_contended(); + } + } + + #[inline] + pub unsafe fn write_unlock(&self) { + let state = self.state.fetch_sub(WRITE_LOCKED, Release) - WRITE_LOCKED; + + debug_assert!(unlocked(state)); + + if writers_waiting(state) || readers_waiting(state) { + self.wake_writer_or_readers(state); + } + } + + #[cold] + fn write_contended(&self) { + let mut state = self.spin_write(); + + let mut other_writers_waiting = 0; + + loop { + // If it's unlocked, we try to lock it. + if unlocked(state) { + match self.state.compare_exchange( + state, + state | WRITE_LOCKED | other_writers_waiting, + Acquire, + Relaxed, + ) { + Ok(_) => return, // Locked! + Err(s) => { + state = s; + continue; + } + } + } + + // Set the waiting bit indicating that we're waiting on it. + if !writers_waiting(state) { + if let Err(s) = + self.state.compare_exchange(state, state | WRITERS_WAITING, Relaxed, Relaxed) + { + state = s; + continue; + } + } + + // Other writers might be waiting now too, so we should make sure + // we keep that bit on once we manage lock it. + other_writers_waiting = WRITERS_WAITING; + + // Examine the notification counter before we check if `state` has changed, + // to make sure we don't miss any notifications. + let seq = self.writer_notify.load(Acquire); + + // Don't go to sleep if the lock has become available, + // or if the writers waiting bit is no longer set. + let s = self.state.load(Relaxed); + if unlocked(state) || !writers_waiting(s) { + state = s; + continue; + } + + // Wait for the state to change. + futex_wait(&self.writer_notify, seq, None); + + // Spin again after waking up. + state = self.spin_write(); + } + } + + /// Wake up waiting threads after unlocking. + /// + /// If both are waiting, this will wake up only one writer, but will fall + /// back to waking up readers if there was no writer to wake up. + #[cold] + fn wake_writer_or_readers(&self, mut state: i32) { + assert!(unlocked(state)); + + // The readers waiting bit might be turned on at any point now, + // since readers will block when there's anything waiting. + // Writers will just lock the lock though, regardless of the waiting bits, + // so we don't have to worry about the writer waiting bit. + // + // If the lock gets locked in the meantime, we don't have to do + // anything, because then the thread that locked the lock will take + // care of waking up waiters when it unlocks. + + // If only writers are waiting, wake one of them up. + if state == WRITERS_WAITING { + match self.state.compare_exchange(state, 0, Relaxed, Relaxed) { + Ok(_) => { + self.wake_writer(); + return; + } + Err(s) => { + // Maybe some readers are now waiting too. So, continue to the next `if`. + state = s; + } + } + } + + // If both writers and readers are waiting, leave the readers waiting + // and only wake up one writer. + if state == READERS_WAITING + WRITERS_WAITING { + if self.state.compare_exchange(state, READERS_WAITING, Relaxed, Relaxed).is_err() { + // The lock got locked. Not our problem anymore. + return; + } + if self.wake_writer() { + return; + } + // No writers were actually waiting. Continue to wake up readers instead. + state = READERS_WAITING; + } + + // If readers are waiting, wake them all up. + if state == READERS_WAITING { + if self.state.compare_exchange(state, 0, Relaxed, Relaxed).is_ok() { + futex_wake_all(&self.state); + } + } + } + + fn wake_writer(&self) -> bool { + self.writer_notify.fetch_add(1, Release); + futex_wake(&self.writer_notify) + } + + /// Spin for a while, but stop directly at the given condition. + fn spin_until(&self, f: impl Fn(i32) -> bool) -> i32 { + let mut spin = 100; // Chosen by fair dice roll. + loop { + let state = self.state.load(Relaxed); + if f(state) || spin == 0 { + return state; + } + crate::hint::spin_loop(); + spin -= 1; + } + } + + fn spin_write(&self) -> i32 { + // Stop spinning when it's unlocked or when there's waiting writers, to keep things somewhat fair. + self.spin_until(|state| unlocked(state) || writers_waiting(state)) + } + + fn spin_read(&self) -> i32 { + // Stop spinning when it's unlocked or read locked, or when there's waiting threads. + self.spin_until(|state| { + !write_locked(state) || readers_waiting(state) || writers_waiting(state) + }) + } +} diff --git a/library/std/src/sys/unix/locks/mod.rs b/library/std/src/sys/unix/locks/mod.rs index 2b8dd168068b5..85afc939d2e89 100644 --- a/library/std/src/sys/unix/locks/mod.rs +++ b/library/std/src/sys/unix/locks/mod.rs @@ -4,13 +4,13 @@ cfg_if::cfg_if! { target_os = "android", ))] { mod futex; + mod futex_rwlock; #[allow(dead_code)] mod pthread_mutex; // Only used for PthreadMutexAttr, needed by pthread_remutex. mod pthread_remutex; // FIXME: Implement this using a futex - mod pthread_rwlock; // FIXME: Implement this using a futex pub use futex::{Mutex, MovableMutex, Condvar, MovableCondvar}; pub use pthread_remutex::ReentrantMutex; - pub use pthread_rwlock::{RwLock, MovableRwLock}; + pub use futex_rwlock::{RwLock, MovableRwLock}; } else { mod pthread_mutex; mod pthread_remutex; From 307aa588f42b65cbce27f0e366197e27ef531c77 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 8 Apr 2022 16:07:07 +0200 Subject: [PATCH 12/25] Fix typo in futex rwlock. Co-authored-by: Amanieu d'Antras --- library/std/src/sys/unix/locks/futex_rwlock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/locks/futex_rwlock.rs b/library/std/src/sys/unix/locks/futex_rwlock.rs index 0665d7b3bfdc5..19ec6fa834908 100644 --- a/library/std/src/sys/unix/locks/futex_rwlock.rs +++ b/library/std/src/sys/unix/locks/futex_rwlock.rs @@ -77,7 +77,7 @@ impl RwLock { #[inline] pub unsafe fn read_unlock(&self) { - let state = self.state.fetch_sub(READ_LOCKED, Release) - 1; + let state = self.state.fetch_sub(READ_LOCKED, Release) - READ_LOCKED; // It's impossible for a reader to be waiting on a read-locked RwLock, // except if there is also a writer waiting. From a5d2c046b4862afb422d768e90264a743cea68b8 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Fri, 8 Apr 2022 15:53:08 -0400 Subject: [PATCH 13/25] Add more clarifications in response to Ralf's comments --- compiler/rustc_middle/src/mir/mod.rs | 186 ++++++++++++--------------- 1 file changed, 84 insertions(+), 102 deletions(-) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 7701fd0a71a3b..5d7d30a694526 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1598,9 +1598,9 @@ pub enum StatementKind<'tcx> { /// Assign statements roughly correspond to an assignment in Rust proper (`x = ...`) except /// without the possibility of dropping the previous value (that must be done separately, if at /// all). The *exact* way this works is undecided. It probably does something like evaluating - /// the LHS and RHS, and then doing the inverse of a place to value conversion to write the - /// resulting value into memory. Various parts of this may do type specific things that are more - /// complicated than simply copying over the bytes depending on the types. + /// the LHS to a place and the RHS to a value, and then storing the value to the place. Various + /// parts of this may do type specific things that are more complicated than simply copying + /// bytes. /// /// **Needs clarification**: The implication of the above idea would be that assignment implies /// that the resulting value is initialized. I believe we could commit to this separately from @@ -1615,8 +1615,9 @@ pub enum StatementKind<'tcx> { /// interesting for optimizations? Do we want to allow such optimizations? /// /// **Needs clarification**: We currently require that the LHS place not overlap with any place - /// read as part of computation of the RHS. This requirement is under discussion in [#68364]. As - /// a part of this discussion, it is also unclear in what order the components are evaluated. + /// read as part of computation of the RHS for some rvalues (generally those not producing + /// primitives). This requirement is under discussion in [#68364]. As a part of this discussion, + /// it is also unclear in what order the components are evaluated. /// /// [#68364]: https://github.com/rust-lang/rust/issues/68364 /// @@ -1705,9 +1706,8 @@ pub enum StatementKind<'tcx> { /// **Needs clarification**: In what order are operands computed and dereferenced? It should /// probably match the order for assignment, but that is also undecided. /// - /// **Needs clarification**: Is this typed or not, ie is there a place to value and back - /// conversion involved? I vaguely remember Ralf saying somewhere that he thought it should not - /// be. + /// **Needs clarification**: Is this typed or not, ie is there a typed load and store involved? + /// I vaguely remember Ralf saying somewhere that he thought it should not be. CopyNonOverlapping(Box>), /// No-op. Useful for deleting instructions without affecting statement indices. @@ -1858,41 +1858,55 @@ pub struct CopyNonOverlapping<'tcx> { /// Places roughly correspond to a "location in memory." Places in MIR are the same mathematical /// object as places in Rust. This of course means that what exactly they are is undecided and part -/// of the Rust memory model. However, they will likely contain at least the following three pieces -/// of information in some form: +/// of the Rust memory model. However, they will likely contain at least the following pieces of +/// information in some form: /// -/// 1. The part of memory that is referred to (see discussion below for details). -/// 2. The type of the place and an optional variant index. See [`PlaceTy`][tcx::PlaceTy] -/// 3. The provenance with which the place is being accessed. +/// 1. The address in memory that the place refers to. +/// 2. The provenance with which the place is being accessed. +/// 3. The type of the place and an optional variant index. See [`PlaceTy`][tcx::PlaceTy]. +/// 4. Optionally, some metadata. This exists if and only if the type of the place is not `Sized`. /// -/// We'll give a description below of how the first two of these three properties are computed for a -/// place. We cannot give a description of the provenance, because that is part of the undecided -/// aliasing model - we only include it here at all to acknowledge its existence. +/// We'll give a description below of how all pieces of the place except for the provenance are +/// calculated. We cannot give a description of the provenance, because that is part of the +/// undecided aliasing model - we only include it here at all to acknowledge its existence. /// -/// For a place that has no projections, ie `Place { local, projection: [] }`, the part of memory is -/// the local's full allocation and the type is the type of the local. For any other place, we -/// define the values as a function of the parent place, that is the place with its last -/// [`ProjectionElem`] stripped. The way this is computed of course depends on the kind of that last -/// projection element: +/// Each local naturally corresponds to the place `Place { local, projection: [] }`. This place has +/// the address of the local's allocation and the type of the local. +/// +/// **Needs clarification:** Unsized locals seem to present a bit of an issue. Their allocation +/// can't actually be created on `StorageLive`, because it's unclear how big to make the allocation. +/// Furthermore, MIR produces assignments to unsized locals, although that is not permitted under +/// `#![feature(unsized_locals)]` in Rust. Besides just putting "unsized locals are special and +/// different" in a bunch of places, I (JakobDegen) don't know how to incorporate this behavior into +/// the current MIR semantics in a clean way - possibly this needs some design work first. +/// +/// For places that are not locals, ie they have a non-empty list of projections, we define the +/// values as a function of the parent place, that is the place with its last [`ProjectionElem`] +/// stripped. The way this is computed of course depends on the kind of that last projection +/// element: /// /// - [`Downcast`](ProjectionElem::Downcast): This projection sets the place's variant index to the /// given one, and makes no other changes. A `Downcast` projection on a place with its variant /// index already set is not well-formed. /// - [`Field`](ProjectionElem::Field): `Field` projections take their parent place and create a -/// place referring to one of the fields of the type. The referred to place in memory is where -/// the layout places the field. The type becomes the type of the field. +/// place referring to one of the fields of the type. The resulting address is the paren't +/// address, plus the offset of the field. The type becomes the type of the field. If the parent +/// was unsized and so had metadata associated with it, then the metadata is retained if the +/// field is unsized and thrown out if it is sized. /// /// These projections are only legal for tuples, ADTs, closures, and generators. If the ADT or /// generator has more than one variant, the parent place's variant index must be set, indicating /// which variant is being used. If it has just one variant, the variant index may or may not be /// included - the single possible variant is inferred if it is not included. /// - [`ConstantIndex`](ProjectionElem::ConstantIndex): Computes an offset in units of `T` into the -/// place as described in the documentation for the `ProjectionElem`. The resulting part of -/// memory is the location of that element of the array/slice, and the type is `T`. This is only -/// legal if the parent place has type `[T; N]` or `[T]` (*not* `&[T]`). -/// - [`Subslice`](ProjectionElem::Subslice): Much like `ConstantIndex`. It is also only legal on -/// `[T; N]` and `[T]`. However, this yields a `Place` of type `[T]`, and may refer to more than -/// one element in the parent place. +/// place as described in the documentation for the `ProjectionElem`. The resulting address is +/// the parent's address plus that offset, and the type is `T`. This is only legal if the parent +/// place has type `[T; N]` or `[T]` (*not* `&[T]`). Since such a `T` is always sized, any +/// resulting metadata is thrown out. +/// - [`Subslice`](ProjectionElem::Subslice): This projection calculates an offset and a new +/// address in a similar manner as `ConstantIndex`. It is also only legal on `[T; N]` and `[T]`. +/// However, this yields a `Place` of type `[T]`, and additionally sets the metadata to be the +/// length of the subslice. /// - [`Index`](ProjectionElem::Index): Like `ConstantIndex`, only legal on `[T; N]` or `[T]`. /// However, `Index` additionally takes a local from which the value of the index is computed at /// runtime. Computing the value of the index involves interpreting the `Local` as a @@ -1901,53 +1915,23 @@ pub struct CopyNonOverlapping<'tcx> { /// have type `usize`. /// - [`Deref`](ProjectionElem::Deref): Derefs are the last type of projection, and the most /// complicated. They are only legal on parent places that are references, pointers, or `Box`. A -/// `Deref` projection begins by creating a value from the parent place, as if by +/// `Deref` projection begins by loading a value from the parent place, as if by /// [`Operand::Copy`]. It then dereferences the resulting pointer, creating a place of the -/// pointed to type. -/// -/// **Needs clarification**: What about metadata resulting from dereferencing wide pointers (and -/// possibly from accessing unsized locals - not sure how those work)? That probably deserves to go -/// on the list above and be discussed too. It is also probably necessary for making the indexing -/// stuff less hand-wavey. -/// -/// **Needs clarification**: When it says "part of memory" what does that mean precisely, and how -/// does it interact with the metadata? +/// pointee's type. The resulting address is the address that was stored in the pointer. If the +/// pointee type is unsized, the pointer additionally stored the value of the metadata. /// -/// One possible model that I believe makes sense is that "part of memory" is actually just the -/// address of the beginning of the referred to range of bytes. For sized types, the size of the -/// range is then stored in the type, and for unsized types it's stored (possibly indirectly, -/// through a vtable) in the metadata. +/// Computing a place may cause UB. One possibility is that the pointer used for a `Deref` may not +/// be suitably aligned. Another possibility is that the place is not in bouns, meaning it does not +/// point to an actual allocation. /// -/// Alternatively, the "part of memory" could be a whole range of bytes. Initially seemed more -/// natural to me, but seems like it falls apart after a little bit. -/// -/// More likely though, we should call this detail a part of the Rust memory model and let that deal -/// with the precise definition of this part of a place. If we feel strongly, I don't think we *have -/// to* though. MIR places are more flexible than Rust places, and we might be able to make a -/// decision on the flexible parts without semi-stabilizing the source language. (end NC) -/// -/// Computing a place may be UB - this is certainly the case with dereferencing, which requires -/// sufficient provenance, but it may additionally be the case for some of the other field -/// projections. -/// -/// It is undecided when this UB kicks in. As best I can tell that is the question being discussed -/// in [UCG#319]. Summarizing from that thread, I believe the options are: +/// However, if this is actually UB and when the UB kicks in is undecided. This is being discussed +/// in [UCG#319]. The options include that every place must obey those rules, that only some places +/// must obey them, or that places impose no rules of their own. /// /// [UCG#319]: https://github.com/rust-lang/unsafe-code-guidelines/issues/319 /// -/// 1. Each intermediate place must have provenance for the whole part of memory it refers to. This -/// is the status quo. -/// 2. Only for intermediate place where the last projection was *not* a deref. This corresponds to -/// "Check inbounds on place projection". -/// 3. Only on place to value conversions, assignments, and referencing operation. This corresponds -/// to "remove the restrictions from `*` entirely." -/// 4. On each intermediate place if the place is used for a place to value conversion as part of -/// an assignment assignment or it is used for a referencing operation. For a raw pointer -/// computation, never. This corresponds to "magic?". -/// -/// Hopefully I am not misrepresenting anyone's opinions - please let me know if I am. Currently, -/// Rust chooses option 1. This is checked by MIRI and taken advantage of by codegen (via `gep -/// inbounds`). That is possibly subject to change. +/// Rust currently requires that every place obey those two rules. This is checked by MIRI and taken +/// advantage of by codegen (via `gep inbounds`). That is possibly subject to change. #[derive(Copy, Clone, PartialEq, Eq, Hash, TyEncodable, HashStable)] pub struct Place<'tcx> { pub local: Local, @@ -2321,32 +2305,30 @@ pub struct SourceScopeLocalData { /// /// [value-def]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/value-domain.md /// -/// The most common way to create values is via a place to value conversion. A place to value -/// conversion is an operation which reads the memory of the place and converts it to a value. This -/// is a fundamentally *typed* operation. The nature of the value produced depends on the type of -/// the conversion. Furthermore, there may be other effects: if the type has a validity constraint -/// the place to value conversion might be UB if the validity constraint is not met. +/// The most common way to create values is via loading a place. Loading a place is an operation +/// which reads the memory of the place and converts it to a value. This is a fundamentally *typed* +/// operation. The nature of the value produced depends on the type of the conversion. Furthermore, +/// there may be other effects: if the type has a validity constraint loading the place might be UB +/// if the validity constraint is not met. /// -/// **Needs clarification:** Ralf proposes that place to value conversions not have side-effects. +/// **Needs clarification:** Ralf proposes that loading a place not have side-effects. /// This is what is implemented in miri today. Are these the semantics we want for MIR? Is this /// something we can even decide without knowing more about Rust's memory model? /// -/// A place to value conversion on a place that has its variant index set is not well-formed. -/// However, note that this rule only applies to places appearing in MIR bodies. Many functions, -/// such as [`Place::ty`], still accept such a place. If you write a function for which it might be -/// ambiguous whether such a thing is accepted, make sure to document your choice clearly. +/// Loading a place that has its variant index set is not well-formed. However, note that this rule +/// only applies to places appearing in MIR bodies. Many functions, such as [`Place::ty`], still +/// accept such a place. If you write a function for which it might be ambiguous whether such a +/// thing is accepted, make sure to document your choice clearly. #[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)] pub enum Operand<'tcx> { - /// Creates a value by performing a place to value conversion at the given place. The type of - /// the place must be `Copy` + /// Creates a value by loading the given place. The type of the place must be `Copy` Copy(Place<'tcx>), - /// Creates a value by performing a place to value conversion for the place, just like the - /// `Copy` operand. + /// Creates a value by performing loading the place, just like the `Copy` operand. /// /// This *may* additionally overwrite the place with `uninit` bytes, depending on how we decide - /// in [UCG#188]. You should not emit MIR that may attempt a subsequent second place to value - /// conversion on this place without first re-initializing it. + /// in [UCG#188]. You should not emit MIR that may attempt a subsequent second load of this + /// place without first re-initializing it. /// /// [UCG#188]: https://github.com/rust-lang/unsafe-code-guidelines/issues/188 Move(Place<'tcx>), @@ -2463,7 +2445,7 @@ impl<'tcx> Operand<'tcx> { /// /// Computing any rvalue begins by evaluating the places and operands in some order (**Needs /// clarification**: Which order?). These are then used to produce a "value" - the same kind of -/// value that an [`Operand`] is. +/// value that an [`Operand`] produces. pub enum Rvalue<'tcx> { /// Yields the operand unchanged Use(Operand<'tcx>), @@ -2487,14 +2469,14 @@ pub enum Rvalue<'tcx> { /// `Shallow` borrows are disallowed after drop lowering. Ref(Region<'tcx>, BorrowKind, Place<'tcx>), - /// Returns a pointer/reference to the given thread local. + /// Creates a pointer/reference to the given thread local. /// /// The yielded type is a `*mut T` if the static is mutable, otherwise if the static is extern a /// `*const T`, and if neither of those apply a `&T`. /// /// **Note:** This is a runtime operation that actually executes code and is in this sense more - /// like a function call. Also, DSEing these causes `fn main() {}` to SIGILL for some reason - /// that I never got a chance to look into. + /// like a function call. Also, eliminating dead stores of this rvalue causes `fn main() {}` to + /// SIGILL for some reason that I (JakobDegen) never got a chance to look into. /// /// **Needs clarification**: Are there weird additional semantics here related to the runtime /// nature of this operation? @@ -2511,9 +2493,9 @@ pub enum Rvalue<'tcx> { /// Yields the length of the place, as a `usize`. /// - /// If the type of the place is an array, this is the array length. This also works for slices - /// (`[T]`, not `&[T]`) through some mechanism that depends on how exactly places work (see - /// there for more details). + /// If the type of the place is an array, this is the array length. For slices (`[T]`, not + /// `&[T]`) this accesses the place's metadata to determine the length. This rvalue is + /// ill-formed for places of other types. Len(Place<'tcx>), /// Performs essentially all of the casts that can be performed via `as`. @@ -2527,21 +2509,21 @@ pub enum Rvalue<'tcx> { /// * `Offset` has the same semantics as [`offset`](pointer::offset), except that the second /// parameter may be a `usize` as well. /// * The comparison operations accept `bool`s, `char`s, signed or unsigned integers, floats, - /// raw pointers, or function pointers and return a `bool`. + /// raw pointers, or function pointers of matching types and return a `bool`. /// * Left and right shift operations accept signed or unsigned integers not necessarily of the - /// same type and return a value of the same type as their LHS. For all other operations, the - /// types of the operands must match. Like in Rust, the RHS is truncated as needed. - /// * The `Bit*` operations accept signed integers, unsigned integers, or bools and return a - /// value of that type. - /// * The remaining operations accept signed integers, unsigned integers, or floats of any - /// matching type and return a value of that type. + /// same type and return a value of the same type as their LHS. Like in Rust, the RHS is + /// truncated as needed. + /// * The `Bit*` operations accept signed integers, unsigned integers, or bools with matching + /// types and return a value of that type. + /// * The remaining operations accept signed integers, unsigned integers, or floats with + /// matching types and return a value of that type. BinaryOp(BinOp, Box<(Operand<'tcx>, Operand<'tcx>)>), /// Same as `BinaryOp`, but yields `(T, bool)` instead of `T`. In addition to performing the /// same computation as the matching `BinaryOp`, checks if the infinite precison result would be /// unequal to the actual result and sets the `bool` if this is the case. /// - /// This only supports addition, subtraction, multiplication, and shift operations. + /// This only supports addition, subtraction, multiplication, and shift operations on integers. CheckedBinaryOp(BinOp, Box<(Operand<'tcx>, Operand<'tcx>)>), /// Computes a value as described by the operation. @@ -2582,7 +2564,7 @@ pub enum Rvalue<'tcx> { /// Transmutes a `*mut u8` into shallow-initialized `Box`. /// - /// This is different a normal transmute because dataflow analysis will treat the box as + /// This is different from a normal transmute because dataflow analysis will treat the box as /// initialized but its content as uninitialized. Like other pointer casts, this in general /// affects alias analysis. /// From 9745a172a9954734367c166f07bcb5a83a07878f Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Sat, 9 Apr 2022 10:00:19 -0400 Subject: [PATCH 14/25] Remove rule that place loads may not happen with variant index set --- compiler/rustc_const_eval/src/transform/validate.rs | 7 ++----- compiler/rustc_middle/src/mir/mod.rs | 9 ++++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index a01e4512e9a1e..bcf67713e0eb5 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -246,12 +246,9 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { self.super_projection_elem(local, proj_base, elem, context, location); } - fn visit_place(&mut self, place: &Place<'tcx>, _: PlaceContext, location: Location) { + fn visit_place(&mut self, place: &Place<'tcx>, _: PlaceContext, _: Location) { // Set off any `bug!`s in the type computation code - let ty = place.ty(&self.body.local_decls, self.tcx); - if ty.variant_index.is_some() { - self.fail(location, "Top level places may not have their variant index set!"); - } + let _ = place.ty(&self.body.local_decls, self.tcx); } fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 5d7d30a694526..5ce811bf1d10c 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1921,7 +1921,7 @@ pub struct CopyNonOverlapping<'tcx> { /// pointee type is unsized, the pointer additionally stored the value of the metadata. /// /// Computing a place may cause UB. One possibility is that the pointer used for a `Deref` may not -/// be suitably aligned. Another possibility is that the place is not in bouns, meaning it does not +/// be suitably aligned. Another possibility is that the place is not in bounds, meaning it does not /// point to an actual allocation. /// /// However, if this is actually UB and when the UB kicks in is undecided. This is being discussed @@ -2315,10 +2315,9 @@ pub struct SourceScopeLocalData { /// This is what is implemented in miri today. Are these the semantics we want for MIR? Is this /// something we can even decide without knowing more about Rust's memory model? /// -/// Loading a place that has its variant index set is not well-formed. However, note that this rule -/// only applies to places appearing in MIR bodies. Many functions, such as [`Place::ty`], still -/// accept such a place. If you write a function for which it might be ambiguous whether such a -/// thing is accepted, make sure to document your choice clearly. +/// **Needs clarifiation:** Is loading a place that has its variant index set well-formed? Miri +/// currently implements it, but it seems like this may be something to check against in the +/// validator. #[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)] pub enum Operand<'tcx> { /// Creates a value by loading the given place. The type of the place must be `Copy` From bf3ef0da0c630ab239e4f26e86602eabe585e74f Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Sat, 9 Apr 2022 15:25:46 -0400 Subject: [PATCH 15/25] Switch to the 'normal' basic block for writing asm outputs if needed. We may sometimes emit an `invoke` instead of a `call` for inline assembly during the MIR -> LLVM IR lowering. But we failed to update the IR builder's current basic block before writing the results to the outputs. This would result in invalid IR because the basic block would end in a `store` instruction, which isn't a valid terminator. --- compiler/rustc_codegen_llvm/src/asm.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compiler/rustc_codegen_llvm/src/asm.rs b/compiler/rustc_codegen_llvm/src/asm.rs index 03c390b4bd427..91d132eb34350 100644 --- a/compiler/rustc_codegen_llvm/src/asm.rs +++ b/compiler/rustc_codegen_llvm/src/asm.rs @@ -290,6 +290,11 @@ impl<'ll, 'tcx> AsmBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> { } attributes::apply_to_callsite(result, llvm::AttributePlace::Function, &{ attrs }); + // Switch to the 'normal' basic block if we did an `invoke` instead of a `call` + if let Some((dest, _, _)) = dest_catch_funclet { + self.switch_to_block(dest); + } + // Write results to outputs for (idx, op) in operands.iter().enumerate() { if let InlineAsmOperandRef::Out { reg, place: Some(place), .. } From 0b2f3604fd733db5ad9498eaf129655879c242b3 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Sat, 9 Apr 2022 15:16:38 -0700 Subject: [PATCH 16/25] Update asm-may_unwind test to handle use of asm with outputs. --- src/test/codegen/asm-may_unwind.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/test/codegen/asm-may_unwind.rs b/src/test/codegen/asm-may_unwind.rs index 3b34d79c3a946..bf4202764a7ec 100644 --- a/src/test/codegen/asm-may_unwind.rs +++ b/src/test/codegen/asm-may_unwind.rs @@ -18,10 +18,23 @@ impl Drop for Foo { } } -// CHECK-LABEL: @may_unwind +// CHECK-LABEL: @asm_may_unwind #[no_mangle] -pub unsafe fn may_unwind() { +pub unsafe fn asm_may_unwind() { let _m = Foo; // CHECK: invoke void asm sideeffect alignstack inteldialect unwind "" asm!("", options(may_unwind)); } + +// CHECK-LABEL: @asm_with_result_may_unwind +#[no_mangle] +pub unsafe fn asm_with_result_may_unwind() -> u64 { + let _m = Foo; + let res: u64; + // CHECK: [[RES:%[0-9]+]] = invoke i64 asm sideeffect alignstack inteldialect unwind + // CHECK-NEXT: to label %[[NORMALBB:[a-b0-9]+]] + asm!("mov {}, 1", out(reg) res, options(may_unwind)); + // CHECK: [[NORMALBB]]: + // CHECK: ret i64 [[RES:%[0-9]+]] + res +} From bb3a071df8d75bad5e6fecb54ec61477ded777c3 Mon Sep 17 00:00:00 2001 From: nyanpasu64 Date: Sun, 10 Apr 2022 12:41:31 -0700 Subject: [PATCH 17/25] Fix formatting error in pin.rs docs --- library/core/src/pin.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/pin.rs b/library/core/src/pin.rs index cef6a68b4d329..720317b05e080 100644 --- a/library/core/src/pin.rs +++ b/library/core/src/pin.rs @@ -175,7 +175,7 @@ //! relies on pinning requires unsafe code, but be aware that deciding to make //! use of pinning in your type (for example by implementing some operation on //! [Pin]<[&]Self> or [Pin]<[&mut] Self>) has consequences for your -//! [`Drop`][Drop]implementation as well: if an element of your type could have been pinned, +//! [`Drop`][Drop] implementation as well: if an element of your type could have been pinned, //! you must treat [`Drop`][Drop] as implicitly taking [Pin]<[&mut] Self>. //! //! For example, you could implement [`Drop`][Drop] as follows: From b92cd1a32c842e82575e59374545dda5f9b9f77a Mon Sep 17 00:00:00 2001 From: Christopher Durham Date: Sun, 10 Apr 2022 15:04:57 -0500 Subject: [PATCH 18/25] Clarify str::from_utf8_unchecked's invariants Specifically, make it clear that it is immediately UB to pass ill-formed UTF-8 into the function. The previous wording left space to interpret that the UB only occurred when calling another function, which "assumes that `&str`s are valid UTF-8." This does not change whether str being UTF-8 is a safety or a validity invariant. (As per previous discussion, it is a safety invariant, not a validity invariant.) It just makes it clear that valid UTF-8 is a precondition of str::from_utf8_unchecked, and that emitting an Abstract Machine fault (e.g. UB or a sanitizer error) on invalid UTF-8 is a valid thing to do. If user code wants to create an unsafe `&str` pointing to ill-formed UTF-8, it must be done via transmutes. Also, just, don't. --- library/core/src/str/converts.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/library/core/src/str/converts.rs b/library/core/src/str/converts.rs index ef26cbfb640bf..81b1db4ac6fed 100644 --- a/library/core/src/str/converts.rs +++ b/library/core/src/str/converts.rs @@ -144,11 +144,7 @@ pub const fn from_utf8_mut(v: &mut [u8]) -> Result<&mut str, Utf8Error> { /// /// # Safety /// -/// This function is unsafe because it does not check that the bytes passed to -/// it are valid UTF-8. If this constraint is violated, undefined behavior -/// results, as the rest of Rust assumes that [`&str`]s are valid UTF-8. -/// -/// [`&str`]: str +/// The bytes passed in must be valid UTF-8. /// /// # Examples /// From 986c1687f868e7839f4e967bba32aeec6b2df5da Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 10 Apr 2022 16:37:24 -0500 Subject: [PATCH 19/25] Remove duplicate aliases for `codegen_{cranelift,gcc}` Bootstrap already allows selecting these in `PathSet::has`, which allows any string that matches the end of a full path. I found these by adding `assert!(path.exists())` in `StepDescription::paths`. I think ideally we wouldn't have any aliases that aren't paths, but I've held off on enforcing that here since it may be controversial, I'll open a separate PR. --- src/bootstrap/check.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 28e7f1fdca7a1..432a6c34ed584 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -243,12 +243,7 @@ impl Step for CodegenBackend { const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.paths(&[ - "compiler/rustc_codegen_cranelift", - "rustc_codegen_cranelift", - "compiler/rustc_codegen_gcc", - "rustc_codegen_gcc", - ]) + run.paths(&["compiler/rustc_codegen_cranelift", "compiler/rustc_codegen_gcc"]) } fn make_run(run: RunConfig<'_>) { From 4c1438332be229ee1d1e1dbc02181a03678e04f9 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 10 Apr 2022 16:41:21 -0500 Subject: [PATCH 20/25] Add `build compiler/rustc_codegen_gcc` as an alias for `CodegenBackend` These paths (`_cranelift` and `_gcc`) are somewhat misleading, since they actually tell bootstrap to build *all* codegen backends. But this seems like a useful improvement in the meantime. --- src/bootstrap/compile.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index afc333b5048ce..45991381dc0fe 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -795,7 +795,7 @@ impl Step for CodegenBackend { const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.path("compiler/rustc_codegen_cranelift") + run.paths(&["compiler/rustc_codegen_cranelift", "compiler/rustc_codegen_gcc"]) } fn make_run(run: RunConfig<'_>) { From aeb3df76f66c4496b0fe4ed64ea86896c6bc8640 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 11 Apr 2022 10:05:05 +0200 Subject: [PATCH 21/25] CI: do not compile libcore twice when performing LLVM PGO --- src/ci/pgo.sh | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/ci/pgo.sh b/src/ci/pgo.sh index 689e6a11d6110..691d1282cf499 100755 --- a/src/ci/pgo.sh +++ b/src/ci/pgo.sh @@ -47,12 +47,6 @@ python3 ../x.py build --target=$PGO_HOST --host=$PGO_HOST \ --stage 2 library/std \ --llvm-profile-generate -# Profile libcore compilation in opt-level=0 and opt-level=3 -RUSTC_BOOTSTRAP=1 ./build/$PGO_HOST/stage2/bin/rustc \ - --edition=2021 --crate-type=lib ../library/core/src/lib.rs -RUSTC_BOOTSTRAP=1 ./build/$PGO_HOST/stage2/bin/rustc \ - --edition=2021 --crate-type=lib -Copt-level=3 ../library/core/src/lib.rs - # Compile rustc perf cp -r /tmp/rustc-perf ./ chown -R $(whoami): ./rustc-perf From 7c287915654d0b3f1cc4b060b41db32a83b000c5 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 11 Apr 2022 14:26:52 +0200 Subject: [PATCH 22/25] Add doc comments to futex operations. --- library/std/src/sys/unix/futex.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/library/std/src/sys/unix/futex.rs b/library/std/src/sys/unix/futex.rs index 4231241a14224..b45d1c0149cb4 100644 --- a/library/std/src/sys/unix/futex.rs +++ b/library/std/src/sys/unix/futex.rs @@ -7,6 +7,11 @@ use crate::sync::atomic::AtomicI32; use crate::time::Duration; +/// Wait for a futex_wake operation to wake us. +/// +/// Returns directly if the futex doesn't hold the expected value. +/// +/// Returns false on timeout, and true in all other cases. #[cfg(any(target_os = "linux", target_os = "android"))] pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option) -> bool { use super::time::Timespec; @@ -68,6 +73,10 @@ pub fn futex_wait(futex: &AtomicI32, expected: i32, timeout: Option) { } } +/// Wake up one thread that's blocked on futex_wait on this futex. +/// +/// Returns true if this actually woke up such a thread, +/// or false if no thread was waiting on this futex. #[cfg(any(target_os = "linux", target_os = "android"))] pub fn futex_wake(futex: &AtomicI32) -> bool { unsafe { @@ -80,6 +89,7 @@ pub fn futex_wake(futex: &AtomicI32) -> bool { } } +/// Wake up all threads that are waiting on futex_wait on this futex. #[cfg(any(target_os = "linux", target_os = "android"))] pub fn futex_wake_all(futex: &AtomicI32) { unsafe { From 1f2c2bb24f88e9fd008ce130017cc1628626d296 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 11 Apr 2022 14:27:06 +0200 Subject: [PATCH 23/25] Add comments to futex rwlock implementation. --- library/std/src/sys/unix/locks/futex_rwlock.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/locks/futex_rwlock.rs b/library/std/src/sys/unix/locks/futex_rwlock.rs index 19ec6fa834908..f602479bb52bb 100644 --- a/library/std/src/sys/unix/locks/futex_rwlock.rs +++ b/library/std/src/sys/unix/locks/futex_rwlock.rs @@ -45,6 +45,11 @@ fn writers_waiting(state: i32) -> bool { fn read_lockable(state: i32) -> bool { // This also returns false if the counter could overflow if we tried to read lock it. + // + // We don't allow read-locking if there's readers waiting, even if the lock is unlocked + // and there's no writers waiting. The only situation when this happens is after unlocking, + // at which point the unlocking thread might be waking up writers, which have priority over readers. + // The unlocking thread will clear the readers waiting bit and wake up readers, if necssary. state & MASK < MAX_READERS && !readers_waiting(state) && !writers_waiting(state) } @@ -249,7 +254,8 @@ impl RwLock { if self.wake_writer() { return; } - // No writers were actually waiting. Continue to wake up readers instead. + // No writers were actually blocked on futex_wait, so we continue + // to wake up readers instead, since we can't be sure if we notified a writer. state = READERS_WAITING; } @@ -261,6 +267,11 @@ impl RwLock { } } + /// This wakes one writer and returns true if we woke up a writer that was + /// blocked on futex_wait. + /// + /// If this returns false, it might still be the case that we notified a + /// writer that was about to go to sleep. fn wake_writer(&self) -> bool { self.writer_notify.fetch_add(1, Release); futex_wake(&self.writer_notify) From c4a4f48c527453ddce2a2f1496473319290c6d8e Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 11 Apr 2022 14:29:32 +0200 Subject: [PATCH 24/25] Use compare_exchange_weak in futex rwlock implementation. --- library/std/src/sys/unix/locks/futex_rwlock.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/unix/locks/futex_rwlock.rs b/library/std/src/sys/unix/locks/futex_rwlock.rs index f602479bb52bb..2862cdea7dad7 100644 --- a/library/std/src/sys/unix/locks/futex_rwlock.rs +++ b/library/std/src/sys/unix/locks/futex_rwlock.rs @@ -75,7 +75,13 @@ impl RwLock { #[inline] pub unsafe fn read(&self) { - if !self.try_read() { + let state = self.state.load(Relaxed); + if !read_lockable(state) + || self + .state + .compare_exchange_weak(state, state + READ_LOCKED, Acquire, Relaxed) + .is_err() + { self.read_contended(); } } @@ -101,7 +107,8 @@ impl RwLock { loop { // If we can lock it, lock it. if read_lockable(state) { - match self.state.compare_exchange(state, state + READ_LOCKED, Acquire, Relaxed) { + match self.state.compare_exchange_weak(state, state + READ_LOCKED, Acquire, Relaxed) + { Ok(_) => return, // Locked! Err(s) => { state = s; @@ -140,7 +147,7 @@ impl RwLock { #[inline] pub unsafe fn write(&self) { - if !self.try_write() { + if self.state.compare_exchange_weak(0, WRITE_LOCKED, Acquire, Relaxed).is_err() { self.write_contended(); } } @@ -165,7 +172,7 @@ impl RwLock { loop { // If it's unlocked, we try to lock it. if unlocked(state) { - match self.state.compare_exchange( + match self.state.compare_exchange_weak( state, state | WRITE_LOCKED | other_writers_waiting, Acquire, From 83393817419e197d696fb724560bc55b18346a01 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 11 Apr 2022 14:52:02 +0200 Subject: [PATCH 25/25] Use is_ or has_ prefix for pure `-> bool` functions. --- .../std/src/sys/unix/locks/futex_rwlock.rs | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/library/std/src/sys/unix/locks/futex_rwlock.rs b/library/std/src/sys/unix/locks/futex_rwlock.rs index 2862cdea7dad7..aa16da97e4c19 100644 --- a/library/std/src/sys/unix/locks/futex_rwlock.rs +++ b/library/std/src/sys/unix/locks/futex_rwlock.rs @@ -27,33 +27,33 @@ const MAX_READERS: i32 = MASK - 1; const READERS_WAITING: i32 = 1 << 30; const WRITERS_WAITING: i32 = 1 << 31; -fn unlocked(state: i32) -> bool { +fn is_unlocked(state: i32) -> bool { state & MASK == 0 } -fn write_locked(state: i32) -> bool { +fn is_write_locked(state: i32) -> bool { state & MASK == WRITE_LOCKED } -fn readers_waiting(state: i32) -> bool { +fn has_readers_waiting(state: i32) -> bool { state & READERS_WAITING != 0 } -fn writers_waiting(state: i32) -> bool { +fn has_writers_waiting(state: i32) -> bool { state & WRITERS_WAITING != 0 } -fn read_lockable(state: i32) -> bool { +fn is_read_lockable(state: i32) -> bool { // This also returns false if the counter could overflow if we tried to read lock it. // // We don't allow read-locking if there's readers waiting, even if the lock is unlocked // and there's no writers waiting. The only situation when this happens is after unlocking, // at which point the unlocking thread might be waking up writers, which have priority over readers. // The unlocking thread will clear the readers waiting bit and wake up readers, if necssary. - state & MASK < MAX_READERS && !readers_waiting(state) && !writers_waiting(state) + state & MASK < MAX_READERS && !has_readers_waiting(state) && !has_writers_waiting(state) } -fn reached_max_readers(state: i32) -> bool { +fn has_reached_max_readers(state: i32) -> bool { state & MASK == MAX_READERS } @@ -69,14 +69,14 @@ impl RwLock { #[inline] pub unsafe fn try_read(&self) -> bool { self.state - .fetch_update(Acquire, Relaxed, |s| read_lockable(s).then(|| s + READ_LOCKED)) + .fetch_update(Acquire, Relaxed, |s| is_read_lockable(s).then(|| s + READ_LOCKED)) .is_ok() } #[inline] pub unsafe fn read(&self) { let state = self.state.load(Relaxed); - if !read_lockable(state) + if !is_read_lockable(state) || self .state .compare_exchange_weak(state, state + READ_LOCKED, Acquire, Relaxed) @@ -92,10 +92,10 @@ impl RwLock { // It's impossible for a reader to be waiting on a read-locked RwLock, // except if there is also a writer waiting. - debug_assert!(!readers_waiting(state) || writers_waiting(state)); + debug_assert!(!has_readers_waiting(state) || has_writers_waiting(state)); // Wake up a writer if we were the last reader and there's a writer waiting. - if unlocked(state) && writers_waiting(state) { + if is_unlocked(state) && has_writers_waiting(state) { self.wake_writer_or_readers(state); } } @@ -106,7 +106,7 @@ impl RwLock { loop { // If we can lock it, lock it. - if read_lockable(state) { + if is_read_lockable(state) { match self.state.compare_exchange_weak(state, state + READ_LOCKED, Acquire, Relaxed) { Ok(_) => return, // Locked! @@ -118,12 +118,12 @@ impl RwLock { } // Check for overflow. - if reached_max_readers(state) { + if has_reached_max_readers(state) { panic!("too many active read locks on RwLock"); } // Make sure the readers waiting bit is set before we go to sleep. - if !readers_waiting(state) { + if !has_readers_waiting(state) { if let Err(s) = self.state.compare_exchange(state, state | READERS_WAITING, Relaxed, Relaxed) { @@ -142,7 +142,9 @@ impl RwLock { #[inline] pub unsafe fn try_write(&self) -> bool { - self.state.fetch_update(Acquire, Relaxed, |s| unlocked(s).then(|| s + WRITE_LOCKED)).is_ok() + self.state + .fetch_update(Acquire, Relaxed, |s| is_unlocked(s).then(|| s + WRITE_LOCKED)) + .is_ok() } #[inline] @@ -156,9 +158,9 @@ impl RwLock { pub unsafe fn write_unlock(&self) { let state = self.state.fetch_sub(WRITE_LOCKED, Release) - WRITE_LOCKED; - debug_assert!(unlocked(state)); + debug_assert!(is_unlocked(state)); - if writers_waiting(state) || readers_waiting(state) { + if has_writers_waiting(state) || has_readers_waiting(state) { self.wake_writer_or_readers(state); } } @@ -171,7 +173,7 @@ impl RwLock { loop { // If it's unlocked, we try to lock it. - if unlocked(state) { + if is_unlocked(state) { match self.state.compare_exchange_weak( state, state | WRITE_LOCKED | other_writers_waiting, @@ -187,7 +189,7 @@ impl RwLock { } // Set the waiting bit indicating that we're waiting on it. - if !writers_waiting(state) { + if !has_writers_waiting(state) { if let Err(s) = self.state.compare_exchange(state, state | WRITERS_WAITING, Relaxed, Relaxed) { @@ -207,7 +209,7 @@ impl RwLock { // Don't go to sleep if the lock has become available, // or if the writers waiting bit is no longer set. let s = self.state.load(Relaxed); - if unlocked(state) || !writers_waiting(s) { + if is_unlocked(state) || !has_writers_waiting(s) { state = s; continue; } @@ -226,7 +228,7 @@ impl RwLock { /// back to waking up readers if there was no writer to wake up. #[cold] fn wake_writer_or_readers(&self, mut state: i32) { - assert!(unlocked(state)); + assert!(is_unlocked(state)); // The readers waiting bit might be turned on at any point now, // since readers will block when there's anything waiting. @@ -299,13 +301,13 @@ impl RwLock { fn spin_write(&self) -> i32 { // Stop spinning when it's unlocked or when there's waiting writers, to keep things somewhat fair. - self.spin_until(|state| unlocked(state) || writers_waiting(state)) + self.spin_until(|state| is_unlocked(state) || has_writers_waiting(state)) } fn spin_read(&self) -> i32 { // Stop spinning when it's unlocked or read locked, or when there's waiting threads. self.spin_until(|state| { - !write_locked(state) || readers_waiting(state) || writers_waiting(state) + !is_write_locked(state) || has_readers_waiting(state) || has_writers_waiting(state) }) } }