Skip to content

Fix an ICE with unsized structs and optimised enums #18591

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

Merged
merged 2 commits into from
Nov 7, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 19 additions & 26 deletions src/librustc/middle/trans/adt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use std::num::Int;
use std::rc::Rc;

use llvm::{ValueRef, True, IntEQ, IntNE};
use back::abi::slice_elt_base;
use middle::subst;
use middle::subst::Subst;
use middle::trans::_match;
Expand Down Expand Up @@ -235,7 +236,7 @@ fn represent_type_uncached(cx: &CrateContext, t: ty::t) -> Repr {
if cases[1 - discr].is_zerolen(cx, t) {
let st = mk_struct(cx, cases[discr].tys.as_slice(),
false, t);
match cases[discr].find_ptr() {
match cases[discr].find_ptr(cx) {
Some(ThinPointer(_)) if st.fields.len() == 1 => {
return RawNullablePointer {
nndiscr: discr as Disr,
Expand Down Expand Up @@ -290,47 +291,38 @@ struct Case {
#[deriving(Eq, PartialEq, Show)]
pub enum PointerField {
ThinPointer(uint),
FatPointer(uint, uint)
FatPointer(uint)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a reasonable cleanup, but slightly confusing in that it uses slice_elt_base everywhere and silently depends on it being equal to trt_field_box. I would suggest merging the two constants into something like dst_fat_base or similar.

EDIT: I just noticed you added a comment about this which I hadn't read yet. I'll leave it up to you whether to bother fixing this now or later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I deliberately left it as a follow up, because I know we make the same assumption all over the place, so fixing isn't super-urgent.

}

impl Case {
fn is_zerolen(&self, cx: &CrateContext, scapegoat: ty::t) -> bool {
mk_struct(cx, self.tys.as_slice(), false, scapegoat).size == 0
}

fn find_ptr(&self) -> Option<PointerField> {
use back::abi::{fn_field_code, slice_elt_base, trt_field_box};

fn find_ptr(&self, cx: &CrateContext) -> Option<PointerField> {
for (i, &ty) in self.tys.iter().enumerate() {
match ty::get(ty).sty {
// &T/&mut T could either be a thin or fat pointer depending on T
ty::ty_rptr(_, ty::mt { ty, .. }) => match ty::get(ty).sty {
// &T/&mut T/Box<T> could either be a thin or fat pointer depending on T
ty::ty_rptr(_, ty::mt { ty, .. }) | ty::ty_uniq(ty) => match ty::get(ty).sty {
// &[T] and &str are a pointer and length pair
ty::ty_vec(_, None) | ty::ty_str => return Some(FatPointer(i, slice_elt_base)),

// &Trait/&mut Trait are a pair of pointers: the actual object and a vtable
ty::ty_trait(..) => return Some(FatPointer(i, trt_field_box)),

// Any other &T/&mut T is just a pointer
_ => return Some(ThinPointer(i))
},
ty::ty_vec(_, None) | ty::ty_str => return Some(FatPointer(i)),

// Box<T> could either be a thin or fat pointer depending on T
ty::ty_uniq(t) => match ty::get(t).sty {
ty::ty_vec(_, None) => return Some(FatPointer(i, slice_elt_base)),
// &Trait is a pair of pointers: the actual object and a vtable
ty::ty_trait(..) => return Some(FatPointer(i)),

// Box<Trait> is a pair of pointers: the actual object and a vtable
ty::ty_trait(..) => return Some(FatPointer(i, trt_field_box)),
ty::ty_struct(..) if !ty::type_is_sized(cx.tcx(), ty) => {
return Some(FatPointer(i))
}

// Any other Box<T> is just a pointer
// Any other &T is just a pointer
_ => return Some(ThinPointer(i))
},

// Functions are just pointers
ty::ty_bare_fn(..) => return Some(ThinPointer(i)),

// Closures are a pair of pointers: the code and environment
ty::ty_closure(..) => return Some(FatPointer(i, fn_field_code)),
ty::ty_closure(..) => return Some(FatPointer(i)),

// Anything else is not a pointer
_ => continue
Expand Down Expand Up @@ -636,6 +628,7 @@ pub fn trans_get_discr(bcx: Block, r: &Repr, scrutinee: ValueRef, cast_to: Optio
-> ValueRef {
let signed;
let val;
debug!("trans_get_discr r: {}", r);
match *r {
CEnum(ity, min, max) => {
val = load_discr(bcx, ity, scrutinee, min, max);
Expand Down Expand Up @@ -671,7 +664,7 @@ fn struct_wrapped_nullable_bitdiscr(bcx: Block, nndiscr: Disr, ptrfield: Pointer
scrutinee: ValueRef) -> ValueRef {
let llptrptr = match ptrfield {
ThinPointer(field) => GEPi(bcx, scrutinee, [0, field]),
FatPointer(field, pair) => GEPi(bcx, scrutinee, [0, field, pair])
FatPointer(field) => GEPi(bcx, scrutinee, [0, field, slice_elt_base])
};
let llptr = Load(bcx, llptrptr);
let cmp = if nndiscr == 0 { IntEQ } else { IntNE };
Expand Down Expand Up @@ -767,8 +760,8 @@ pub fn trans_set_discr(bcx: Block, r: &Repr, val: ValueRef, discr: Disr) {
ThinPointer(field) =>
(GEPi(bcx, val, [0, field]),
type_of::type_of(bcx.ccx(), nonnull.fields[field])),
FatPointer(field, pair) => {
let v = GEPi(bcx, val, [0, field, pair]);
FatPointer(field) => {
let v = GEPi(bcx, val, [0, field, slice_elt_base]);
(v, val_ty(v).element_type())
}
};
Expand Down Expand Up @@ -1102,7 +1095,7 @@ pub fn const_get_discrim(ccx: &CrateContext, r: &Repr, val: ValueRef)
StructWrappedNullablePointer { nndiscr, ptrfield, .. } => {
let (idx, sub_idx) = match ptrfield {
ThinPointer(field) => (field, None),
FatPointer(field, pair) => (field, Some(pair))
FatPointer(field) => (field, Some(slice_elt_base))
};
if is_null(const_struct_field(ccx, val, idx, sub_idx)) {
/* subtraction as uint is ok because nndiscr is either 0 or 1 */
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/trans/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2255,7 +2255,7 @@ impl EnumMemberDescriptionFactory {
let null_variant_name = token::get_name((*self.variants)[null_variant_index].name);
let discrfield = match ptrfield {
adt::ThinPointer(field) => format!("{}", field),
adt::FatPointer(field, pair) => format!("{}${}", field, pair)
adt::FatPointer(field) => format!("{}", field)
};
let union_member_name = format!("RUST$ENCODED$ENUM${}${}",
discrfield,
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_back/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ pub const box_field_refcnt: uint = 0u;
pub const box_field_drop_glue: uint = 1u;
pub const box_field_body: uint = 4u;

// FIXME(18590) although we have three different layouts here, the compiler relies on
// them being the same. We should replace them with one set of constants.

// The two halves of a closure: code and environment.
pub const fn_field_code: uint = 0u;
pub const fn_field_box: uint = 1u;
Expand Down
21 changes: 21 additions & 0 deletions src/test/run-pass/issue-18353.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2014 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.

// Test that wrapping an unsized struct in an enum which gets optimised does
// not ICE.

struct Str {
f: [u8]
}

fn main() {
let str: Option<&Str> = None;
str.is_some();
}