Skip to content

Commit 618297e

Browse files
borsbors
committed
Auto merge of #44884 - arielb1:pack-safe, r=<try>
[WIP] make accesses to fields of packed structs unsafe To handle packed structs with destructors (which you'll think are a rare case, but the `#[repr(packed)] struct Packed<T>(T);` pattern is ever-popular, which requires handling packed structs with destructors to avoid monomorphization-time errors), drops of subfields of packed structs should drop a local move of the field instead of the original one. That's it, I think I'll use a strategy suggested by @Zoxc, where this mir ``` drop(packed_struct.field) ``` is replaced by ``` tmp0 = packed_struct.field; drop tmp0 ``` cc #27060 - this should deal with that issue after codegen of drop glue is updated. The new errors need to be changed to future-compatibility warnings, but I'll rather do a crater run first with them as errors to assess the impact. cc @eddyb Things which still need to be done for this: - [ ] - handle `repr(packed)` structs in `derive` the same way I did in `Span`, and use derive there again - [ ] - implement the "fix packed drops" pass and call it in both the MIR shim and validated MIR pipelines - [ ] - do a crater run - [ ] - convert the errors to compatibility warnings
2 parents 185cc5f + 4ec62e4 commit 618297e

File tree

10 files changed

+329
-82
lines changed

10 files changed

+329
-82
lines changed

‎src/Cargo.lock

+61-40
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/librustc_mir/transform/check_unsafety.rs

+49-20
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use rustc_data_structures::fx::FxHashSet;
1212
use rustc_data_structures::indexed_vec::IndexVec;
1313

1414
use rustc::ty::maps::Providers;
15-
use rustc::ty::{self, TyCtxt};
15+
use rustc::ty::{self, Ty, TyCtxt};
1616
use rustc::hir;
1717
use rustc::hir::def::Def;
1818
use rustc::hir::def_id::DefId;
@@ -31,6 +31,9 @@ pub struct UnsafetyChecker<'a, 'tcx: 'a> {
3131
visibility_scope_info: &'a IndexVec<VisibilityScope, VisibilityScopeInfo>,
3232
violations: Vec<UnsafetyViolation>,
3333
source_info: SourceInfo,
34+
// true if an a part of this *memory block* of this expression
35+
// is being borrowed, used for repr(packed) checking.
36+
need_check_packed: bool,
3437
tcx: TyCtxt<'a, 'tcx, 'tcx>,
3538
param_env: ty::ParamEnv<'tcx>,
3639
used_unsafe: FxHashSet<ast::NodeId>,
@@ -51,6 +54,7 @@ impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> {
5154
},
5255
tcx,
5356
param_env,
57+
need_check_packed: false,
5458
used_unsafe: FxHashSet(),
5559
}
5660
}
@@ -130,6 +134,14 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
130134
lvalue: &Lvalue<'tcx>,
131135
context: LvalueContext<'tcx>,
132136
location: Location) {
137+
let old_need_check_packed = self.need_check_packed;
138+
if let LvalueContext::Borrow { .. } = context {
139+
let ty = lvalue.ty(self.mir, self.tcx).to_ty(self.tcx);
140+
if !self.has_align_1(ty) {
141+
self.need_check_packed = true;
142+
}
143+
}
144+
133145
match lvalue {
134146
&Lvalue::Projection(box Projection {
135147
ref base, ref elem
@@ -143,31 +155,39 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
143155
self.source_info = self.mir.local_decls[local].source_info;
144156
}
145157
}
158+
if let &ProjectionElem::Deref = elem {
159+
self.need_check_packed = false;
160+
}
146161
let base_ty = base.ty(self.mir, self.tcx).to_ty(self.tcx);
147162
match base_ty.sty {
148163
ty::TyRawPtr(..) => {
149164
self.require_unsafe("dereference of raw pointer")
150165
}
151-
ty::TyAdt(adt, _) if adt.is_union() => {
152-
if context == LvalueContext::Store ||
153-
context == LvalueContext::Drop
154-
{
155-
let elem_ty = match elem {
156-
&ProjectionElem::Field(_, ty) => ty,
157-
_ => span_bug!(
158-
self.source_info.span,
159-
"non-field projection {:?} from union?",
160-
lvalue)
161-
};
162-
if elem_ty.moves_by_default(self.tcx, self.param_env,
163-
self.source_info.span) {
164-
self.require_unsafe(
165-
"assignment to non-`Copy` union field")
166+
ty::TyAdt(adt, _) => {
167+
if adt.is_union() {
168+
if context == LvalueContext::Store ||
169+
context == LvalueContext::Drop
170+
{
171+
let elem_ty = match elem {
172+
&ProjectionElem::Field(_, ty) => ty,
173+
_ => span_bug!(
174+
self.source_info.span,
175+
"non-field projection {:?} from union?",
176+
lvalue)
177+
};
178+
if elem_ty.moves_by_default(self.tcx, self.param_env,
179+
self.source_info.span) {
180+
self.require_unsafe(
181+
"assignment to non-`Copy` union field")
182+
} else {
183+
// write to non-move union, safe
184+
}
166185
} else {
167-
// write to non-move union, safe
186+
self.require_unsafe("access to union field")
168187
}
169-
} else {
170-
self.require_unsafe("access to union field")
188+
}
189+
if adt.repr.packed() && self.need_check_packed {
190+
self.require_unsafe("borrow of packed field")
171191
}
172192
}
173193
_ => {}
@@ -191,8 +211,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
191211
}]);
192212
}
193213
}
194-
}
214+
};
195215
self.super_lvalue(lvalue, context, location);
216+
self.need_check_packed = old_need_check_packed;
196217
}
197218
}
198219

