Skip to content

Commit 9213a54

Browse files
committed
Be stricter about combining repr(packed) with derive.
Currently there are two errors you can get when using `derive` on a `repr(packed)` struct: - "`#[derive]` can't be used on a `#[repr(packed)]` struct with type or const parameters (error E0133)" - "`#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)" These were introduced under #82523, because they can lead to unaligned references, which is UB. They are in the process of being upgraded to hard errors. This is all fine, but the second error overstates things. It *is* possible to use `derive` on a `repr(packed)` struct that doesn't derive `Copy` in two cases. - If all the fields within the struct meet the required alignment: 1 for `repr(packed)`, or N for `repr(packed(N))`. - Or, If `Default` is the only trait derived. These exceptions don't seem particularly important or useful, and the language would be simpler if they are removed. So this commit does that, making these cases a hard error. The new checks are done at derive code creation time which means that `unsafe_derive_on_repr_packed` is no longer needed at MIR build time. Test changes: - deriving-with-repr-packed.rs has numerous changes to the code to broaden coverage. Some of the errors are reported twice now, which is hard to avoid but harmless (and users won't see duplicates thanks to the normal error de-duplication). Also, if a packed struct is both non-`Copy` and has type params, both those errors are now reported. - deriving-all-codegen.rs has the `PackedNonCopy` case removed, because it's no longer allowed. - packed.rs needs `Copy` added to some packed struct that currently only derive `Default`.
1 parent 6d114fb commit 9213a54

File tree

10 files changed

+152
-311
lines changed

10 files changed

+152
-311
lines changed

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

