From 47f37d67f14ef50829c4ff95a3a27a680fd4e1ba Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Sat, 2 Jun 2018 23:34:25 +0200 Subject: [PATCH 1/2] Correctly access ScalarPair fields during const eval --- src/librustc_mir/interpret/place.rs | 4 +- src/librustc_mir/interpret/terminator/mod.rs | 90 +++++++------------- src/librustc_mir/transform/const_prop.rs | 24 ++---- src/test/ui/const-eval/issue-51300.rs | 41 +++++++++ 4 files changed, 80 insertions(+), 79 deletions(-) create mode 100644 src/test/ui/const-eval/issue-51300.rs diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index c1bcffe7e9a..d1f3b8b86de 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -136,8 +136,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { Value::ScalarPair(..) | Value::Scalar(_) if offset.bytes() == 0 && field.size == base_layout.size => Ok(Some((base, field.ty))), // split fat pointers, 2 element tuples, ... - Value::ScalarPair(a, b) if base_layout.fields.count() == 2 => { - let val = [a, b][field_index]; + Value::ScalarPair(a, b) => { + let val = if offset.bytes() == 0 { a } else { b }; Ok(Some((Value::Scalar(val), field.ty))) }, // FIXME(oli-obk): figure out whether we should be calling `try_read_value` here diff --git a/src/librustc_mir/interpret/terminator/mod.rs b/src/librustc_mir/interpret/terminator/mod.rs index 9151bfbdd1b..8ed60b2325a 100644 --- a/src/librustc_mir/interpret/terminator/mod.rs +++ b/src/librustc_mir/interpret/terminator/mod.rs @@ -4,7 +4,7 @@ use rustc::ty::layout::LayoutOf; use syntax::codemap::Span; use rustc_target::spec::abi::Abi; -use rustc::mir::interpret::{EvalResult, Scalar, Value}; +use rustc::mir::interpret::{EvalResult, Value}; use super::{EvalContext, Place, Machine, ValTy}; use rustc_data_structures::indexed_vec::Idx; @@ -338,65 +338,39 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { // unpack and write all other args let layout = self.layout_of(args[1].ty)?; - if let ty::TyTuple(..) = args[1].ty.sty { + if let ty::TyTuple(_) = args[1].ty.sty { + if layout.is_zst() { + // Nothing to do, no need to unpack zsts + return Ok(()); + } if self.frame().mir.args_iter().count() == layout.fields.count() + 1 { - match args[1].value { - Value::ByRef(ptr, align) => { - for (i, arg_local) in arg_locals.enumerate() { - let field = layout.field(&self, i)?; - let offset = layout.fields.offset(i); - let arg = Value::ByRef(ptr.ptr_offset(offset, &self)?, - align.min(field.align)); - let dest = - self.eval_place(&mir::Place::Local(arg_local))?; - trace!( - "writing arg {:?} to {:?} (type: {})", - arg, - dest, - field.ty - ); - let valty = ValTy { - value: arg, - ty: field.ty, - }; - self.write_value(valty, dest)?; - } - } - Value::Scalar(Scalar::Bits { defined: 0, .. }) => {} - other => { - trace!("{:#?}, {:#?}", other, layout); - let mut layout = layout; - 'outer: loop { - for i in 0..layout.fields.count() { - let field = layout.field(&self, i)?; - if layout.fields.offset(i).bytes() == 0 && layout.size == field.size { - layout = field; - continue 'outer; - } - } - break; - } - { - let mut write_next = |value| { - let dest = self.eval_place(&mir::Place::Local( - arg_locals.next().unwrap(), - ))?; - let valty = ValTy { - value: Value::Scalar(value), - ty: layout.ty, - }; - self.write_value(valty, dest) - }; - match other { - Value::Scalar(value) | Value::ScalarPair(value, _) => write_next(value)?, - _ => unreachable!(), - } - if let Value::ScalarPair(_, value) = other { - write_next(value)?; - } - } - assert!(arg_locals.next().is_none()); + for (i, arg_local) in arg_locals.enumerate() { + let field = layout.field(&self, i)?; + if field.is_zst() { + continue; } + let offset = layout.fields.offset(i); + let value = match args[1].value { + Value::ByRef(ptr, align) => Value::ByRef( + ptr.ptr_offset(offset, &self)?, + align.min(field.align), + ), + other if field.size == layout.size => { + // this is the case where the field covers the entire type + assert_eq!(offset.bytes(), 0); + other + }, + Value::ScalarPair(a, _) if offset.bytes() == 0 => Value::Scalar(a), + Value::ScalarPair(_, b) => Value::Scalar(b), + Value::Scalar(_) => bug!("Scalar does not cover entire type"), + }; + let dest = + self.eval_place(&mir::Place::Local(arg_local))?; + let valty = ValTy { + value, + ty: field.ty, + }; + self.write_value(valty, dest)?; } } else { trace!("manual impl of rust-call ABI"); diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index ef61fe099bf..4ef3fb5663f 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -29,7 +29,7 @@ use rustc::ty::subst::Substs; use rustc_data_structures::indexed_vec::IndexVec; use rustc::ty::ParamEnv; use rustc::ty::layout::{ - LayoutOf, TyLayout, LayoutError, LayoutCx, + LayoutOf, TyLayout, LayoutError, HasTyCtxt, TargetDataLayout, HasDataLayout, }; @@ -214,24 +214,10 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> { ProjectionElem::Field(field, _) => { trace!("field proj on {:?}", proj.base); let (base, ty, span) = self.eval_place(&proj.base)?; - match base { - Value::ScalarPair(a, b) => { - trace!("by val pair: {:?}, {:?}", a, b); - let base_layout = self.tcx.layout_of(self.param_env.and(ty)).ok()?; - trace!("layout computed"); - use rustc_data_structures::indexed_vec::Idx; - let field_index = field.index(); - let val = [a, b][field_index]; - let cx = LayoutCx { - tcx: self.tcx, - param_env: self.param_env, - }; - let field = base_layout.field(cx, field_index).ok()?; - trace!("projection resulted in: {:?}", val); - Some((Value::Scalar(val), field.ty, span)) - }, - _ => None, - } + let (value, field_ty) = self.use_ecx(span, |this| { + this.ecx.read_field(base, None, field, ty) + })??; + Some((value, field_ty, span)) }, _ => None, }, diff --git a/src/test/ui/const-eval/issue-51300.rs b/src/test/ui/const-eval/issue-51300.rs new file mode 100644 index 00000000000..f91711c3c0f --- /dev/null +++ b/src/test/ui/const-eval/issue-51300.rs @@ -0,0 +1,41 @@ +// Copyright 2018 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. + +// compile-pass +// https://github.com/rust-lang/rust/issues/51300 + +#[derive(PartialEq, Eq, Clone, Copy)] +pub struct Stat { + pub id: u8, + pub index: usize, +} + +impl Stat { + pub const STUDENT_HAPPINESS: Stat = Stat{ + id: 0, + index: 0, + }; + pub const STUDENT_HUNGER: Stat = Stat{ + id: 0, + index: Self::STUDENT_HAPPINESS.index + 1, + }; + +} + +pub fn from_index(id: u8, index: usize) -> Option { + let stat = Stat{id, index}; + match stat { + Stat::STUDENT_HAPPINESS => Some(Stat::STUDENT_HAPPINESS), + Stat::STUDENT_HUNGER => Some(Stat::STUDENT_HUNGER), + _ => None, + } +} + +fn main() { } From f7eedfab8e03ece5ebaf265f4c7255a8f69c98fc Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 4 Jun 2018 14:50:29 +0200 Subject: [PATCH 2/2] Simplify value field access --- src/librustc_mir/interpret/place.rs | 33 ++++++++++++++------ src/librustc_mir/interpret/terminator/mod.rs | 30 +++--------------- src/librustc_mir/transform/const_prop.rs | 6 ++-- 3 files changed, 30 insertions(+), 39 deletions(-) diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index d1f3b8b86de..51b33fa54b2 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -120,7 +120,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { variant: Option, field: mir::Field, base_ty: Ty<'tcx>, - ) -> EvalResult<'tcx, Option<(Value, Ty<'tcx>)>> { + ) -> EvalResult<'tcx, ValTy<'tcx>> { let mut base_layout = self.layout_of(base_ty)?; if let Some(variant_index) = variant { base_layout = base_layout.for_variant(self, variant_index); @@ -128,21 +128,34 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { let field_index = field.index(); let field = base_layout.field(self, field_index)?; if field.size.bytes() == 0 { - return Ok(Some((Value::Scalar(Scalar::undef()), field.ty))) + return Ok(ValTy { + value: Value::Scalar(Scalar::undef()), + ty: field.ty, + }); } let offset = base_layout.fields.offset(field_index); - match base { + let value = match base { // the field covers the entire type Value::ScalarPair(..) | - Value::Scalar(_) if offset.bytes() == 0 && field.size == base_layout.size => Ok(Some((base, field.ty))), - // split fat pointers, 2 element tuples, ... + Value::Scalar(_) if offset.bytes() == 0 && field.size == base_layout.size => base, + // extract fields from types with `ScalarPair` ABI Value::ScalarPair(a, b) => { let val = if offset.bytes() == 0 { a } else { b }; - Ok(Some((Value::Scalar(val), field.ty))) + Value::Scalar(val) }, - // FIXME(oli-obk): figure out whether we should be calling `try_read_value` here - _ => Ok(None), - } + Value::ByRef(base_ptr, align) => { + let offset = base_layout.fields.offset(field_index); + let ptr = base_ptr.ptr_offset(offset, self)?; + let align = align.min(base_layout.align).min(field.align); + assert!(!field.is_unsized()); + Value::ByRef(ptr, align) + }, + Value::Scalar(val) => bug!("field access on non aggregate {:?}, {:?}", val, base_ty), + }; + Ok(ValTy { + value, + ty: field.ty, + }) } fn try_read_place_projection( @@ -156,7 +169,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { }; let base_ty = self.place_ty(&proj.base); match proj.elem { - Field(field, _) => Ok(self.read_field(base, None, field, base_ty)?.map(|(f, _)| f)), + Field(field, _) => Ok(Some(self.read_field(base, None, field, base_ty)?.value)), // The NullablePointer cases should work fine, need to take care for normal enums Downcast(..) | Subslice { .. } | diff --git a/src/librustc_mir/interpret/terminator/mod.rs b/src/librustc_mir/interpret/terminator/mod.rs index 8ed60b2325a..2994b1b387f 100644 --- a/src/librustc_mir/interpret/terminator/mod.rs +++ b/src/librustc_mir/interpret/terminator/mod.rs @@ -4,7 +4,7 @@ use rustc::ty::layout::LayoutOf; use syntax::codemap::Span; use rustc_target::spec::abi::Abi; -use rustc::mir::interpret::{EvalResult, Value}; +use rustc::mir::interpret::EvalResult; use super::{EvalContext, Place, Machine, ValTy}; use rustc_data_structures::indexed_vec::Idx; @@ -345,31 +345,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { } if self.frame().mir.args_iter().count() == layout.fields.count() + 1 { for (i, arg_local) in arg_locals.enumerate() { - let field = layout.field(&self, i)?; - if field.is_zst() { - continue; - } - let offset = layout.fields.offset(i); - let value = match args[1].value { - Value::ByRef(ptr, align) => Value::ByRef( - ptr.ptr_offset(offset, &self)?, - align.min(field.align), - ), - other if field.size == layout.size => { - // this is the case where the field covers the entire type - assert_eq!(offset.bytes(), 0); - other - }, - Value::ScalarPair(a, _) if offset.bytes() == 0 => Value::Scalar(a), - Value::ScalarPair(_, b) => Value::Scalar(b), - Value::Scalar(_) => bug!("Scalar does not cover entire type"), - }; - let dest = - self.eval_place(&mir::Place::Local(arg_local))?; - let valty = ValTy { - value, - ty: field.ty, - }; + let field = mir::Field::new(i); + let valty = self.read_field(args[1].value, None, field, args[1].ty)?; + let dest = self.eval_place(&mir::Place::Local(arg_local))?; self.write_value(valty, dest)?; } } else { diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 4ef3fb5663f..40a6610c417 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -214,10 +214,10 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> { ProjectionElem::Field(field, _) => { trace!("field proj on {:?}", proj.base); let (base, ty, span) = self.eval_place(&proj.base)?; - let (value, field_ty) = self.use_ecx(span, |this| { + let valty = self.use_ecx(span, |this| { this.ecx.read_field(base, None, field, ty) - })??; - Some((value, field_ty, span)) + })?; + Some((valty.value, valty.ty, span)) }, _ => None, },