From 8293fe9fdf2d3398176297b82a72cd32d13538d8 Mon Sep 17 00:00:00 2001 From: Alexis Beingessner Date: Mon, 20 Nov 2017 12:26:13 -0500 Subject: [PATCH 1/4] Implement the special repr(C)-non-clike-enum layout --- src/librustc/ty/layout.rs | 42 ++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 71bf333a8c6..7d262adffdb 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -942,8 +942,8 @@ impl<'a, 'tcx> LayoutDetails { AlwaysSized, /// A univariant, the last field of which may be coerced to unsized. MaybeUnsized, - /// A univariant, but part of an enum. - EnumVariant(Integer), + /// A univariant, but with a prefix of an arbitrary size & alignment (e.g. enum tag). + Prefixed(Size, Align), } let univariant_uninterned = |fields: &[TyLayout], repr: &ReprOptions, kind| { let packed = repr.packed(); @@ -962,14 +962,11 @@ impl<'a, 'tcx> LayoutDetails { let mut inverse_memory_index: Vec = (0..fields.len() as u32).collect(); // Anything with repr(C) or repr(packed) doesn't optimize. - let optimize = match kind { - StructKind::AlwaysSized | - StructKind::MaybeUnsized | - StructKind::EnumVariant(I8) => { - (repr.flags & ReprFlags::IS_UNOPTIMISABLE).is_empty() - } - StructKind::EnumVariant(_) => false - }; + let mut optimize = (repr.flags & ReprFlags::IS_UNOPTIMISABLE).is_empty(); + if let StructKind::Prefixed(_, align) = kind { + optimize &= align.abi() == 1; + } + if optimize { let end = if let StructKind::MaybeUnsized = kind { fields.len() - 1 @@ -987,7 +984,7 @@ impl<'a, 'tcx> LayoutDetails { (!f.is_zst(), cmp::Reverse(f.align.abi())) }) } - StructKind::EnumVariant(_) => { + StructKind::Prefixed(..) => { optimizing.sort_by_key(|&x| fields[x as usize].align.abi()); } } @@ -1001,12 +998,11 @@ impl<'a, 'tcx> LayoutDetails { let mut offset = Size::from_bytes(0); - if let StructKind::EnumVariant(discr) = kind { - offset = discr.size(); + if let StructKind::Prefixed(prefix_size, prefix_align) = kind { if !packed { - let discr_align = discr.align(dl); - align = align.max(discr_align); + align = align.max(prefix_align); } + offset = prefix_size.abi_align(prefix_align); } for &i in &inverse_memory_index { @@ -1558,10 +1554,24 @@ impl<'a, 'tcx> LayoutDetails { let mut start_align = Align::from_bytes(256, 256).unwrap(); assert_eq!(Integer::for_abi_align(dl, start_align), None); + // repr(C) on an enum tells us to make a (tag, union) layout, + // so we need to grow the prefix alignment to be at least + // the alignment of the union. (This value is used both for + // determining the alignment of the overall enum, and the + // determining the alignment of the payload after the tag.) + let mut prefix_align = min_ity.align(dl); + if def.repr.c() { + for fields in &variants { + for field in fields { + prefix_align = prefix_align.max(field.align); + } + } + } + // Create the set of structs that represent each variant. let mut variants = variants.into_iter().enumerate().map(|(i, field_layouts)| { let mut st = univariant_uninterned(&field_layouts, - &def.repr, StructKind::EnumVariant(min_ity))?; + &def.repr, StructKind::Prefixed(min_ity.size(), prefix_align))?; st.variants = Variants::Single { index: i }; // Find the first field we can't move later // to make room for a larger discriminant. From 3d7a6fee79a7a42ac69b185806733dbbd1fc664e Mon Sep 17 00:00:00 2001 From: Alexis Beingessner Date: Mon, 20 Nov 2017 12:26:54 -0500 Subject: [PATCH 2/4] Prevent repr(C, u8) from triggering a warning on non-clike enums --- src/librustc/hir/check_attr.rs | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/librustc/hir/check_attr.rs b/src/librustc/hir/check_attr.rs index 05c371113b4..003255f8796 100644 --- a/src/librustc/hir/check_attr.rs +++ b/src/librustc/hir/check_attr.rs @@ -75,7 +75,9 @@ impl<'a> CheckAttrVisitor<'a> { } }; - let mut conflicting_reprs = 0; + let mut int_reprs = 0; + let mut is_c = false; + let mut is_simd = false; for word in words { @@ -86,7 +88,7 @@ impl<'a> CheckAttrVisitor<'a> { let (message, label) = match &*name.as_str() { "C" => { - conflicting_reprs += 1; + is_c = true; if target != Target::Struct && target != Target::Union && target != Target::Enum { @@ -108,7 +110,7 @@ impl<'a> CheckAttrVisitor<'a> { } } "simd" => { - conflicting_reprs += 1; + is_simd = true; if target != Target::Struct { ("attribute should be applied to struct", "a struct") @@ -128,7 +130,7 @@ impl<'a> CheckAttrVisitor<'a> { "i8" | "u8" | "i16" | "u16" | "i32" | "u32" | "i64" | "u64" | "isize" | "usize" => { - conflicting_reprs += 1; + int_reprs += 1; if target != Target::Enum { ("attribute should be applied to enum", "an enum") @@ -142,7 +144,11 @@ impl<'a> CheckAttrVisitor<'a> { .span_label(item.span, format!("not {}", label)) .emit(); } - if conflicting_reprs > 1 { + + // Warn on repr(u8, u16), repr(C, simd), and c-like-enum-repr(C, u8) + if (int_reprs > 1) + || (is_simd && is_c) + || (int_reprs == 1 && is_c && is_c_like_enum(item)) { span_warn!(self.sess, attr.span, E0566, "conflicting representation hints"); } @@ -162,3 +168,17 @@ impl<'a> Visitor<'a> for CheckAttrVisitor<'a> { pub fn check_crate(sess: &Session, krate: &ast::Crate) { visit::walk_crate(&mut CheckAttrVisitor { sess: sess }, krate); } + +fn is_c_like_enum(item: &ast::Item) -> bool { + if let ast::ItemKind::Enum(ref def, _) = item.node { + for variant in &def.variants { + match variant.node.data { + ast::VariantData::Unit(_) => { /* continue */ } + _ => { return false; } + } + } + true + } else { + false + } +} From 7e79b9bbf5e16d3675fc3e14864779ad32f33059 Mon Sep 17 00:00:00 2001 From: Alexis Beingessner Date: Mon, 20 Nov 2017 12:27:53 -0500 Subject: [PATCH 3/4] Add tests for repr(C)-non-clike-enum layout --- .../enum-non-c-like-repr-c-and-int.rs | 177 ++++++++++++++++++ src/test/run-pass/enum-non-c-like-repr-c.rs | 177 ++++++++++++++++++ 2 files changed, 354 insertions(+) create mode 100644 src/test/run-pass/enum-non-c-like-repr-c-and-int.rs create mode 100644 src/test/run-pass/enum-non-c-like-repr-c.rs diff --git a/src/test/run-pass/enum-non-c-like-repr-c-and-int.rs b/src/test/run-pass/enum-non-c-like-repr-c-and-int.rs new file mode 100644 index 00000000000..86453fdf6fa --- /dev/null +++ b/src/test/run-pass/enum-non-c-like-repr-c-and-int.rs @@ -0,0 +1,177 @@ +// Copyright 2012 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. + +// This test deserializes an enum in-place by transmuting to a union that +// should have the same layout, and manipulating the tag and payloads +// independently. This verifies that `repr(some_int)` has a stable representation, +// and that we don't miscompile these kinds of manipulations. + +use std::time::Duration; +use std::mem; + +#[repr(C, u8)] +#[derive(Copy, Clone, Eq, PartialEq, Debug)] +enum MyEnum { + A(u32), // Single primitive value + B { x: u8, y: i16 }, // Composite, and the offset of `y` depends on tag being internal + C, // Empty + D(Option), // Contains an enum + E(Duration), // Contains a struct +} + +#[repr(C)] +struct MyEnumRepr { + tag: MyEnumTag, + payload: MyEnumPayload, +} + +#[repr(C)] +#[allow(non_snake_case)] +union MyEnumPayload { + A: MyEnumVariantA, + B: MyEnumVariantB, + D: MyEnumVariantD, + E: MyEnumVariantE, +} + +#[repr(u8)] #[derive(Copy, Clone)] enum MyEnumTag { A, B, C, D, E } +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantA(u32); +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantB {x: u8, y: i16 } +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantD(Option); +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantE(Duration); + +fn main() { + let result: Vec> = vec![ + Ok(MyEnum::A(17)), + Ok(MyEnum::B { x: 206, y: 1145 }), + Ok(MyEnum::C), + Err(()), + Ok(MyEnum::D(Some(407))), + Ok(MyEnum::D(None)), + Ok(MyEnum::E(Duration::from_secs(100))), + Err(()), + ]; + + // Binary serialized version of the above (little-endian) + let input: Vec = vec![ + 0, 17, 0, 0, 0, + 1, 206, 121, 4, + 2, + 8, /* invalid tag value */ + 3, 0, 151, 1, 0, 0, + 3, 1, + 4, 100, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, /* incomplete value */ + ]; + + let mut output = vec![]; + let mut buf = &input[..]; + + unsafe { + // This should be safe, because we don't match on it unless it's fully formed, + // and it doesn't have a destructor. + let mut dest: MyEnum = mem::uninitialized(); + while buf.len() > 0 { + match parse_my_enum(&mut dest, &mut buf) { + Ok(()) => output.push(Ok(dest)), + Err(()) => output.push(Err(())), + } + } + } + + assert_eq!(output, result); +} + +fn parse_my_enum<'a>(dest: &'a mut MyEnum, buf: &mut &[u8]) -> Result<(), ()> { + unsafe { + // Should be correct to do this transmute. + let dest: &'a mut MyEnumRepr = mem::transmute(dest); + let tag = read_u8(buf)?; + + dest.tag = match tag { + 0 => MyEnumTag::A, + 1 => MyEnumTag::B, + 2 => MyEnumTag::C, + 3 => MyEnumTag::D, + 4 => MyEnumTag::E, + _ => return Err(()), + }; + + match dest.tag { + MyEnumTag::A => { + dest.payload.A.0 = read_u32_le(buf)?; + } + MyEnumTag::B => { + dest.payload.B.x = read_u8(buf)?; + dest.payload.B.y = read_u16_le(buf)? as i16; + } + MyEnumTag::C => { + /* do nothing */ + } + MyEnumTag::D => { + let is_some = read_u8(buf)? == 0; + if is_some { + dest.payload.D.0 = Some(read_u32_le(buf)?); + } else { + dest.payload.D.0 = None; + } + } + MyEnumTag::E => { + let secs = read_u64_le(buf)?; + let nanos = read_u32_le(buf)?; + dest.payload.E.0 = Duration::new(secs, nanos); + } + } + Ok(()) + } +} + + + +// reader helpers + +fn read_u64_le(buf: &mut &[u8]) -> Result { + if buf.len() < 8 { return Err(()) } + let val = (buf[0] as u64) << 0 + | (buf[1] as u64) << 8 + | (buf[2] as u64) << 16 + | (buf[3] as u64) << 24 + | (buf[4] as u64) << 32 + | (buf[5] as u64) << 40 + | (buf[6] as u64) << 48 + | (buf[7] as u64) << 56; + *buf = &buf[8..]; + Ok(val) +} + +fn read_u32_le(buf: &mut &[u8]) -> Result { + if buf.len() < 4 { return Err(()) } + let val = (buf[0] as u32) << 0 + | (buf[1] as u32) << 8 + | (buf[2] as u32) << 16 + | (buf[3] as u32) << 24; + *buf = &buf[4..]; + Ok(val) +} + +fn read_u16_le(buf: &mut &[u8]) -> Result { + if buf.len() < 2 { return Err(()) } + let val = (buf[0] as u16) << 0 + | (buf[1] as u16) << 8; + *buf = &buf[2..]; + Ok(val) +} + +fn read_u8(buf: &mut &[u8]) -> Result { + if buf.len() < 1 { return Err(()) } + let val = buf[0]; + *buf = &buf[1..]; + Ok(val) +} diff --git a/src/test/run-pass/enum-non-c-like-repr-c.rs b/src/test/run-pass/enum-non-c-like-repr-c.rs new file mode 100644 index 00000000000..b4e0fe8d457 --- /dev/null +++ b/src/test/run-pass/enum-non-c-like-repr-c.rs @@ -0,0 +1,177 @@ +// Copyright 2012 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. + +// This test deserializes an enum in-place by transmuting to a union that +// should have the same layout, and manipulating the tag and payloads +// independently. This verifies that `repr(some_int)` has a stable representation, +// and that we don't miscompile these kinds of manipulations. + +use std::time::Duration; +use std::mem; + +#[repr(C)] +#[derive(Copy, Clone, Eq, PartialEq, Debug)] +enum MyEnum { + A(u32), // Single primitive value + B { x: u8, y: i16 }, // Composite, and the offset of `y` depends on tag being internal + C, // Empty + D(Option), // Contains an enum + E(Duration), // Contains a struct +} + +#[repr(C)] +struct MyEnumRepr { + tag: MyEnumTag, + payload: MyEnumPayload, +} + +#[repr(C)] +#[allow(non_snake_case)] +union MyEnumPayload { + A: MyEnumVariantA, + B: MyEnumVariantB, + D: MyEnumVariantD, + E: MyEnumVariantE, +} + +#[repr(C)] #[derive(Copy, Clone)] enum MyEnumTag { A, B, C, D, E } +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantA(u32); +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantB {x: u8, y: i16 } +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantD(Option); +#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantE(Duration); + +fn main() { + let result: Vec> = vec![ + Ok(MyEnum::A(17)), + Ok(MyEnum::B { x: 206, y: 1145 }), + Ok(MyEnum::C), + Err(()), + Ok(MyEnum::D(Some(407))), + Ok(MyEnum::D(None)), + Ok(MyEnum::E(Duration::from_secs(100))), + Err(()), + ]; + + // Binary serialized version of the above (little-endian) + let input: Vec = vec![ + 0, 17, 0, 0, 0, + 1, 206, 121, 4, + 2, + 8, /* invalid tag value */ + 3, 0, 151, 1, 0, 0, + 3, 1, + 4, 100, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, /* incomplete value */ + ]; + + let mut output = vec![]; + let mut buf = &input[..]; + + unsafe { + // This should be safe, because we don't match on it unless it's fully formed, + // and it doesn't have a destructor. + let mut dest: MyEnum = mem::uninitialized(); + while buf.len() > 0 { + match parse_my_enum(&mut dest, &mut buf) { + Ok(()) => output.push(Ok(dest)), + Err(()) => output.push(Err(())), + } + } + } + + assert_eq!(output, result); +} + +fn parse_my_enum<'a>(dest: &'a mut MyEnum, buf: &mut &[u8]) -> Result<(), ()> { + unsafe { + // Should be correct to do this transmute. + let dest: &'a mut MyEnumRepr = mem::transmute(dest); + let tag = read_u8(buf)?; + + dest.tag = match tag { + 0 => MyEnumTag::A, + 1 => MyEnumTag::B, + 2 => MyEnumTag::C, + 3 => MyEnumTag::D, + 4 => MyEnumTag::E, + _ => return Err(()), + }; + + match dest.tag { + MyEnumTag::A => { + dest.payload.A.0 = read_u32_le(buf)?; + } + MyEnumTag::B => { + dest.payload.B.x = read_u8(buf)?; + dest.payload.B.y = read_u16_le(buf)? as i16; + } + MyEnumTag::C => { + /* do nothing */ + } + MyEnumTag::D => { + let is_some = read_u8(buf)? == 0; + if is_some { + dest.payload.D.0 = Some(read_u32_le(buf)?); + } else { + dest.payload.D.0 = None; + } + } + MyEnumTag::E => { + let secs = read_u64_le(buf)?; + let nanos = read_u32_le(buf)?; + dest.payload.E.0 = Duration::new(secs, nanos); + } + } + Ok(()) + } +} + + + +// reader helpers + +fn read_u64_le(buf: &mut &[u8]) -> Result { + if buf.len() < 8 { return Err(()) } + let val = (buf[0] as u64) << 0 + | (buf[1] as u64) << 8 + | (buf[2] as u64) << 16 + | (buf[3] as u64) << 24 + | (buf[4] as u64) << 32 + | (buf[5] as u64) << 40 + | (buf[6] as u64) << 48 + | (buf[7] as u64) << 56; + *buf = &buf[8..]; + Ok(val) +} + +fn read_u32_le(buf: &mut &[u8]) -> Result { + if buf.len() < 4 { return Err(()) } + let val = (buf[0] as u32) << 0 + | (buf[1] as u32) << 8 + | (buf[2] as u32) << 16 + | (buf[3] as u32) << 24; + *buf = &buf[4..]; + Ok(val) +} + +fn read_u16_le(buf: &mut &[u8]) -> Result { + if buf.len() < 2 { return Err(()) } + let val = (buf[0] as u16) << 0 + | (buf[1] as u16) << 8; + *buf = &buf[2..]; + Ok(val) +} + +fn read_u8(buf: &mut &[u8]) -> Result { + if buf.len() < 1 { return Err(()) } + let val = buf[0]; + *buf = &buf[1..]; + Ok(val) +} From 0e63d2727c3215fab617a64c0a159ea871f4819c Mon Sep 17 00:00:00 2001 From: Alexis Beingessner Date: Mon, 20 Nov 2017 14:22:33 -0500 Subject: [PATCH 4/4] Fix and improve test for enum repr sizes --- src/test/run-pass/multiple-reprs.rs | 52 +++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/src/test/run-pass/multiple-reprs.rs b/src/test/run-pass/multiple-reprs.rs index c2fe943eed8..d8eafb806f7 100644 --- a/src/test/run-pass/multiple-reprs.rs +++ b/src/test/run-pass/multiple-reprs.rs @@ -9,7 +9,8 @@ // except according to those terms. -use std::mem::size_of; +use std::mem::{size_of, align_of}; +use std::os::raw::c_int; // The two enums that follow are designed so that bugs trigger layout optimization. // Specifically, if either of the following reprs used here is not detected by the compiler, @@ -27,6 +28,38 @@ enum E2 { B(u8, u16, u8) } +// Check that repr(int) and repr(C) are in fact different from the above + +#[repr(u8)] +enum E3 { + A(u8, u16, u8), + B(u8, u16, u8) +} + +#[repr(u16)] +enum E4 { + A(u8, u16, u8), + B(u8, u16, u8) +} + +#[repr(u32)] +enum E5 { + A(u8, u16, u8), + B(u8, u16, u8) +} + +#[repr(u64)] +enum E6 { + A(u8, u16, u8), + B(u8, u16, u8) +} + +#[repr(C)] +enum E7 { + A(u8, u16, u8), + B(u8, u16, u8) +} + // From pr 37429 #[repr(C,packed)] @@ -37,7 +70,20 @@ pub struct p0f_api_query { } pub fn main() { - assert_eq!(size_of::(), 6); - assert_eq!(size_of::(), 6); + assert_eq!(size_of::(), 8); + assert_eq!(size_of::(), 8); + assert_eq!(size_of::(), 6); + assert_eq!(size_of::(), 8); + assert_eq!(size_of::(), align_size(10, align_of::())); + assert_eq!(size_of::(), align_size(14, align_of::())); + assert_eq!(size_of::(), align_size(6 + size_of::(), align_of::())); assert_eq!(size_of::(), 21); } + +fn align_size(size: usize, align: usize) -> usize { + if size % align != 0 { + size + (align - (size % align)) + } else { + size + } +}