-7
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,6 @@ pub fn expand_deriving_partial_eq(
3838
// stripping `&` or adding `*`. This isn't necessary for
3939
// type checking, but it results in much better error
4040
// messages if something goes wrong.
41-
//
42-
// For `packed` structs that impl `Copy`, we'll end up with
43-
// an expr like `{ self.0 } != { other.0 }`. It would be
44-
// nice to strip those blocks, giving `self.0 != other.0`,
45-
// but then the "`#[derive]` can't be used on a
46-
// `#[repr(packed)]` struct that does not derive Copy"
47-
// error is issued erroneously by the MIR builder.
4841
let convert = |expr: &P<Expr>| {
4942
if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, inner) =
5043
&expr.kind

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

+83-82
Original file line numberDiff line numberDiff line change
@@ -413,6 +413,12 @@ fn find_type_parameters(
413413
visitor.type_params
414414
}
415415

416+
#[derive(Clone, Copy)]
417+
struct PackedStatus {
418+
has_type_params: bool,
419+
has_derive_copy: bool,
420+
}
421+
416422
impl<'a> TraitDef<'a> {
417423
pub fn expand(
418424
self,
@@ -442,17 +448,22 @@ impl<'a> TraitDef<'a> {
442448
}
443449
false
444450
});
445-
let has_no_type_params = match item.kind {
446-
ast::ItemKind::Struct(_, ref generics)
447-
| ast::ItemKind::Enum(_, ref generics)
448-
| ast::ItemKind::Union(_, ref generics) => !generics
449-
.params
450-
.iter()
451-
.any(|param| matches!(param.kind, ast::GenericParamKind::Type { .. })),
452-
_ => unreachable!(),
451+
let packed_status = if !is_packed {
452+
None
453+
} else {
454+
let has_type_params = match item.kind {
455+
ast::ItemKind::Struct(_, ref generics)
456+
| ast::ItemKind::Enum(_, ref generics)
457+
| ast::ItemKind::Union(_, ref generics) => generics
458+
.params
459+
.iter()
460+
.any(|param| matches!(param.kind, ast::GenericParamKind::Type { .. })),
461+
_ => unreachable!(),
462+
};
463+
let container_id = cx.current_expansion.id.expn_data().parent.expect_local();
464+
let has_derive_copy = cx.resolver.has_derive_copy(container_id);
465+
Some(PackedStatus { has_type_params, has_derive_copy })
453466
};
454-
let container_id = cx.current_expansion.id.expn_data().parent.expect_local();
455-
let always_copy = has_no_type_params && cx.resolver.has_derive_copy(container_id);
456467

457468
let newitem = match item.kind {
458469
ast::ItemKind::Struct(ref struct_def, ref generics) => self.expand_struct_def(
@@ -461,11 +472,10 @@ impl<'a> TraitDef<'a> {
461472
item.ident,
462473
generics,
463474
from_scratch,
464-
is_packed,
465-
always_copy,
475+
packed_status,
466476
),
467477
ast::ItemKind::Enum(ref enum_def, ref generics) => {
468-
// We ignore `is_packed`/`always_copy` here, because
478+
// We ignore `packed_status` here, because
469479
// `repr(packed)` enums cause an error later on.
470480
//
471481
// This can only cause further compilation errors
@@ -481,8 +491,7 @@ impl<'a> TraitDef<'a> {
481491
item.ident,
482492
generics,
483493
from_scratch,
484-
is_packed,
485-
always_copy,
494+
packed_status,
486495
)
487496
} else {
488497
cx.span_err(mitem.span, "this trait cannot be derived for unions");
@@ -763,8 +772,7 @@ impl<'a> TraitDef<'a> {
763772
type_ident: Ident,
764773
generics: &Generics,
765774
from_scratch: bool,
766-
is_packed: bool,
767-
always_copy: bool,
775+
packed_status: Option<PackedStatus>,
768776
) -> P<ast::Item> {
769777
let field_tys: Vec<P<ast::Ty>> =
770778
struct_def.fields().iter().map(|field| field.ty.clone()).collect();
@@ -783,6 +791,7 @@ impl<'a> TraitDef<'a> {
783791
struct_def,
784792
type_ident,
785793
&nonselflike_args,
794+
packed_status,
786795
)
787796
} else {
788797
method_def.expand_struct_method_body(
@@ -792,8 +801,7 @@ impl<'a> TraitDef<'a> {
792801
type_ident,
793802
&selflike_args,
794803
&nonselflike_args,
795-
is_packed,
796-
always_copy,
804+
packed_status,
797805
)
798806
};
799807

@@ -1013,33 +1021,24 @@ impl<'a> MethodDef<'a> {
10131021
/// }
10141022
/// ```
10151023
/// But if the struct is `repr(packed)`, we can't use something like
1016-
/// `&self.x` on a packed type (as required for e.g. `Debug` and `Hash`)
1017-
/// because that might cause an unaligned ref. If the struct impls `Copy`,
1018-
/// we use a local block to force a copy:
1024+
/// `&self.x` on a packed type because that might cause an unaligned ref.
1025+
/// So for any trait method that takes a reference we use a local block to
1026+
/// force a copy, which works because such structs must be `Copy`:
10191027
/// ```
10201028
/// # struct A { x: u8, y: u8 }
10211029
/// impl PartialEq for A {
10221030
/// fn eq(&self, other: &A) -> bool {
1031+
/// // Desugars to `{ self.x }.eq(&{ other.y }) && ...`
10231032
/// { self.x } == { other.y } && { self.y } == { other.y }
10241033
/// }
10251034
/// }
1026-
/// ```
1027-
/// (The copy isn't necessary for `eq`, but it makes more sense for other
1028-
/// functions that use e.g. `&{ self.x }`.)
1029-
///
1030-
/// If it doesn't impl `Copy`, we use let-destructuring with `ref`:
1031-
/// ```
1032-
/// # struct A { x: u8, y: u8 }
1033-
/// impl PartialEq for A {
1034-
/// fn eq(&self, other: &A) -> bool {
1035-
/// let Self { x: ref __self_0_0, y: ref __self_0_1 } = *self;
1036-
/// let Self { x: ref __self_1_0, y: ref __self_1_1 } = *other;
1037-
/// *__self_0_0 == *__self_1_0 && *__self_0_1 == *__self_1_1
1035+
/// impl Hash for A {
1036+
/// fn hash<__H: ::core::hash::Hasher>(&self, state: &mut __H) -> () {
1037+
/// ::core::hash::Hash::hash(&{ self.x }, state);
1038+
/// ::core::hash::Hash::hash(&{ self.y }, state)
10381039
/// }
10391040
/// }
10401041
/// ```
1041-
/// This latter case only works if the fields match the alignment required
1042-
/// by the `packed(N)` attribute.
10431042
fn expand_struct_method_body<'b>(
10441043
&self,
10451044
cx: &mut ExtCtxt<'_>,
@@ -1048,54 +1047,24 @@ impl<'a> MethodDef<'a> {
10481047
type_ident: Ident,
10491048
selflike_args: &[P<Expr>],
10501049
nonselflike_args: &[P<Expr>],
1051-
is_packed: bool,
1052-
always_copy: bool,
1050+
packed_status: Option<PackedStatus>,
10531051
) -> BlockOrExpr {
10541052
let span = trait_.span;
10551053
assert!(selflike_args.len() == 1 || selflike_args.len() == 2);
10561054

1057-
let mk_body = |cx, selflike_fields| {
1058-
self.call_substructure_method(
1059-
cx,
1060-
trait_,
1061-
type_ident,
1062-
nonselflike_args,
1063-
&Struct(struct_def, selflike_fields),
1064-
)
1065-
};
1066-
1067-
if !is_packed {
1068-
let selflike_fields =
1069-
trait_.create_struct_field_access_fields(cx, selflike_args, struct_def, false);
1070-
mk_body(cx, selflike_fields)
1071-
} else if always_copy {
1072-
let selflike_fields =
1073-
trait_.create_struct_field_access_fields(cx, selflike_args, struct_def, true);
1074-
mk_body(cx, selflike_fields)
1075-
} else {
1076-
// Neither packed nor copy. Need to use ref patterns.
1077-
let prefixes: Vec<_> =
1078-
(0..selflike_args.len()).map(|i| format!("__self_{}", i)).collect();
1079-
let addr_of = always_copy;
1080-
let selflike_fields =
1081-
trait_.create_struct_pattern_fields(cx, struct_def, &prefixes, addr_of);
1082-
let mut body = mk_body(cx, selflike_fields);
1083-
1084-
let struct_path = cx.path(span, vec![Ident::new(kw::SelfUpper, type_ident.span)]);
1085-
let use_ref_pat = is_packed && !always_copy;
1086-
let patterns =
1087-
trait_.create_struct_patterns(cx, struct_path, struct_def, &prefixes, use_ref_pat);
1088-
1089-
// Do the let-destructuring.
1090-
let mut stmts: Vec<_> = iter::zip(selflike_args, patterns)
1091-
.map(|(selflike_arg_expr, pat)| {
1092-
let selflike_arg_expr = cx.expr_deref(span, selflike_arg_expr.clone());
1093-
cx.stmt_let_pat(span, pat, selflike_arg_expr)
1094-
})
1095-
.collect();
1096-
stmts.extend(std::mem::take(&mut body.0));
1097-
BlockOrExpr(stmts, body.1)
1055+
if let Err(res) = Self::check_packed_status(cx, span, packed_status) {
1056+
return res;
10981057
}
1058+
1059+
let selflike_fields =
1060+
trait_.create_struct_field_access_fields(cx, selflike_args, struct_def, packed_status);
1061+
self.call_substructure_method(
1062+
cx,
1063+
trait_,
1064+
type_ident,
1065+
nonselflike_args,
1066+
&Struct(struct_def, selflike_fields),
1067+
)
10991068
}
11001069

11011070
fn expand_static_struct_method_body(
@@ -1105,9 +1074,15 @@ impl<'a> MethodDef<'a> {
11051074
struct_def: &VariantData,
11061075
type_ident: Ident,
11071076
nonselflike_args: &[P<Expr>],
1077+
packed_status: Option<PackedStatus>,
11081078
) -> BlockOrExpr {
1109-
let summary = trait_.summarise_struct(cx, struct_def);
1079+
let span = trait_.span;
1080+
1081+
if Self::check_packed_status(cx, span, packed_status).is_err() {
1082+
return BlockOrExpr(vec![], Some(deriving::call_abort(cx, span)));
1083+
}
11101084

1085+
let summary = trait_.summarise_struct(cx, struct_def);
11111086
self.call_substructure_method(
11121087
cx,
11131088
trait_,
@@ -1117,6 +1092,32 @@ impl<'a> MethodDef<'a> {
11171092
)
11181093
}
11191094

1095+
fn check_packed_status(
1096+
cx: &ExtCtxt<'_>,
1097+
span: Span,
1098+
packed_status: Option<PackedStatus>,
1099+
) -> Result<(), BlockOrExpr> {
1100+
let mut res = Ok(());
1101+
let err = || Err(BlockOrExpr(vec![], Some(deriving::call_abort(cx, span))));
1102+
if let Some(PackedStatus { has_type_params, has_derive_copy }) = packed_status {
1103+
// FIXME: when we make this a hard error, this should have its
1104+
// own error code.
1105+
if has_type_params {
1106+
let msg = "`#[derive]` can't be used on a `#[repr(packed)]` struct with \
1107+
type or const parameters (error E0133)";
1108+
cx.sess.struct_span_err(span, msg).emit();
1109+
res = err();
1110+
}
1111+
if !has_derive_copy {
1112+
let msg = "`#[derive]` can't be used on a `#[repr(packed)]` struct that \
1113+
does not derive Copy (error E0133)";
1114+
cx.sess.struct_span_err(span, msg).emit();
1115+
res = err();
1116+
}
1117+
}
1118+
res
1119+
}
1120+
11201121
/// ```
11211122
/// #[derive(PartialEq)]
11221123
/// # struct Dummy;
@@ -1542,7 +1543,7 @@ impl<'a> TraitDef<'a> {
15421543
cx: &mut ExtCtxt<'_>,
15431544
selflike_args: &[P<Expr>],
15441545
struct_def: &'a VariantData,
1545-
copy: bool,
1546+
packed_status: Option<PackedStatus>,
15461547
) -> Vec<FieldInfo> {
15471548
self.create_fields(struct_def, |i, struct_field, sp| {
15481549
selflike_args
@@ -1561,7 +1562,7 @@ impl<'a> TraitDef<'a> {
15611562
}),
15621563
),
15631564
);
1564-
if copy {
1565+
if packed_status.is_some() {
15651566
field_expr = cx.expr_block(
15661567
cx.block(struct_field.span, vec![cx.stmt_expr(field_expr)]),
15671568
);

compiler/rustc_builtin_macros/src/deriving/mod.rs

+7
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,13 @@ fn call_unreachable(cx: &ExtCtxt<'_>, span: Span) -> P<ast::Expr> {
106106
}))
107107
}
108108

109+
/// Constructs an expression that calls the `abort` intrinsic.
110+
fn call_abort(cx: &ExtCtxt<'_>, span: Span) -> P<ast::Expr> {
111+
let span = cx.with_def_site_ctxt(span);
112+
let path = cx.std_path(&[sym::intrinsics, sym::abort]);
113+
cx.expr_call_global(span, path, vec![])
114+
}
115+
109116
// Injects `impl<...> Structural for ItemType<...> { }`. In particular,
110117
// does *not* add `where T: Structural` for parameters `T` in `...`.
111118
// (That's the main reason we cannot use TraitDef here.)

compiler/rustc_mir_transform/src/check_packed_ref.rs

+6-30
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,11 @@
1-
use rustc_hir::def_id::LocalDefId;
21
use rustc_middle::mir::visit::{PlaceContext, Visitor};
32
use rustc_middle::mir::*;
4-
use rustc_middle::ty::query::Providers;
53
use rustc_middle::ty::{self, TyCtxt};
64
use rustc_session::lint::builtin::UNALIGNED_REFERENCES;
75

86
use crate::util;
97
use crate::MirLint;
108

11-
pub(crate) fn provide(providers: &mut Providers) {
12-
*providers = Providers { unsafe_derive_on_repr_packed, ..*providers };
13-
}
14-
159
pub struct CheckPackedRef;
1610

1711
impl<'tcx> MirLint<'tcx> for CheckPackedRef {
@@ -30,23 +24,6 @@ struct PackedRefChecker<'a, 'tcx> {
3024
source_info: SourceInfo,
3125
}
3226

33-
fn unsafe_derive_on_repr_packed(tcx: TyCtxt<'_>, def_id: LocalDefId) {
34-
let lint_hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
35-
36-
tcx.struct_span_lint_hir(UNALIGNED_REFERENCES, lint_hir_id, tcx.def_span(def_id), |lint| {
37-
// FIXME: when we make this a hard error, this should have its
38-
// own error code.
39-
let message = if tcx.generics_of(def_id).own_requires_monomorphization() {
40-
"`#[derive]` can't be used on a `#[repr(packed)]` struct with \
41-
type or const parameters (error E0133)"
42-
} else {
43-
"`#[derive]` can't be used on a `#[repr(packed)]` struct that \
44-
does not derive Copy (error E0133)"
45-
};
46-
lint.build(message).emit();
47-
});
48-
}
49-
5027
impl<'tcx> Visitor<'tcx> for PackedRefChecker<'_, 'tcx> {
5128
fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
5229
// Make sure we know where in the MIR we are.
@@ -64,14 +41,13 @@ impl<'tcx> Visitor<'tcx> for PackedRefChecker<'_, 'tcx> {
6441
if context.is_borrow() {
6542
if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
6643
let def_id = self.body.source.instance.def_id();
67-
if let Some(impl_def_id) = self
68-
.tcx
69-
.impl_of_method(def_id)
70-
.filter(|&def_id| self.tcx.is_builtin_derive(def_id))
44+
if let Some(impl_def_id) = self.tcx.impl_of_method(def_id)
45+
&& self.tcx.is_builtin_derive(impl_def_id)
7146
{
72-
// If a method is defined in the local crate,
73-
// the impl containing that method should also be.
74-
self.tcx.ensure().unsafe_derive_on_repr_packed(impl_def_id.expect_local());
47+
// If we ever reach here it means that the generated derive
48+
// code is somehow doing an unaligned reference, which it
49+
// shouldn't do.
50+
unreachable!();
7551
} else {
7652
let source_info = self.source_info;
7753
let lint_root = self.body.source_scopes[source_info.scope]

compiler/rustc_mir_transform/src/lib.rs

-1
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,6 @@ use rustc_mir_dataflow::rustc_peek;
9797

9898
pub fn provide(providers: &mut Providers) {
9999
check_unsafety::provide(providers);
100-
check_packed_ref::provide(providers);
101100
coverage::query::provide(providers);
102101
ffi_unwind_calls::provide(providers);
103102
shim::provide(providers);

0 commit comments

Comments
 (0)