From 365807f0576b719ba9af6b4f724bb5f133ea81ac Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 17 Apr 2020 09:27:35 -0300 Subject: [PATCH 1/2] Make Box respect self alignment --- .../build/expr/as_operand.rs | 121 ++++++++++++++++++ src/librustc_mir_build/build/expr/into.rs | 6 +- src/test/ui/fn/dyn-fn-alignment.rs | 24 ++++ .../unsized-locals/borrow-after-move.stderr | 6 +- src/test/ui/unsized-locals/double-move.stderr | 8 +- .../ui/unsized-locals/unsized-exprs2.stderr | 10 +- 6 files changed, 160 insertions(+), 15 deletions(-) create mode 100644 src/test/ui/fn/dyn-fn-alignment.rs diff --git a/src/librustc_mir_build/build/expr/as_operand.rs b/src/librustc_mir_build/build/expr/as_operand.rs index 783637fa45831..5216d305ffbd9 100644 --- a/src/librustc_mir_build/build/expr/as_operand.rs +++ b/src/librustc_mir_build/build/expr/as_operand.rs @@ -21,6 +21,64 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.as_operand(block, local_scope, expr) } + /// Returns an operand suitable for use until the end of the current scope expression and + /// suitable also to be passed as function arguments. + /// + /// The operand returned from this function will *not be valid* after an ExprKind::Scope is + /// passed, so please do *not* return it from functions to avoid bad miscompiles. Returns an + /// operand suitable for use as a call argument. This is almost always equivalent to + /// `as_operand`, except for the particular case of passing values of (potentially) unsized + /// types "by value" (see details below). + /// + /// As with `as_operand`, the operand returned from this function will *not be valid* after an + /// `ExprKind::Scope` is passed, so do not return it from functions. + /// + /// # Parameters of unsized types + /// + /// We tweak the handling of parameters of unsized type slightly to avoid the need to create a + /// local variable of unsized type. For example, consider this program: + /// + /// ```rust + /// fn foo(p: dyn Debug) { ... } + /// + /// fn bar(box_p: Box) { foo(*p); } + /// ``` + /// + /// Ordinarily, for sized types, we would compile the call `foo(*p)` like so: + /// + /// ```rust + /// let tmp0 = *box_p; // tmp0 would be the operand returned by this function call + /// foo(tmp0) + /// ``` + /// + /// But because the parameter to `foo` is of the unsized type `dyn Debug`, and because it is + /// being moved the deref of a box, we compile it slightly differently. The temporary `tmp0` + /// that we create *stores the entire box*, and the parameter to the call itself will be + /// `*tmp0`: + /// + /// ```rust + /// let tmp0 = box_p; call foo(*tmp0) + /// ``` + /// + /// This way, the temporary `tmp0` that we create has type `Box`, which is sized. + /// The value passed to the call (`*tmp0`) still has the `dyn Debug` type -- but the way that + /// calls are compiled means that this parameter will be passed "by reference", meaning that we + /// will actually provide a pointer to the interior of the box, and not move the `dyn Debug` + /// value to the stack. + /// + /// See #68034 for more details. + crate fn as_local_call_operand( + &mut self, + block: BasicBlock, + expr: M, + ) -> BlockAnd> + where + M: Mirror<'tcx, Output = Expr<'tcx>>, + { + let local_scope = self.local_scope(); + self.as_call_operand(block, local_scope, expr) + } + /// Compile `expr` into a value that can be used as an operand. /// If `expr` is a place like `x`, this will introduce a /// temporary `tmp = x`, so that we capture the value of `x` at @@ -40,6 +98,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.expr_as_operand(block, scope, expr) } + fn as_call_operand( + &mut self, + block: BasicBlock, + scope: Option, + expr: M, + ) -> BlockAnd> + where + M: Mirror<'tcx, Output = Expr<'tcx>>, + { + let expr = self.hir.mirror(expr); + self.expr_as_call_operand(block, scope, expr) + } + fn expr_as_operand( &mut self, mut block: BasicBlock, @@ -69,4 +140,54 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } } + + fn expr_as_call_operand( + &mut self, + mut block: BasicBlock, + scope: Option, + expr: Expr<'tcx>, + ) -> BlockAnd> { + debug!("expr_as_call_operand(block={:?}, expr={:?})", block, expr); + let this = self; + + if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind { + let source_info = this.source_info(expr.span); + let region_scope = (region_scope, source_info); + return this.in_scope(region_scope, lint_level, |this| { + this.as_call_operand(block, scope, value) + }); + } + + let tcx = this.hir.tcx(); + + if tcx.features().unsized_locals { + let ty = expr.ty; + let span = expr.span; + let param_env = this.hir.param_env; + + if !ty.is_sized(tcx.at(span), param_env) { + // !sized means !copy, so this is an unsized move + assert!(!ty.is_copy_modulo_regions(tcx, param_env, span)); + + // As described above, detect the case where we are passing a value of unsized + // type, and that value is coming from the deref of a box. + if let ExprKind::Deref { ref arg } = expr.kind { + let arg = this.hir.mirror(arg.clone()); + + // Generate let tmp0 = arg0 + let operand = unpack!(block = this.as_temp(block, scope, arg, Mutability::Mut)); + + // Return the operand *tmp0 to be used as the call argument + let place = Place { + local: operand, + projection: tcx.intern_place_elems(&[PlaceElem::Deref]), + }; + + return block.and(Operand::Move(place)); + } + } + } + + this.expr_as_operand(block, scope, expr) + } } diff --git a/src/librustc_mir_build/build/expr/into.rs b/src/librustc_mir_build/build/expr/into.rs index 6b93755e9da7c..3e073ec3ab5be 100644 --- a/src/librustc_mir_build/build/expr/into.rs +++ b/src/librustc_mir_build/build/expr/into.rs @@ -3,10 +3,10 @@ use crate::build::expr::category::{Category, RvalueFunc}; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; use crate::hair::*; -use rustc_middle::mir::*; -use rustc_middle::ty::{self, CanonicalUserTypeAnnotation}; use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; +use rustc_middle::mir::*; +use rustc_middle::ty::{self, CanonicalUserTypeAnnotation}; use rustc_span::symbol::sym; use rustc_target::spec::abi::Abi; @@ -207,7 +207,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } else { let args: Vec<_> = args .into_iter() - .map(|arg| unpack!(block = this.as_local_operand(block, arg))) + .map(|arg| unpack!(block = this.as_local_call_operand(block, arg))) .collect(); let success = this.cfg.start_new_block(); diff --git a/src/test/ui/fn/dyn-fn-alignment.rs b/src/test/ui/fn/dyn-fn-alignment.rs new file mode 100644 index 0000000000000..125f44bbf0093 --- /dev/null +++ b/src/test/ui/fn/dyn-fn-alignment.rs @@ -0,0 +1,24 @@ +// run-pass + +#![feature(unsized_locals)] +#![allow(dead_code)] +#[repr(align(256))] +struct A { + v: u8, +} + +impl A { + fn f(&self) -> *const A { + self + } +} + +fn f2(v: u8) -> Box *const A> { + let a = A { v }; + Box::new(move || a.f()) +} + +fn main() { + let addr = f2(0)(); + assert_eq!(addr as usize % 256, 0, "addr: {:?}", addr); +} diff --git a/src/test/ui/unsized-locals/borrow-after-move.stderr b/src/test/ui/unsized-locals/borrow-after-move.stderr index 010e182674b75..110edab69be87 100644 --- a/src/test/ui/unsized-locals/borrow-after-move.stderr +++ b/src/test/ui/unsized-locals/borrow-after-move.stderr @@ -45,12 +45,12 @@ LL | println!("{}", &y); error[E0382]: borrow of moved value: `x` --> $DIR/borrow-after-move.rs:39:24 | +LL | let x = "hello".to_owned().into_boxed_str(); + | - move occurs because `x` has type `std::boxed::Box`, which does not implement the `Copy` trait LL | x.foo(); | - value moved here LL | println!("{}", &x); - | ^^ value borrowed here after partial move - | - = note: move occurs because `*x` has type `str`, which does not implement the `Copy` trait + | ^^ value borrowed here after move error: aborting due to 5 previous errors diff --git a/src/test/ui/unsized-locals/double-move.stderr b/src/test/ui/unsized-locals/double-move.stderr index 47fa0d4a437ba..5b936fb64474f 100644 --- a/src/test/ui/unsized-locals/double-move.stderr +++ b/src/test/ui/unsized-locals/double-move.stderr @@ -38,25 +38,25 @@ LL | y.foo(); LL | y.foo(); | ^ value used here after move -error[E0382]: use of moved value: `*x` +error[E0382]: use of moved value: `x` --> $DIR/double-move.rs:45:9 | LL | let _y = *x; | -- value moved here LL | x.foo(); - | ^ value used here after move + | ^ value used here after partial move | = note: move occurs because `*x` has type `str`, which does not implement the `Copy` trait error[E0382]: use of moved value: `*x` --> $DIR/double-move.rs:51:18 | +LL | let x = "hello".to_owned().into_boxed_str(); + | - move occurs because `x` has type `std::boxed::Box`, which does not implement the `Copy` trait LL | x.foo(); | - value moved here LL | let _y = *x; | ^^ value used here after move - | - = note: move occurs because `*x` has type `str`, which does not implement the `Copy` trait error: aborting due to 6 previous errors diff --git a/src/test/ui/unsized-locals/unsized-exprs2.stderr b/src/test/ui/unsized-locals/unsized-exprs2.stderr index 650285cb6740a..88269f237afb7 100644 --- a/src/test/ui/unsized-locals/unsized-exprs2.stderr +++ b/src/test/ui/unsized-locals/unsized-exprs2.stderr @@ -1,11 +1,11 @@ error[E0508]: cannot move out of type `[u8]`, a non-copy slice - --> $DIR/unsized-exprs2.rs:22:19 + --> $DIR/unsized-exprs2.rs:22:5 | LL | udrop::<[u8]>(foo()[..]); - | ^^^^^^^^^ - | | - | cannot move out of here - | move occurs because value has type `[u8]`, which does not implement the `Copy` trait + | ^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | cannot move out of here + | move occurs because value has type `[u8]`, which does not implement the `Copy` trait error: aborting due to previous error From 4e53a9af4725222a617900aac0680f1a472255ec Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 20 Apr 2020 09:41:54 +0000 Subject: [PATCH 2/2] tweak comments --- src/librustc_mir_build/build/expr/as_operand.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir_build/build/expr/as_operand.rs b/src/librustc_mir_build/build/expr/as_operand.rs index 5216d305ffbd9..9a75f3afe8f08 100644 --- a/src/librustc_mir_build/build/expr/as_operand.rs +++ b/src/librustc_mir_build/build/expr/as_operand.rs @@ -10,9 +10,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// Returns an operand suitable for use until the end of the current /// scope expression. /// - /// The operand returned from this function will *not be valid* after - /// an ExprKind::Scope is passed, so please do *not* return it from - /// functions to avoid bad miscompiles. + /// The operand returned from this function will *not be valid* + /// after the current enclosing `ExprKind::Scope` has ended, so + /// please do *not* return it from functions to avoid bad + /// miscompiles. crate fn as_local_operand(&mut self, block: BasicBlock, expr: M) -> BlockAnd> where M: Mirror<'tcx, Output = Expr<'tcx>>, @@ -30,8 +31,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// `as_operand`, except for the particular case of passing values of (potentially) unsized /// types "by value" (see details below). /// - /// As with `as_operand`, the operand returned from this function will *not be valid* after an - /// `ExprKind::Scope` is passed, so do not return it from functions. + /// The operand returned from this function will *not be valid* + /// after the current enclosing `ExprKind::Scope` has ended, so + /// please do *not* return it from functions to avoid bad + /// miscompiles. /// /// # Parameters of unsized types /// @@ -98,6 +101,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.expr_as_operand(block, scope, expr) } + /// Like `as_local_call_operand`, except that the argument will + /// not be valid once `scope` ends. fn as_call_operand( &mut self, block: BasicBlock,