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.

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

fix Miri discriminant handling #63448

Merged
merged 6 commits into from
Sep 20, 2019
Merged

fix Miri discriminant handling #63448

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8c0f601
fix Miri discriminant load/store when overflows are involved
RalfJung Aug 17, 2019
b73f1a5
better and more consistent variable names
RalfJung Sep 16, 2019
75fe84f
factor getting the discriminant layout to a new method
RalfJung Sep 16, 2019
61a28d5
do the variant idx computations on the host (non-overflowing)
RalfJung Sep 16, 2019
b21622c
add test
RalfJung Sep 16, 2019
1de533a
first determine if the variant is a niche-variant, then compute absol…
RalfJung Sep 19, 2019
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
11 changes: 11 additions & 0 deletions src/librustc/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ impl IntegerExt for Integer {

pub trait PrimitiveExt {
fn to_ty<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx>;
fn to_int_ty<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx>;
}

impl PrimitiveExt for Primitive {
Expand All @@ -138,6 +139,16 @@ impl PrimitiveExt for Primitive {
Pointer => tcx.mk_mut_ptr(tcx.mk_unit()),
}
}

/// Return an *integer* type matching this primitive.
/// Useful in particular when dealing with enum discriminants.
fn to_int_ty(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> {
match *self {
Int(i, signed) => i.to_ty(tcx, signed),
Pointer => tcx.types.usize,
Float(..) => bug!("floats do not have an int type"),
}
}
}

/// The first half of a fat pointer.
Expand Down
42 changes: 31 additions & 11 deletions src/librustc_mir/interpret/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::convert::TryInto;

use rustc::{mir, ty};
use rustc::ty::layout::{
self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerExt, VariantIdx,
self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerExt, PrimitiveExt, VariantIdx,
};

use rustc::mir::interpret::{
Expand Down Expand Up @@ -609,15 +609,20 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
) -> InterpResult<'tcx, (u128, VariantIdx)> {
trace!("read_discriminant_value {:#?}", rval.layout);

let (discr_kind, discr_index) = match rval.layout.variants {
let (discr_layout, discr_kind, discr_index) = match rval.layout.variants {
layout::Variants::Single { index } => {
let discr_val = rval.layout.ty.discriminant_for_variant(*self.tcx, index).map_or(
index.as_u32() as u128,
|discr| discr.val);
return Ok((discr_val, index));
}
layout::Variants::Multiple { ref discr_kind, discr_index, .. } =>
(discr_kind, discr_index),
layout::Variants::Multiple {
discr: ref discr_layout,
ref discr_kind,
discr_index,
..
} =>
(discr_layout, discr_kind, discr_index),
};

// read raw discriminant value
Expand All @@ -634,7 +639,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
.map_err(|_| err_unsup!(InvalidDiscriminant(raw_discr.erase_tag())))?;
let real_discr = if discr_val.layout.ty.is_signed() {
// going from layout tag type to typeck discriminant type
// requires first sign extending with the layout discriminant
// requires first sign extending with the discriminant layout
let sexted = sign_extend(bits_discr, discr_val.layout.size) as i128;
// and then zeroing with the typeck discriminant type
let discr_ty = rval.layout.ty
Expand Down Expand Up @@ -682,16 +687,31 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
(dataful_variant.as_u32() as u128, dataful_variant)
},
Ok(raw_discr) => {
let adjusted_discr = raw_discr.wrapping_sub(niche_start)
.wrapping_add(variants_start);
if variants_start <= adjusted_discr && adjusted_discr <= variants_end {
let index = adjusted_discr as usize;
assert_eq!(index as u128, adjusted_discr);
// We need to use machine arithmetic to get the relative variant idx.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
let discr_layout = self.layout_of(discr_layout.value.to_int_ty(*self.tcx))?;
let discr_val = ImmTy::from_uint(raw_discr, discr_layout);
let niche_start_val = ImmTy::from_uint(niche_start, discr_layout);
let variant_index_relative_val = self.binary_op(
mir::BinOp::Sub,
discr_val,
niche_start_val,
)?;
let variant_index_relative = variant_index_relative_val
.to_scalar()?
.assert_bits(discr_val.layout.size);
// Then computing the absolute variant idx should not overflow any more.
let variant_index = variants_start
.checked_add(variant_index_relative)
.expect("oveflow computing absolute variant idx");
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this can overflow when it's the dataful variant. Still, ICE > silently doing the wrong thing.

// Check if this is in the range that indicates an actual discriminant.
if variants_start <= variant_index && variant_index <= variants_end {
Copy link
Member

@eddyb eddyb Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh wait you can just compare variant_index_relative against variants_end - variants_start, and then you can move the addition inside the "niche" side of the if.

Copy link
Member Author

Choose a reason for hiding this comment

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

So the if should change to variant_index_relative <= variants_end - variants_start you say?

What is the "niche side of the if"?

let index = variant_index as usize;
assert_eq!(index as u128, variant_index);
assert!(index < rval.layout.ty
.ty_adt_def()
.expect("tagged layout for non adt")
.variants.len());
(adjusted_discr, VariantIdx::from_usize(index))
(variant_index, VariantIdx::from_usize(index))
} else {
(dataful_variant.as_u32() as u128, dataful_variant)
}
Expand Down
33 changes: 22 additions & 11 deletions src/librustc_mir/interpret/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use std::hash::Hash;
use rustc::mir;
use rustc::mir::interpret::truncate;
use rustc::ty::{self, Ty};
use rustc::ty::layout::{self, Size, Align, LayoutOf, TyLayout, HasDataLayout, VariantIdx};
use rustc::ty::layout::{
self, Size, Align, LayoutOf, TyLayout, HasDataLayout, VariantIdx, PrimitiveExt
};
use rustc::ty::TypeFoldable;

use super::{
Expand Down Expand Up @@ -1027,7 +1029,7 @@ where
}
layout::Variants::Multiple {
discr_kind: layout::DiscriminantKind::Tag,
ref discr,
discr: ref discr_layout,
discr_index,
..
} => {
Expand All @@ -1038,7 +1040,7 @@ where
// raw discriminants for enums are isize or bigger during
// their computation, but the in-memory tag is the smallest possible
// representation
let size = discr.value.size(self);
let size = discr_layout.value.size(self);
let discr_val = truncate(discr_val, size);

let discr_dest = self.place_field(dest, discr_index as u64)?;
Expand All @@ -1050,22 +1052,31 @@ where
ref niche_variants,
niche_start,
},
discr: ref discr_layout,
discr_index,
..
} => {
assert!(
variant_index.as_usize() < dest.layout.ty.ty_adt_def().unwrap().variants.len(),
);
if variant_index != dataful_variant {
let niche_dest =
self.place_field(dest, discr_index as u64)?;
let niche_value = variant_index.as_u32() - niche_variants.start().as_u32();
let niche_value = (niche_value as u128)
.wrapping_add(niche_start);
self.write_scalar(
Scalar::from_uint(niche_value, niche_dest.layout.size),
niche_dest
let variants_start = niche_variants.start().as_u32();
let variant_index_relative = variant_index.as_u32()
.checked_sub(variants_start)
.expect("overflow computing relative variant idx");
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine because this is a "niche" variant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure but is there any reason not to ICE if this overflows anyway?

// We need to use machine arithmetic when taking into account `niche_start`.
let discr_layout = self.layout_of(discr_layout.value.to_int_ty(*self.tcx))?;
let niche_start_val = ImmTy::from_uint(niche_start, discr_layout);
let variant_index_relative_val =
ImmTy::from_uint(variant_index_relative, discr_layout);
let discr_val = self.binary_op(
mir::BinOp::Add,
variant_index_relative_val,
niche_start_val,
)?;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how much this matters - you just need to truncate the result, I think? Either way, the symmetry with reading is probably fine.

// Write result.
let niche_dest = self.place_field(dest, discr_index as u64)?;
self.write_immediate(*discr_val, niche_dest)?;
}
}
}
Expand Down
110 changes: 110 additions & 0 deletions src/test/ui/consts/miri_unleashed/enum_discriminants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// compile-flags: -Zunleash-the-miri-inside-of-you
// run-pass

//! Make sure that we read and write enum discriminants correctly for corner cases caused
//! by layout optimizations.

const OVERFLOW: usize = {
// Tests for https://github.com/rust-lang/rust/issues/62138.
#[repr(u8)]
#[allow(dead_code)]
enum WithWraparoundInvalidValues {
X = 1,
Y = 254,
}

#[allow(dead_code)]
enum Foo {
A,
B,
C(WithWraparoundInvalidValues),
}

let x = Foo::B;
match x {
Foo::B => 0,
_ => panic!(),
}
};

const MORE_OVERFLOW: usize = {
pub enum Infallible {}

// The check that the `bool` field of `V1` is encoding a "niche variant"
// (i.e. not `V1`, so `V3` or `V4`) used to be mathematically incorrect,
// causing valid `V1` values to be interpreted as other variants.
#[allow(dead_code)]
pub enum E1 {
V1 { f: bool },
V2 { f: Infallible },
V3,
V4,
}

// Computing the discriminant used to be done using the niche type (here `u8`,
// from the `bool` field of `V1`), overflowing for variants with large enough
// indices (`V3` and `V4`), causing them to be interpreted as other variants.
#[allow(dead_code)]
pub enum E2<X> {
V1 { f: bool },

/*_00*/ _01(X), _02(X), _03(X), _04(X), _05(X), _06(X), _07(X),
_08(X), _09(X), _0A(X), _0B(X), _0C(X), _0D(X), _0E(X), _0F(X),
_10(X), _11(X), _12(X), _13(X), _14(X), _15(X), _16(X), _17(X),
_18(X), _19(X), _1A(X), _1B(X), _1C(X), _1D(X), _1E(X), _1F(X),
_20(X), _21(X), _22(X), _23(X), _24(X), _25(X), _26(X), _27(X),
_28(X), _29(X), _2A(X), _2B(X), _2C(X), _2D(X), _2E(X), _2F(X),
_30(X), _31(X), _32(X), _33(X), _34(X), _35(X), _36(X), _37(X),
_38(X), _39(X), _3A(X), _3B(X), _3C(X), _3D(X), _3E(X), _3F(X),
_40(X), _41(X), _42(X), _43(X), _44(X), _45(X), _46(X), _47(X),
_48(X), _49(X), _4A(X), _4B(X), _4C(X), _4D(X), _4E(X), _4F(X),
_50(X), _51(X), _52(X), _53(X), _54(X), _55(X), _56(X), _57(X),
_58(X), _59(X), _5A(X), _5B(X), _5C(X), _5D(X), _5E(X), _5F(X),
_60(X), _61(X), _62(X), _63(X), _64(X), _65(X), _66(X), _67(X),
_68(X), _69(X), _6A(X), _6B(X), _6C(X), _6D(X), _6E(X), _6F(X),
_70(X), _71(X), _72(X), _73(X), _74(X), _75(X), _76(X), _77(X),
_78(X), _79(X), _7A(X), _7B(X), _7C(X), _7D(X), _7E(X), _7F(X),
_80(X), _81(X), _82(X), _83(X), _84(X), _85(X), _86(X), _87(X),
_88(X), _89(X), _8A(X), _8B(X), _8C(X), _8D(X), _8E(X), _8F(X),
_90(X), _91(X), _92(X), _93(X), _94(X), _95(X), _96(X), _97(X),
_98(X), _99(X), _9A(X), _9B(X), _9C(X), _9D(X), _9E(X), _9F(X),
_A0(X), _A1(X), _A2(X), _A3(X), _A4(X), _A5(X), _A6(X), _A7(X),
_A8(X), _A9(X), _AA(X), _AB(X), _AC(X), _AD(X), _AE(X), _AF(X),
_B0(X), _B1(X), _B2(X), _B3(X), _B4(X), _B5(X), _B6(X), _B7(X),
_B8(X), _B9(X), _BA(X), _BB(X), _BC(X), _BD(X), _BE(X), _BF(X),
_C0(X), _C1(X), _C2(X), _C3(X), _C4(X), _C5(X), _C6(X), _C7(X),
_C8(X), _C9(X), _CA(X), _CB(X), _CC(X), _CD(X), _CE(X), _CF(X),
_D0(X), _D1(X), _D2(X), _D3(X), _D4(X), _D5(X), _D6(X), _D7(X),
_D8(X), _D9(X), _DA(X), _DB(X), _DC(X), _DD(X), _DE(X), _DF(X),
_E0(X), _E1(X), _E2(X), _E3(X), _E4(X), _E5(X), _E6(X), _E7(X),
_E8(X), _E9(X), _EA(X), _EB(X), _EC(X), _ED(X), _EE(X), _EF(X),
_F0(X), _F1(X), _F2(X), _F3(X), _F4(X), _F5(X), _F6(X), _F7(X),
_F8(X), _F9(X), _FA(X), _FB(X), _FC(X), _FD(X), _FE(X), _FF(X),

V3,
V4,
}

if let E1::V2 { .. } = (E1::V1 { f: true }) {
unreachable!()
}
if let E1::V1 { .. } = (E1::V1 { f: true }) {
} else {
unreachable!()
}

if let E2::V1 { .. } = E2::V3::<Infallible> {
unreachable!()
}
if let E2::V3 { .. } = E2::V3::<Infallible> {
} else {
unreachable!()
}

0
};

fn main() {
assert_eq!(OVERFLOW, 0);
assert_eq!(MORE_OVERFLOW, 0);
}
72 changes: 72 additions & 0 deletions src/test/ui/consts/miri_unleashed/enum_discriminants.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
warning: skipping const checks
--> $DIR/enum_discriminants.rs:23:13
|
LL | let x = Foo::B;
| ^^^^^^

warning: skipping const checks
--> $DIR/enum_discriminants.rs:25:9
|
LL | Foo::B => 0,
| ^^^^^^

warning: skipping const checks
--> $DIR/enum_discriminants.rs:88:28
|
LL | if let E1::V2 { .. } = (E1::V1 { f: true }) {
| ^^^^^^^^^^^^^^^^^^^^

warning: skipping const checks
--> $DIR/enum_discriminants.rs:88:12
|
LL | if let E1::V2 { .. } = (E1::V1 { f: true }) {
| ^^^^^^^^^^^^^

warning: skipping const checks
--> $DIR/enum_discriminants.rs:108:5
|
LL | assert_eq!(OVERFLOW, 0);
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: skipping const checks
--> $DIR/enum_discriminants.rs:108:5
|
LL | assert_eq!(OVERFLOW, 0);
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: skipping const checks
--> $DIR/enum_discriminants.rs:108:5
|
LL | assert_eq!(OVERFLOW, 0);
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: skipping const checks
--> $DIR/enum_discriminants.rs:109:5
|
LL | assert_eq!(MORE_OVERFLOW, 0);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: skipping const checks
--> $DIR/enum_discriminants.rs:109:5
|
LL | assert_eq!(MORE_OVERFLOW, 0);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: skipping const checks
--> $DIR/enum_discriminants.rs:109:5
|
LL | assert_eq!(MORE_OVERFLOW, 0);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)