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

Get rid of the Scalar and ScalarPair variants of ConstValue... #55260

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
edf94c5
Update function names in comments
oli-obk Oct 19, 2018
856eaed
Simplify `ConstValue`
oli-obk Oct 21, 2018
fe4e950
Make the HashStable impl of Allocation safer for future changes of th…
oli-obk Oct 21, 2018
04fc561
The `Hash` and `Eq` impls of `ConstValue` were inherently broken
oli-obk Oct 21, 2018
b11b3cb
Fixup a bunch of things for "simplify const value"
oli-obk Oct 21, 2018
3aecb0d
Fix some simd intrinsics
oli-obk Oct 21, 2018
8d42376
Fix an ICE that can occur while errors are already being emitted
oli-obk Oct 21, 2018
847f5b9
Const prop is only interested in immediate constants right now
oli-obk Oct 21, 2018
e52644c
Update a few tests with their improved diagnostic messages
oli-obk Oct 21, 2018
8246dd4
Alignment check facepalm
oli-obk Oct 21, 2018
5620d68
Enums can produce scalar pairs with undefs in one of the elements
oli-obk Oct 21, 2018
fc6472c
Hide a constructor function that is unused outside this module
oli-obk Oct 22, 2018
336094f
Use a more appropriate parameter environment
oli-obk Oct 22, 2018
b4ba332
Don't resolve `impl Trait` in function def constants to the concrete …
oli-obk Oct 22, 2018
dfc5d26
Constants carry their type with them
oli-obk Oct 22, 2018
c50d77d
Alignment of `const_field` results must be of the field's type
oli-obk Oct 22, 2018
1d14a84
Add a debugging aid
oli-obk Oct 22, 2018
9fbce39
Change some useless `ParamEnv::empty` to `reveal_all`
oli-obk Oct 22, 2018
1f2fcee
Improve debugging output of layout computation failure
oli-obk Oct 22, 2018
e3bbe6d
Shrink allocations to fit the type at hand
oli-obk Oct 22, 2018
6ff70da
Resize `Allocation`s when doing field accesses into them
oli-obk Oct 22, 2018
7b526d8
Read the slice length from the correct place in the fat pointer
oli-obk Oct 22, 2018
9df63e9
Global Allocations don't have an `extra` value
oli-obk Oct 22, 2018
a7a92e2
Rebase fallout
oli-obk Oct 22, 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
33 changes: 17 additions & 16 deletions src/librustc/mir/interpret/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -684,24 +684,24 @@ impl<Tag: Copy, Extra: Default> Allocation<Tag, Extra> {
offset: Size,
size: Size,
) -> EvalResult<'tcx> {
if self.relocations(hdl, offset, size)?.len() != 0 {
if self.relocations(hdl, offset, size).len() != 0 {
err!(ReadPointerAsBytes)
} else {
Ok(())
}
}

pub fn relocations(
fn relocations(
&self,
hdl: impl HasDataLayout,
offset: Size,
size: Size,
) -> EvalResult<'tcx, &[(Size, (Tag, AllocId))]> {
) -> &[(Size, (Tag, AllocId))] {
// We have to go back `pointer_size - 1` bytes, as that one would still overlap with
// the beginning of this range.
let start = offset.bytes().saturating_sub(hdl.pointer_size().bytes() - 1);
let start = offset.bytes().saturating_sub(hdl.data_layout().pointer_size.bytes() - 1);
let end = offset + size; // this does overflow checking
Ok(self.relocations.range(Size::from_bytes(start)..end))
self.relocations.range(Size::from_bytes(start)..end)
}