@@ -215,6 +236,14 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
215236
}
216237
}
217238
}
239+
240+
fn has_align_1(&self, ty: Ty<'tcx>) -> bool {
241+
self.tcx.at(self.source_info.span)
242+
.layout_raw(self.param_env.and(ty))
243+
.map(|layout| layout.align(self.tcx).abi() == 1)
244+
.unwrap_or(false)
245+
}
246+
218247
fn require_unsafe(&mut self,
219248
description: &'static str)
220249
{

‎src/librustc_typeck/check/wfcheck.rs

+24-4
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use constrained_type_params::{identify_constrained_type_params, Parameter};
1414

1515
use hir::def_id::DefId;
1616
use rustc::traits::{self, ObligationCauseCode};
17-
use rustc::ty::{self, Ty, TyCtxt};
17+
use rustc::ty::{self, Lift, Ty, TyCtxt};
1818
use rustc::util::nodemap::{FxHashSet, FxHashMap};
1919
use rustc::middle::lang_items;
2020

@@ -223,10 +223,31 @@ impl<'a, 'gcx> CheckTypeWellFormedVisitor<'a, 'gcx> {
223223
{
224224
self.for_item(item).with_fcx(|fcx, this| {
225225
let variants = lookup_fields(fcx);
226+
let def_id = fcx.tcx.hir.local_def_id(item.id);
227+
let packed = fcx.tcx.adt_def(def_id).repr.packed();
226228

227229
for variant in &variants {
228-
// For DST, all intermediate types must be sized.
229-
let unsized_len = if all_sized || variant.fields.is_empty() { 0 } else { 1 };
230+
// For DST, or when drop needs to copy things around, all
231+
// intermediate types must be sized.
232+
let needs_drop_copy = || {
233+
packed && {
234+
let ty = variant.fields.last().unwrap().ty;
235+
let ty = fcx.tcx.erase_regions(&ty).lift_to_tcx(this.tcx)
236+
.unwrap_or_else(|| {
237+
span_bug!(item.span, "inference variables in {:?}", ty)
238+
});
239+
ty.needs_drop(this.tcx, this.tcx.param_env(def_id))
240+
}
241+
};
242+
let unsized_len = if
243+
all_sized ||
244+
variant.fields.is_empty() ||
245+
needs_drop_copy()
246+
{
247+
0
248+
} else {
249+
1
250+
};
230251
for field in &variant.fields[..variant.fields.len() - unsized_len] {
231252
fcx.register_bound(
232253
field.ty,
@@ -245,7 +266,6 @@ impl<'a, 'gcx> CheckTypeWellFormedVisitor<'a, 'gcx> {
245266
}
246267
}
247268

248-
let def_id = fcx.tcx.hir.local_def_id(item.id);
249269
let predicates = fcx.tcx.predicates_of(def_id).instantiate_identity(fcx.tcx);
250270
let predicates = fcx.normalize_associated_types_in(item.span, &predicates);
251271
this.check_where_clauses(fcx, item.span, &predicates);

‎src/librustdoc/Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ doctest = false
1414
env_logger = { version = "0.4", default-features = false }
1515
log = "0.3"
1616
pulldown-cmark = { version = "0.0.14", default-features = false }
17-
html-diff = "0.0.4"
17+
html-diff = "0.0.5"
1818

1919
[build-dependencies]
2020
build_helper = { path = "../build_helper" }

‎src/libsyntax_ext/deriving/generic/mod.rs

+64-16
Original file line numberDiff line numberDiff line change
@@ -393,34 +393,47 @@ fn find_type_parameters(ty: &ast::Ty,
393393
}
394394

395395
impl<'a> TraitDef<'a> {
396-
pub fn expand(&self,
396+
pub fn expand(self,
397397
cx: &mut ExtCtxt,
398398
mitem: &ast::MetaItem,
399399
item: &'a Annotatable,
400400
push: &mut FnMut(Annotatable)) {
401401
self.expand_ext(cx, mitem, item, push, false);
402402
}
403403

404-
pub fn expand_ext(&self,
404+
pub fn expand_ext(self,
405405
cx: &mut ExtCtxt,
406406
mitem: &ast::MetaItem,
407407
item: &'a Annotatable,
408408
push: &mut FnMut(Annotatable),
409409
from_scratch: bool) {
410410
match *item {
411411
Annotatable::Item(ref item) => {
412+
let is_packed = item.attrs.iter().any(|attr| {
413+
attr::find_repr_attrs(&cx.parse_sess.span_diagnostic, attr)
414+
.contains(&attr::ReprPacked)
415+
});
416+
let use_temporaries = is_packed;
412417
let newitem = match item.node {
413418
ast::ItemKind::Struct(ref struct_def, ref generics) => {
414-
self.expand_struct_def(cx, &struct_def, item.ident, generics, from_scratch)
419+
self.expand_struct_def(cx, &struct_def, item.ident, generics, from_scratch,
420+
use_temporaries)
415421
}
416422
ast::ItemKind::Enum(ref enum_def, ref generics) => {
423+
// We ignore `use_temporaries` here, because
424+
// `repr(packed)` enums cause an error later on.
425+
//
426+
// This can only cause further compilation errors
427+
// downstream in blatantly illegal code, so it
428+
// is fine.
417429
self.expand_enum_def(cx, enum_def, &item.attrs,
418430
item.ident, generics, from_scratch)
419431
}
420432
ast::ItemKind::Union(ref struct_def, ref generics) => {
421433
if self.supports_unions {
422434
self.expand_struct_def(cx, &struct_def, item.ident,
423-
generics, from_scratch)
435+
generics, from_scratch,
436+
use_temporaries)
424437
} else {
425438
cx.span_err(mitem.span,
426439
"this trait cannot be derived for unions");
@@ -674,7 +687,8 @@ impl<'a> TraitDef<'a> {
674687
struct_def: &'a VariantData,
675688
type_ident: Ident,
676689
generics: &Generics,
677-
from_scratch: bool)
690+
from_scratch: bool,
691+
use_temporaries: bool)
678692
-> P<ast::Item> {
679693
let field_tys: Vec<P<ast::Ty>> = struct_def.fields()
680694
.iter()
@@ -700,7 +714,8 @@ impl<'a> TraitDef<'a> {
700714
struct_def,
701715
type_ident,
702716
&self_args[..],
703-
&nonself_args[..])
717+
&nonself_args[..],
718+
use_temporaries)
704719
};
705720

706721
method_def.create_method(cx,
@@ -957,14 +972,31 @@ impl<'a> MethodDef<'a> {
957972
/// }
958973
/// }
959974
/// }
975+
///
976+
/// // or if A is repr(packed) - note fields are matched by-value
977+
/// // instead of by-reference.
978+
/// impl PartialEq for A {
979+
/// fn eq(&self, __arg_1: &A) -> bool {
980+
/// match *self {
981+
/// A {x: __self_0_0, y: __self_0_1} => {
982+
/// match __arg_1 {
983+
/// A {x: __self_1_0, y: __self_1_1} => {
984+
/// __self_0_0.eq(&__self_1_0) && __self_0_1.eq(&__self_1_1)
985+
/// }
986+
/// }
987+
/// }
988+
/// }
989+
/// }
990+
/// }
960991
/// ```
961992
fn expand_struct_method_body<'b>(&self,
962993
cx: &mut ExtCtxt,
963994
trait_: &TraitDef<'b>,
964995
struct_def: &'b VariantData,
965996
type_ident: Ident,
966997
self_args: &[P<Expr>],
967-
nonself_args: &[P<Expr>])
998+
nonself_args: &[P<Expr>],
999+
use_temporaries: bool)
9681000
-> P<Expr> {
9691001

9701002
let mut raw_fields = Vec::new(); // Vec<[fields of self],
@@ -976,7 +1008,8 @@ impl<'a> MethodDef<'a> {
9761008
struct_path,
9771009
struct_def,
9781010
&format!("__self_{}", i),
979-
ast::Mutability::Immutable);
1011+
ast::Mutability::Immutable,
1012+
use_temporaries);
9801013
patterns.push(pat);
9811014
raw_fields.push(ident_expr);
9821015
}
@@ -1139,7 +1172,6 @@ impl<'a> MethodDef<'a> {
11391172
self_args: Vec<P<Expr>>,
11401173
nonself_args: &[P<Expr>])
11411174
-> P<Expr> {
1142-
11431175
let sp = trait_.span;
11441176
let variants = &enum_def.variants;
11451177

@@ -1511,12 +1543,18 @@ impl<'a> TraitDef<'a> {
15111543
fn create_subpatterns(&self,
15121544
cx: &mut ExtCtxt,
15131545
field_paths: Vec<ast::SpannedIdent>,
1514-
mutbl: ast::Mutability)
1546+
mutbl: ast::Mutability,
1547+
use_temporaries: bool)
15151548
-> Vec<P<ast::Pat>> {
15161549
field_paths.iter()
15171550
.map(|path| {
1551+
let binding_mode = if use_temporaries {
1552+
ast::BindingMode::ByValue(ast::Mutability::Immutable)
1553+
} else {
1554+
ast::BindingMode::ByRef(mutbl)
1555+
};
15181556
cx.pat(path.span,
1519-
PatKind::Ident(ast::BindingMode::ByRef(mutbl), (*path).clone(), None))
1557+
PatKind::Ident(binding_mode, (*path).clone(), None))
15201558
})
15211559
.collect()
15221560
}
@@ -1527,8 +1565,10 @@ impl<'a> TraitDef<'a> {
15271565
struct_path: ast::Path,
15281566
struct_def: &'a VariantData,
15291567
prefix: &str,
1530-
mutbl: ast::Mutability)
1531-
-> (P<ast::Pat>, Vec<(Span, Option<Ident>, P<Expr>, &'a [ast::Attribute])>) {
1568+
mutbl: ast::Mutability,
1569+
use_temporaries: bool)
1570+
-> (P<ast::Pat>, Vec<(Span, Option<Ident>, P<Expr>, &'a [ast::Attribute])>)
1571+
{
15321572
let mut paths = Vec::new();
15331573
let mut ident_exprs = Vec::new();
15341574
for (i, struct_field) in struct_def.fields().iter().enumerate() {
@@ -1538,12 +1578,18 @@ impl<'a> TraitDef<'a> {
15381578
span: sp,
15391579
node: ident,
15401580
});
1541-
let val = cx.expr_deref(sp, cx.expr_path(cx.path_ident(sp, ident)));
1581+
let val = cx.expr_path(cx.path_ident(sp, ident));
1582+
let val = if use_temporaries {
1583+
val
1584+
} else {
1585+
cx.expr_deref(sp, val)
1586+
};
15421587
let val = cx.expr(sp, ast::ExprKind::Paren(val));
1588+
15431589
ident_exprs.push((sp, struct_field.ident, val, &struct_field.attrs[..]));
15441590
}
15451591

1546-
let subpats = self.create_subpatterns(cx, paths, mutbl);
1592+
let subpats = self.create_subpatterns(cx, paths, mutbl, use_temporaries);
15471593
let pattern = match *struct_def {
15481594
VariantData::Struct(..) => {
15491595
let field_pats = subpats.into_iter()
@@ -1587,7 +1633,9 @@ impl<'a> TraitDef<'a> {
15871633
let variant_ident = variant.node.name;
15881634
let sp = variant.span.with_ctxt(self.span.ctxt());
15891635
let variant_path = cx.path(sp, vec![enum_ident, variant_ident]);
1590-
self.create_struct_pattern(cx, variant_path, &variant.node.data, prefix, mutbl)
1636+
let use_temporaries = false; // enums can't be repr(packed)
1637+
self.create_struct_pattern(cx, variant_path, &variant.node.data, prefix, mutbl,
1638+
use_temporaries)
15911639
}
15921640
}
15931641

0 commit comments

Comments
 (0)