From 54dbd0baadae58c811b0c85de4223cbf81f24725 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Wed, 20 May 2015 05:06:38 -0400 Subject: [PATCH] librustc_trans: Remove misoptimization in treating derefs of Box as rvalues. --- src/librustc_trans/trans/datum.rs | 11 ------ src/librustc_trans/trans/expr.rs | 57 ++++--------------------------- src/test/run-pass/issue-18845.rs | 25 ++++++++++++++ src/test/run-pass/issue-25497.rs | 28 +++++++++++++++ 4 files changed, 60 insertions(+), 61 deletions(-) create mode 100644 src/test/run-pass/issue-18845.rs create mode 100644 src/test/run-pass/issue-25497.rs diff --git a/src/librustc_trans/trans/datum.rs b/src/librustc_trans/trans/datum.rs index a736a9fe88a..dd32ed3bc1e 100644 --- a/src/librustc_trans/trans/datum.rs +++ b/src/librustc_trans/trans/datum.rs @@ -74,17 +74,6 @@ //! `&foo()` or `match foo() { ref x => ... }`, where the user is //! implicitly requesting a temporary. //! -//! Somewhat surprisingly, not all lvalue expressions yield lvalue datums -//! when trans'd. Ultimately the reason for this is to micro-optimize -//! the resulting LLVM. For example, consider the following code: -//! -//! fn foo() -> Box { ... } -//! let x = *foo(); -//! -//! The expression `*foo()` is an lvalue, but if you invoke `expr::trans`, -//! it will return an rvalue datum. See `deref_once` in expr.rs for -//! more details. -//! //! ### Rvalues in detail //! //! Rvalues datums are values with no cleanup scheduled. One must be diff --git a/src/librustc_trans/trans/expr.rs b/src/librustc_trans/trans/expr.rs index 33eb3814087..3f781851d30 100644 --- a/src/librustc_trans/trans/expr.rs +++ b/src/librustc_trans/trans/expr.rs @@ -2247,16 +2247,20 @@ fn deref_once<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, let r = match datum.ty.sty { ty::ty_uniq(content_ty) => { + // Make sure we have an lvalue datum here to get the + // proper cleanups scheduled + let datum = unpack_datum!( + bcx, datum.to_lvalue_datum(bcx, "deref", expr.id)); + if type_is_sized(bcx.tcx(), content_ty) { - deref_owned_pointer(bcx, expr, datum, content_ty) + let ptr = load_ty(bcx, datum.val, datum.ty); + DatumBlock::new(bcx, Datum::new(ptr, content_ty, LvalueExpr)) } else { // A fat pointer and a DST lvalue have the same representation // just different types. Since there is no temporary for `*e` // here (because it is unsized), we cannot emulate the sized // object code path for running drop glue and free. Instead, // we schedule cleanup for `e`, turning it into an lvalue. - let datum = unpack_datum!( - bcx, datum.to_lvalue_datum(bcx, "deref", expr.id)); let datum = Datum::new(datum.val, content_ty, LvalueExpr); DatumBlock::new(bcx, datum) @@ -2293,53 +2297,6 @@ fn deref_once<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, expr.id, method_call, r.datum.to_string(ccx)); return r; - - /// We microoptimize derefs of owned pointers a bit here. Basically, the idea is to make the - /// deref of an rvalue result in an rvalue. This helps to avoid intermediate stack slots in the - /// resulting LLVM. The idea here is that, if the `Box` pointer is an rvalue, then we can - /// schedule a *shallow* free of the `Box` pointer, and then return a ByRef rvalue into the - /// pointer. Because the free is shallow, it is legit to return an rvalue, because we know that - /// the contents are not yet scheduled to be freed. The language rules ensure that the contents - /// will be used (or moved) before the free occurs. - fn deref_owned_pointer<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, - expr: &ast::Expr, - datum: Datum<'tcx, Expr>, - content_ty: Ty<'tcx>) - -> DatumBlock<'blk, 'tcx, Expr> { - match datum.kind { - RvalueExpr(Rvalue { mode: ByRef }) => { - let scope = cleanup::temporary_scope(bcx.tcx(), expr.id); - let ptr = Load(bcx, datum.val); - if !type_is_zero_size(bcx.ccx(), content_ty) { - bcx.fcx.schedule_free_value(scope, ptr, cleanup::HeapExchange, content_ty); - } - } - RvalueExpr(Rvalue { mode: ByValue }) => { - let scope = cleanup::temporary_scope(bcx.tcx(), expr.id); - if !type_is_zero_size(bcx.ccx(), content_ty) { - bcx.fcx.schedule_free_value(scope, datum.val, cleanup::HeapExchange, - content_ty); - } - } - LvalueExpr => { } - } - - // If we had an rvalue in, we produce an rvalue out. - let (llptr, kind) = match datum.kind { - LvalueExpr => { - (Load(bcx, datum.val), LvalueExpr) - } - RvalueExpr(Rvalue { mode: ByRef }) => { - (Load(bcx, datum.val), RvalueExpr(Rvalue::new(ByRef))) - } - RvalueExpr(Rvalue { mode: ByValue }) => { - (datum.val, RvalueExpr(Rvalue::new(ByRef))) - } - }; - - let datum = Datum { ty: content_ty, val: llptr, kind: kind }; - DatumBlock { bcx: bcx, datum: datum } - } } #[derive(Debug)] diff --git a/src/test/run-pass/issue-18845.rs b/src/test/run-pass/issue-18845.rs new file mode 100644 index 00000000000..3d8e0556a56 --- /dev/null +++ b/src/test/run-pass/issue-18845.rs @@ -0,0 +1,25 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This used to generate invalid IR in that even if we took the +// `false` branch we'd still try to free the Box from the other +// arm. This was due to treating `*Box::new(9)` as an rvalue datum +// instead of as an lvalue. + +fn test(foo: bool) -> u8 { + match foo { + true => *Box::new(9), + false => 0 + } +} + +fn main() { + assert_eq!(9, test(true)); +} diff --git a/src/test/run-pass/issue-25497.rs b/src/test/run-pass/issue-25497.rs new file mode 100644 index 00000000000..730b0a274bf --- /dev/null +++ b/src/test/run-pass/issue-25497.rs @@ -0,0 +1,28 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#[derive(Clone, Debug, PartialEq)] +enum Expression { + Dummy, + Add(Box), +} + +use Expression::*; + +fn simplify(exp: Expression) -> Expression { + match exp { + Add(n) => *n.clone(), + _ => Dummy + } +} + +fn main() { + assert_eq!(simplify(Add(Box::new(Dummy))), Dummy); +}