From dee8a71cd5221536c319ca8c14108e93521092f5 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 2 Oct 2017 15:15:23 +0200 Subject: [PATCH] fix #[derive] implementation for repr(packed) structs Fix the derive implementation for repr(packed) structs to move the fields out instead of calling functions on references to each subfield. That's it, `#[derive(PartialEq)]` on a packed struct now does: ```Rust fn eq(&self, other: &Self) { let field_0 = self.0; let other_field_0 = other.0; &field_0 == &other_field_0 } ``` Instead of ```Rust fn eq(&self, other: &Self) { let ref field_0 = self.0; let ref other_field_0 = other.0; &*field_0 == &*other_field_0 } ``` Taking (unaligned) references to each subfield is undefined, unsound and is an error with MIR effectck, so it had to be prevented. This causes a borrowck error when a `repr(packed)` struct has a non-Copy field (and therefore is a [breaking-change]), but I don't see a sound way to avoid that error. --- src/libsyntax_ext/deriving/generic/mod.rs | 80 +++++++++++++++---- src/libsyntax_pos/span_encoding.rs | 16 +--- .../deriving-with-repr-packed-not-copy.rs | 28 +++++++ .../run-pass/deriving-with-repr-packed.rs | 45 +++++++++++ 4 files changed, 138 insertions(+), 31 deletions(-) create mode 100644 src/test/compile-fail/deriving-with-repr-packed-not-copy.rs create mode 100644 src/test/run-pass/deriving-with-repr-packed.rs diff --git a/src/libsyntax_ext/deriving/generic/mod.rs b/src/libsyntax_ext/deriving/generic/mod.rs index 18897047538f..5abf524313ac 100644 --- a/src/libsyntax_ext/deriving/generic/mod.rs +++ b/src/libsyntax_ext/deriving/generic/mod.rs @@ -393,7 +393,7 @@ fn find_type_parameters(ty: &ast::Ty, } impl<'a> TraitDef<'a> { - pub fn expand(&self, + pub fn expand(self, cx: &mut ExtCtxt, mitem: &ast::MetaItem, item: &'a Annotatable, @@ -401,7 +401,7 @@ impl<'a> TraitDef<'a> { self.expand_ext(cx, mitem, item, push, false); } - pub fn expand_ext(&self, + pub fn expand_ext(self, cx: &mut ExtCtxt, mitem: &ast::MetaItem, item: &'a Annotatable, @@ -409,18 +409,31 @@ impl<'a> TraitDef<'a> { from_scratch: bool) { match *item { Annotatable::Item(ref item) => { + let is_packed = item.attrs.iter().any(|attr| { + attr::find_repr_attrs(&cx.parse_sess.span_diagnostic, attr) + .contains(&attr::ReprPacked) + }); + let use_temporaries = is_packed; let newitem = match item.node { ast::ItemKind::Struct(ref struct_def, ref generics) => { - self.expand_struct_def(cx, &struct_def, item.ident, generics, from_scratch) + self.expand_struct_def(cx, &struct_def, item.ident, generics, from_scratch, + use_temporaries) } ast::ItemKind::Enum(ref enum_def, ref generics) => { + // We ignore `use_temporaries` here, because + // `repr(packed)` enums cause an error later on. + // + // This can only cause further compilation errors + // downstream in blatantly illegal code, so it + // is fine. self.expand_enum_def(cx, enum_def, &item.attrs, item.ident, generics, from_scratch) } ast::ItemKind::Union(ref struct_def, ref generics) => { if self.supports_unions { self.expand_struct_def(cx, &struct_def, item.ident, - generics, from_scratch) + generics, from_scratch, + use_temporaries) } else { cx.span_err(mitem.span, "this trait cannot be derived for unions"); @@ -675,7 +688,8 @@ impl<'a> TraitDef<'a> { struct_def: &'a VariantData, type_ident: Ident, generics: &Generics, - from_scratch: bool) + from_scratch: bool, + use_temporaries: bool) -> P { let field_tys: Vec> = struct_def.fields() .iter() @@ -701,7 +715,8 @@ impl<'a> TraitDef<'a> { struct_def, type_ident, &self_args[..], - &nonself_args[..]) + &nonself_args[..], + use_temporaries) }; method_def.create_method(cx, @@ -958,6 +973,22 @@ impl<'a> MethodDef<'a> { /// } /// } /// } + /// + /// // or if A is repr(packed) - note fields are matched by-value + /// // instead of by-reference. + /// impl PartialEq for A { + /// fn eq(&self, __arg_1: &A) -> bool { + /// match *self { + /// A {x: __self_0_0, y: __self_0_1} => { + /// match __arg_1 { + /// A {x: __self_1_0, y: __self_1_1} => { + /// __self_0_0.eq(&__self_1_0) && __self_0_1.eq(&__self_1_1) + /// } + /// } + /// } + /// } + /// } + /// } /// ``` fn expand_struct_method_body<'b>(&self, cx: &mut ExtCtxt, @@ -965,7 +996,8 @@ impl<'a> MethodDef<'a> { struct_def: &'b VariantData, type_ident: Ident, self_args: &[P], - nonself_args: &[P]) + nonself_args: &[P], + use_temporaries: bool) -> P { let mut raw_fields = Vec::new(); // Vec<[fields of self], @@ -977,7 +1009,8 @@ impl<'a> MethodDef<'a> { struct_path, struct_def, &format!("__self_{}", i), - ast::Mutability::Immutable); + ast::Mutability::Immutable, + use_temporaries); patterns.push(pat); raw_fields.push(ident_expr); } @@ -1140,7 +1173,6 @@ impl<'a> MethodDef<'a> { self_args: Vec>, nonself_args: &[P]) -> P { - let sp = trait_.span; let variants = &enum_def.variants; @@ -1512,12 +1544,18 @@ impl<'a> TraitDef<'a> { fn create_subpatterns(&self, cx: &mut ExtCtxt, field_paths: Vec, - mutbl: ast::Mutability) + mutbl: ast::Mutability, + use_temporaries: bool) -> Vec> { field_paths.iter() .map(|path| { + let binding_mode = if use_temporaries { + ast::BindingMode::ByValue(ast::Mutability::Immutable) + } else { + ast::BindingMode::ByRef(mutbl) + }; cx.pat(path.span, - PatKind::Ident(ast::BindingMode::ByRef(mutbl), (*path).clone(), None)) + PatKind::Ident(binding_mode, (*path).clone(), None)) }) .collect() } @@ -1528,8 +1566,10 @@ impl<'a> TraitDef<'a> { struct_path: ast::Path, struct_def: &'a VariantData, prefix: &str, - mutbl: ast::Mutability) - -> (P, Vec<(Span, Option, P, &'a [ast::Attribute])>) { + mutbl: ast::Mutability, + use_temporaries: bool) + -> (P, Vec<(Span, Option, P, &'a [ast::Attribute])>) + { let mut paths = Vec::new(); let mut ident_exprs = Vec::new(); for (i, struct_field) in struct_def.fields().iter().enumerate() { @@ -1539,12 +1579,18 @@ impl<'a> TraitDef<'a> { span: sp, node: ident, }); - let val = cx.expr_deref(sp, cx.expr_path(cx.path_ident(sp, ident))); + let val = cx.expr_path(cx.path_ident(sp, ident)); + let val = if use_temporaries { + val + } else { + cx.expr_deref(sp, val) + }; let val = cx.expr(sp, ast::ExprKind::Paren(val)); + ident_exprs.push((sp, struct_field.ident, val, &struct_field.attrs[..])); } - let subpats = self.create_subpatterns(cx, paths, mutbl); + let subpats = self.create_subpatterns(cx, paths, mutbl, use_temporaries); let pattern = match *struct_def { VariantData::Struct(..) => { let field_pats = subpats.into_iter() @@ -1588,7 +1634,9 @@ impl<'a> TraitDef<'a> { let variant_ident = variant.node.name; let sp = variant.span.with_ctxt(self.span.ctxt()); let variant_path = cx.path(sp, vec![enum_ident, variant_ident]); - self.create_struct_pattern(cx, variant_path, &variant.node.data, prefix, mutbl) + let use_temporaries = false; // enums can't be repr(packed) + self.create_struct_pattern(cx, variant_path, &variant.node.data, prefix, mutbl, + use_temporaries) } } diff --git a/src/libsyntax_pos/span_encoding.rs b/src/libsyntax_pos/span_encoding.rs index a5c338c913c1..b23e40ce7a93 100644 --- a/src/libsyntax_pos/span_encoding.rs +++ b/src/libsyntax_pos/span_encoding.rs @@ -19,30 +19,16 @@ use hygiene::SyntaxContext; use rustc_data_structures::fx::FxHashMap; use std::cell::RefCell; -use std::hash::{Hash, Hasher}; /// A compressed span. /// Contains either fields of `SpanData` inline if they are small, or index into span interner. /// The primary goal of `Span` is to be as small as possible and fit into other structures /// (that's why it uses `packed` as well). Decoding speed is the second priority. /// See `SpanData` for the info on span fields in decoded representation. -#[derive(Clone, Copy)] +#[derive(Clone, Copy, PartialEq, Eq, Hash)] #[repr(packed)] pub struct Span(u32); -impl PartialEq for Span { - #[inline] - fn eq(&self, other: &Self) -> bool { ({self.0}) == ({other.0}) } -} - -impl Eq for Span {} - -impl Hash for Span { - fn hash(&self, s: &mut H) { - {self.0}.hash(s) - } -} - /// Dummy span, both position and length are zero, syntax context is zero as well. /// This span is kept inline and encoded with format 0. pub const DUMMY_SP: Span = Span(0); diff --git a/src/test/compile-fail/deriving-with-repr-packed-not-copy.rs b/src/test/compile-fail/deriving-with-repr-packed-not-copy.rs new file mode 100644 index 000000000000..5ab916b91fa3 --- /dev/null +++ b/src/test/compile-fail/deriving-with-repr-packed-not-copy.rs @@ -0,0 +1,28 @@ +// Copyright 2017 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. + +// check that derive on a packed struct with non-Copy fields +// correctly. This can't be made to work perfectly because +// we can't just use the field from the struct as it might +// not be aligned. + +#[derive(PartialEq)] +struct Y(usize); + +#[derive(PartialEq)] +//~^ ERROR cannot move out of borrowed +//~| ERROR cannot move out of borrowed +//~| ERROR cannot move out of borrowed +//~| ERROR cannot move out of borrowed +#[repr(packed)] +struct X(Y); + +fn main() { +} diff --git a/src/test/run-pass/deriving-with-repr-packed.rs b/src/test/run-pass/deriving-with-repr-packed.rs new file mode 100644 index 000000000000..fcc31b462f8c --- /dev/null +++ b/src/test/run-pass/deriving-with-repr-packed.rs @@ -0,0 +1,45 @@ +// Copyright 2017 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. + +// check that derive on a packed struct does not call field +// methods with a misaligned field. + +use std::mem; + +#[derive(Copy, Clone)] +struct Aligned(usize); + +#[inline(never)] +fn check_align(ptr: *const Aligned) { + assert_eq!(ptr as usize % mem::align_of::(), + 0); +} + +impl PartialEq for Aligned { + fn eq(&self, other: &Self) -> bool { + check_align(self); + check_align(other); + self.0 == other.0 + } +} + +#[repr(packed)] +#[derive(PartialEq)] +struct Packed(Aligned, Aligned); + +#[derive(PartialEq)] +#[repr(C)] +struct Dealigned(u8, T); + +fn main() { + let d1 = Dealigned(0, Packed(Aligned(1), Aligned(2))); + let ck = d1 == d1; + assert!(ck); +}