pub fn get_bytes(
Copy link
Member

@RalfJung RalfJung Oct 23, 2018

Choose a reason for hiding this comment

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

Needs a doc comment explaining exactly which checks are performed, and which not. (Internal relocations? Relocations on the edges?)

Also, if memory.rs uses this in copy, it is required to provide an address stability guarantee on its return value.

Expand Down Expand Up @@ -736,22 +736,23 @@ impl<Tag: Copy, Extra: Default> Allocation<Tag, Extra> {
&self,
hdl: impl HasDataLayout,
offset: Size,
size: Size,
align: Align,
) -> EvalResult<'tcx, Scalar<Tag>> {
Copy link
Member

Choose a reason for hiding this comment

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

Wait, why does get_bytes return a Scalar<Tag>? That doesn't make any sense, a method of that name should return a &[u8]. Now I am even more puzzled about what it does.

(See, a doc comment would have been useful. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function is called read_scalar?

Copy link
Member

@RalfJung RalfJung Oct 23, 2018

Choose a reason for hiding this comment

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

Hm did the diff view fool me? Yes it did.

I am still quite confused why this looks so different from what we do in memory.rs?

let size = hdl.data_layout().pointer_size;
let required_align = hdl.data_layout().pointer_align;
self.check_align(offset, required_align)?;
self.check_align(offset, align)?;
self.check_bounds(offset, size, true)?;
self.check_defined(offset, size)?;
let bytes = self.bytes_ignoring_relocations_and_undef(offset, size);
let offset = read_target_uint(hdl.data_layout().endian, &bytes).unwrap();
let offset = Size::from_bytes(offset as u64);
if let Some(&(tag, alloc_id)) = self.relocations.get(&offset) {
Ok(Pointer::new_with_tag(alloc_id, offset, tag).into())
} else {
Ok(Scalar::Bits {
bits: offset.bytes() as u128,
let int = read_target_uint(hdl.data_layout().endian, &bytes).unwrap();
match self.relocations(hdl, offset, size) {
&[(_, (tag, alloc_id))] if size == hdl.data_layout().pointer_size => {
Ok(Pointer::new_with_tag(alloc_id, Size::from_bytes(int as u64), tag).into())
},
&[] => Ok(Scalar::Bits {
bits: int,
size: size.bytes() as u8,
})
}),
_ => err!(ReadPointerAsBytes),
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/librustc/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ impl<'tcx> ConstValue<'tcx> {
hdl: impl HasDataLayout,
) -> Option<Pointer> {
let (_, alloc, offset) = self.try_as_by_ref()?;
alloc.read_scalar(hdl, offset).ok()?.to_ptr().ok()
let size = hdl.data_layout().pointer_size;
let required_align = hdl.data_layout().pointer_align;
alloc.read_scalar(hdl, offset, size, required_align).ok()?.to_ptr().ok()
}

/// e.g. for vtables, fat pointers or single pointers
Expand Down
4 changes: 3 additions & 1 deletion src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2487,7 +2487,9 @@ pub fn fmt_const_val(f: &mut impl Write, const_val: &ty::Const<'_>) -> fmt::Resu
},
// print string literals
Ref(_, &ty::TyS { sty: Str, .. }, _) => {
let ptr = ty::tls::with(|tcx| alloc.read_scalar(tcx, offset));
let ptr = ty::tls::with(|tcx| alloc.read_scalar(
tcx, offset, tcx.data_layout.pointer_size, tcx.data_layout.pointer_align,
));
let ptr = ptr.and_then(Scalar::to_ptr);
if let Ok(ptr) = ptr {
let len = ty::tls::with(|tcx| alloc.read_bits(
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_codegen_llvm/mir/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ impl FunctionCx<'a, 'll, 'tcx> {
constant
.and_then(|c| {
let field_ty = c.ty.builtin_index().unwrap();
let layout = bx.cx.layout_of(field_ty);
let fields = match c.ty.sty {
ty::Array(_, n) => n.unwrap_usize(bx.tcx()),
ref other => bug!("invalid simd shuffle type: {}", other),
Expand All @@ -195,8 +196,7 @@ impl FunctionCx<'a, 'll, 'tcx> {
c,
)?;
// FIXME(oli-obk): are these indices always usize?
if let Some(prim) = field.val.try_to_usize(bx.tcx()) {
let layout = bx.cx.layout_of(field_ty);
if let Some(prim) = field.val.try_to_bits(bx.tcx(), layout) {
let scalar = match layout.abi {
layout::Abi::Scalar(ref x) => x,
_ => bug!("from_const: invalid ByVal layout: {:#?}", layout)
Expand Down
64 changes: 60 additions & 4 deletions src/librustc_codegen_llvm/mir/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use rustc::mir;
use rustc::ty;
use rustc::ty::layout::{self, Align, LayoutOf, TyLayout};
use rustc_data_structures::sync::Lrc;
use syntax_pos::Span;

use base;
use common::{CodegenCx, C_undef, C_usize};
Expand All @@ -25,6 +26,7 @@ use glue;
use std::fmt;

use super::{FunctionCx, LocalRef};
use super::constant::scalar_to_llvm;
use super::place::PlaceRef;

/// The representation of a Rust value. The enum variant is in fact
Expand Down Expand Up @@ -77,6 +79,7 @@ impl OperandRef<'ll, 'tcx> {
}

pub fn from_const(bx: &Builder<'a, 'll, 'tcx>,
span: Span,
val: &'tcx ty::Const<'tcx>)
-> Result<OperandRef<'ll, 'tcx>, Lrc<ConstEvalErr<'tcx>>> {
let layout = bx.cx.layout_of(val.ty);
Expand All @@ -85,12 +88,65 @@ impl OperandRef<'ll, 'tcx> {
return Ok(OperandRef::new_zst(bx.cx, layout));
}

match val.val {
let econv = |err| ConstEvalErr {
error: err,
stacktrace: Vec::new(),
span,
};

let val = match val.val {
ConstValue::Unevaluated(..) => bug!(),
ConstValue::ByRef(_, alloc, offset) => {
Ok(PlaceRef::from_const_alloc(bx, layout, alloc, offset).load(bx))
// FIXME: the first two arms are needed for simd_simple_float_intrinsic which reads
// the constants back from llvm values. We can probably do better.
match layout.abi {
layout::Abi::Scalar(ref scalar) => {
let x = alloc.read_scalar(
bx.tcx(), offset, layout.size, layout.align,
).map_err(econv)?;
let llval = scalar_to_llvm(
bx.cx,
x,
scalar,
layout.immediate_llvm_type(bx.cx),
);
OperandValue::Immediate(llval)
},
layout::Abi::ScalarPair(ref a_scalar, ref b_scalar) => {
Copy link
Member

@RalfJung RalfJung Oct 23, 2018

Choose a reason for hiding this comment

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

This is a duplicate of... read_value, I guess? Why can't we use that instead?

let a_size = a_scalar.value.size(bx.tcx());
let a = alloc.read_scalar(
bx.tcx(), offset, a_size, a_scalar.value.align(bx.tcx()),
).map_err(econv)?;
let b_align = b_scalar.value.align(bx.tcx());
let b_offset = offset + a_size.abi_align(b_align);
let b_size = b_scalar.value.size(bx.tcx());
let b = alloc.read_scalar(
bx.tcx(), b_offset, b_size, b_align,
).map_err(econv)?;
let a_llval = scalar_to_llvm(
bx.cx,
a,
a_scalar,
layout.scalar_pair_element_llvm_type(bx.cx, 0, true),
);
let b_layout = layout.scalar_pair_element_llvm_type(bx.cx, 1, true);
let b_llval = scalar_to_llvm(
bx.cx,
b,
b_scalar,
b_layout,
);
OperandValue::Pair(a_llval, b_llval)
},
_ => return Ok(PlaceRef::from_const_alloc(bx, layout, alloc, offset).load(bx)),
}
},
}
};

Ok(OperandRef {
val,
layout
})
}

/// Asserts that this operand refers to a scalar and returns
Expand Down Expand Up @@ -383,7 +439,7 @@ impl FunctionCx<'a, 'll, 'tcx> {
mir::Operand::Constant(ref constant) => {
let ty = self.monomorphize(&constant.ty);
self.eval_mir_constant(bx, constant)
.and_then(|c| OperandRef::from_const(bx, c))
.and_then(|c| OperandRef::from_const(bx, constant.span, c))
.unwrap_or_else(|err| {
err.report_as_error(
bx.tcx().at(constant.span),
Expand Down