Skip to content

ScalarPairs are offset==0 field + other non-zst field #51307

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 5, 2018
Merged

ScalarPairs are offset==0 field + other non-zst field #51307

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47f37d6
Correctly access ScalarPair fields during const eval
oli-obk Jun 2, 2018
f7eedfa
Simplify value field access
oli-obk Jun 4, 2018
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
4 changes: 2 additions & 2 deletions src/librustc_mir/interpret/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, ...
Copy link
Member

Choose a reason for hiding this comment

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

This comment is too specific.

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
Expand Down
90 changes: 32 additions & 58 deletions src/librustc_mir/interpret/terminator/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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))?;
Copy link
Member

Choose a reason for hiding this comment

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

Why not do something like this for the source, too?

let valty = ValTy {
value,
ty: field.ty,
};
self.write_value(valty, dest)?;
}
} else {
trace!("manual impl of rust-call ABI");
Expand Down
24 changes: 5 additions & 19 deletions src/librustc_mir/transform/const_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Like this, maybe?

})??;
Some((value, field_ty, span))
},
_ => None,
},
Expand Down
41 changes: 41 additions & 0 deletions src/test/ui/const-eval/issue-51300.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<Stat> {
let stat = Stat{id, index};
match stat {
Stat::STUDENT_HAPPINESS => Some(Stat::STUDENT_HAPPINESS),
Stat::STUDENT_HUNGER => Some(Stat::STUDENT_HUNGER),
_ => None,
}
}

fn main() { }