Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Box<dyn FnOnce> respect self alignment #71170

Merged
merged 2 commits into from
Apr 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 129 additions & 3 deletions src/librustc_mir_build/build/expr/as_operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<M>(&mut self, block: BasicBlock, expr: M) -> BlockAnd<Operand<'tcx>>
where
M: Mirror<'tcx, Output = Expr<'tcx>>,
Expand All @@ -21,6 +22,66 @@ 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).
///
/// 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
///
/// 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<dyn Debug>) { 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<dyn Debug>`, 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<M>(
&mut self,
block: BasicBlock,
expr: M,
) -> BlockAnd<Operand<'tcx>>
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
Expand All @@ -40,6 +101,21 @@ 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<M>(
&mut self,
block: BasicBlock,
scope: Option<region::Scope>,
expr: M,
) -> BlockAnd<Operand<'tcx>>
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,
Expand Down Expand Up @@ -69,4 +145,54 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}
}

fn expr_as_call_operand(
&mut self,
mut block: BasicBlock,
scope: Option<region::Scope>,
expr: Expr<'tcx>,
) -> BlockAnd<Operand<'tcx>> {
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)
}
}
6 changes: 3 additions & 3 deletions src/librustc_mir_build/build/expr/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/fn/dyn-fn-alignment.rs
Original file line number Diff line number Diff line change
@@ -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<dyn FnOnce() -> *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);
}
6 changes: 3 additions & 3 deletions src/test/ui/unsized-locals/borrow-after-move.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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<str>`, which does not implement the `Copy` trait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... this is a bit suboptimal, in that the move is really occurring because foo was invoked on *x -- i.e., the type str -- but the new IR makes the borrow checker see it otherwise. I'm not sure how easy/hard that would be to fix. Probably not worth worrying about, I think.

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

Expand Down
8 changes: 4 additions & 4 deletions src/test/ui/unsized-locals/double-move.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem ok 👍

|
= 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<str>`, 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

Expand Down
10 changes: 5 additions & 5 deletions src/test/ui/unsized-locals/unsized-exprs2.stderr
Original file line number Diff line number Diff line change
@@ -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

Expand Down