bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

fix #[derive] implementation for repr(packed) structs

Browse source 
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.
This commit is contained in:
Ariel Ben-Yehuda 2017-10-02 15:15:23 +02:00 • committed by Ariel Ben-Yehuda
parent 1a2d443f55
commit dee8a71cd5
4 changed files with 138 additions and 31 deletions
Download patch file Download diff file Expand all files Collapse all files

80
src/libsyntax_ext/deriving/generic/mod.rs
View file

@ -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<ast::Item> {
let field_tys: Vec<P<ast::Ty>> = 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<Expr>],
nonself_args: &[P<Expr>])
nonself_args: &[P<Expr>],
use_temporaries: bool)
-> P<Expr> {
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<P<Expr>>,
nonself_args: &[P<Expr>])
-> P<Expr> {
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<ast::SpannedIdent>,
mutbl: ast::Mutability)
mutbl: ast::Mutability,
use_temporaries: bool)
-> Vec<P<ast::Pat>> {
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<ast::Pat>, Vec<(Span, Option<Ident>, P<Expr>, &'a [ast::Attribute])>) {
mutbl: ast::Mutability,
use_temporaries: bool)
-> (P<ast::Pat>, Vec<(Span, Option<Ident>, P<Expr>, &'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)
}
}

16
src/libsyntax_pos/span_encoding.rs
View file

@ -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<H: Hasher>(&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);

28
src/test/compile-fail/deriving-with-repr-packed-not-copy.rs Normal file
View file

@ -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 <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.
// 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() {
}

45
src/test/run-pass/deriving-with-repr-packed.rs Normal file
View file

@ -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 <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.
// 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::<Aligned>(),
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<T>(u8, T);
fn main() {
let d1 = Dealigned(0, Packed(Aligned(1), Aligned(2)));
let ck = d1 == d1;
assert!(ck);
}