Skip to content

Commit 58e1234

Browse files
borsbors
committed
Auto merge of #44884 - arielb1:pack-safe, r=nikomatsakis,eddyb
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 5face5f + f3b2d7f commit 58e1234

32 files changed

+1026
-232
lines changed

‎src/Cargo.lock

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

‎src/librustc/dep_graph/dep_node.rs

+1
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,7 @@ define_dep_nodes!( <'tcx>
479479
[] BorrowCheck(DefId),
480480
[] MirBorrowCheck(DefId),
481481
[] UnsafetyCheckResult(DefId),
482+
[] UnsafeDeriveOnReprPacked(DefId),
482483

483484
[] Reachability,
484485
[] MirKeys,

‎src/librustc/ich/impls_mir.rs

+20-1
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,28 @@ impl_stable_hash_for!(struct mir::LocalDecl<'tcx> {
3333
});
3434
impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, by_ref });
3535
impl_stable_hash_for!(struct mir::BasicBlockData<'tcx> { statements, terminator, is_cleanup });
36-
impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, lint_node_id });
36+
impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, kind });
3737
impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_blocks });
3838

39+
impl<'gcx> HashStable<StableHashingContext<'gcx>>
40+
for mir::UnsafetyViolationKind {
41+
#[inline]
42+
fn hash_stable<W: StableHasherResult>(&self,
43+
hcx: &mut StableHashingContext<'gcx>,
44+
hasher: &mut StableHasher<W>) {
45+
46+
mem::discriminant(self).hash_stable(hcx, hasher);
47+
48+
match *self {
49+
mir::UnsafetyViolationKind::General => {}
50+
mir::UnsafetyViolationKind::ExternStatic(lint_node_id) |
51+
mir::UnsafetyViolationKind::BorrowPacked(lint_node_id) => {
52+
lint_node_id.hash_stable(hcx, hasher);
53+
}
54+
55+
}
56+
}
57+
}
3958
impl<'gcx> HashStable<StableHashingContext<'gcx>>
4059
for mir::Terminator<'gcx> {
4160
#[inline]

‎src/librustc/lint/builtin.rs

+7
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,12 @@ declare_lint! {
155155
"safe access to extern statics was erroneously allowed"
156156
}
157157

158+
declare_lint! {
159+
pub SAFE_PACKED_BORROWS,
160+
Warn,
161+
"safe borrows of fields of packed structs were was erroneously allowed"
162+
}
163+
158164
declare_lint! {
159165
pub PATTERNS_IN_FNS_WITHOUT_BODY,
160166
Warn,
@@ -247,6 +253,7 @@ impl LintPass for HardwiredLints {
247253
RENAMED_AND_REMOVED_LINTS,
248254
RESOLVE_TRAIT_ON_DEFAULTED_UNIT,
249255
SAFE_EXTERN_STATICS,
256+
SAFE_PACKED_BORROWS,
250257
PATTERNS_IN_FNS_WITHOUT_BODY,
251258
LEGACY_DIRECTORY_OWNERSHIP,
252259
LEGACY_IMPORTS,

‎src/librustc/mir/mod.rs

+8-1
Original file line numberDiff line numberDiff line change
@@ -1722,11 +1722,18 @@ impl Location {
17221722
}
17231723
}
17241724

1725+
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
1726+
pub enum UnsafetyViolationKind {
1727+
General,
1728+
ExternStatic(ast::NodeId),
1729+
BorrowPacked(ast::NodeId),
1730+
}
1731+
17251732
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
17261733
pub struct UnsafetyViolation {
17271734
pub source_info: SourceInfo,
17281735
pub description: &'static str,
1729-
pub lint_node_id: Option<ast::NodeId>,
1736+
pub kind: UnsafetyViolationKind,
17301737
}
17311738

17321739
#[derive(Clone, Debug, PartialEq, Eq, Hash)]

‎src/librustc/traits/coherence.rs

+79-46
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,18 @@ use ty::subst::Subst;
1919

2020
use infer::{InferCtxt, InferOk};
2121

22-
#[derive(Copy, Clone)]
23-
struct InferIsLocal(bool);
22+
#[derive(Copy, Clone, Debug)]
23+
enum InferIsLocal {
24+
BrokenYes,
25+
Yes,
26+
No
27+
}
28+
29+
#[derive(Debug, Copy, Clone)]
30+
pub enum Conflict {
31+
Upstream,
32+
Downstream
33+
}
2434

2535
pub struct OverlapResult<'tcx> {
2636
pub impl_header: ty::ImplHeader<'tcx>,
@@ -126,32 +136,46 @@ fn overlap<'cx, 'gcx, 'tcx>(selcx: &mut SelectionContext<'cx, 'gcx, 'tcx>,
126136
}
127137

128138
pub fn trait_ref_is_knowable<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
129-
trait_ref: ty::TraitRef<'tcx>) -> bool
139+
trait_ref: ty::TraitRef<'tcx>,
140+
broken: bool)
141+
-> Option<Conflict>
130142
{
131-
debug!("trait_ref_is_knowable(trait_ref={:?})", trait_ref);
132-
133-
// if the orphan rules pass, that means that no ancestor crate can
134-
// impl this, so it's up to us.
135-
if orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(false)).is_ok() {
136-
debug!("trait_ref_is_knowable: orphan check passed");
137-
return true;
143+
debug!("trait_ref_is_knowable(trait_ref={:?}, broken={:?})", trait_ref, broken);
144+
let mode = if broken {
145+
InferIsLocal::BrokenYes
146+
} else {
147+
InferIsLocal::Yes
148+
};
149+
if orphan_check_trait_ref(tcx, trait_ref, mode).is_ok() {
150+
// A downstream or cousin crate is allowed to implement some
151+
// substitution of this trait-ref.
152+
debug!("trait_ref_is_knowable: downstream crate might implement");
153+
return Some(Conflict::Downstream);
138154
}
139155

140-
// if the trait is not marked fundamental, then it's always possible that
141-
// an ancestor crate will impl this in the future, if they haven't
142-
// already
143-
if !trait_ref_is_local_or_fundamental(tcx, trait_ref) {
144-
debug!("trait_ref_is_knowable: trait is neither local nor fundamental");
145-
return false;
156+
if trait_ref_is_local_or_fundamental(tcx, trait_ref) {
157+
// This is a local or fundamental trait, so future-compatibility
158+
// is no concern. We know that downstream/cousin crates are not
159+
// allowed to implement a substitution of this trait ref, which
160+
// means impls could only come from dependencies of this crate,
161+
// which we already know about.
162+
return None;
163+
}
164+
// This is a remote non-fundamental trait, so if another crate
165+
// can be the "final owner" of a substitution of this trait-ref,
166+
// they are allowed to implement it future-compatibly.
167+
//
168+
// However, if we are a final owner, then nobody else can be,
169+
// and if we are an intermediate owner, then we don't care
170+
// about future-compatibility, which means that we're OK if
171+
// we are an owner.
172+
if orphan_check_trait_ref(tcx, trait_ref, InferIsLocal::No).is_ok() {
173+
debug!("trait_ref_is_knowable: orphan check passed");
174+
return None;
175+
} else {
176+
debug!("trait_ref_is_knowable: nonlocal, nonfundamental, unowned");
177+
return Some(Conflict::Upstream);
146178
}
147-
148-
// find out when some downstream (or cousin) crate could impl this
149-
// trait-ref, presuming that all the parameters were instantiated
150-
// with downstream types. If not, then it could only be
151-
// implemented by an upstream crate, which means that the impl
152-
// must be visible to us, and -- since the trait is fundamental
153-
// -- we can test.
154-
orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(true)).is_err()
155179
}
156180

157181
pub fn trait_ref_is_local_or_fundamental<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
@@ -189,16 +213,16 @@ pub fn orphan_check<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
189213
return Ok(());
190214
}
191215

192-
orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(false))
216+
orphan_check_trait_ref(tcx, trait_ref, InferIsLocal::No)
193217
}
194218

195219
fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
196220
trait_ref: ty::TraitRef<'tcx>,
197221
infer_is_local: InferIsLocal)
198222
-> Result<(), OrphanCheckErr<'tcx>>
199223
{
200-
debug!("orphan_check_trait_ref(trait_ref={:?}, infer_is_local={})",
201-
trait_ref, infer_is_local.0);
224+
debug!("orphan_check_trait_ref(trait_ref={:?}, infer_is_local={:?})",
225+
trait_ref, infer_is_local);
202226

203227
// First, create an ordered iterator over all the type parameters to the trait, with the self
204228
// type appearing first.
@@ -212,7 +236,9 @@ fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
212236
// uncovered type parameters.
213237
let uncovered_tys = uncovered_tys(tcx, input_ty, infer_is_local);
214238
for uncovered_ty in uncovered_tys {
215-
if let Some(param) = uncovered_ty.walk().find(|t| is_type_parameter(t)) {
239+
if let Some(param) = uncovered_ty.walk()
240+
.find(|t| is_possibly_remote_type(t, infer_is_local))
241+
{
216242
debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
217243
return Err(OrphanCheckErr::UncoveredTy(param));
218244
}
@@ -224,11 +250,11 @@ fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
224250

225251
// Otherwise, enforce invariant that there are no type
226252
// parameters reachable.
227-
if !infer_is_local.0 {
228-
if let Some(param) = input_ty.walk().find(|t| is_type_parameter(t)) {
229-
debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
230-
return Err(OrphanCheckErr::UncoveredTy(param));
231-
}
253+
if let Some(param) = input_ty.walk()
254+
.find(|t| is_possibly_remote_type(t, infer_is_local))
255+
{
256+
debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
257+
return Err(OrphanCheckErr::UncoveredTy(param));
232258
}
233259
}
234260

@@ -250,7 +276,7 @@ fn uncovered_tys<'tcx>(tcx: TyCtxt, ty: Ty<'tcx>, infer_is_local: InferIsLocal)
250276
}
251277
}
252278

253-
fn is_type_parameter(ty: Ty) -> bool {
279+
fn is_possibly_remote_type(ty: Ty, _infer_is_local: InferIsLocal) -> bool {
254280
match ty.sty {
255281
ty::TyProjection(..) | ty::TyParam(..) => true,
256282
_ => false,
@@ -273,7 +299,15 @@ fn fundamental_ty(tcx: TyCtxt, ty: Ty) -> bool {
273299
}
274300
}
275301

276-
fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal)-> bool {
302+
fn def_id_is_local(def_id: DefId, infer_is_local: InferIsLocal) -> bool {
303+
match infer_is_local {
304+
InferIsLocal::Yes => false,
305+
InferIsLocal::No |
306+
InferIsLocal::BrokenYes => def_id.is_local()
307+
}
308+
}
309+
310+
fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal) -> bool {
277311
debug!("ty_is_local_constructor({:?})", ty);
278312

279313
match ty.sty {
@@ -296,20 +330,19 @@ fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal)-> bool {
296330
false
297331
}
298332

299-
ty::TyInfer(..) => {
300-
infer_is_local.0
301-
}
302-
303-
ty::TyAdt(def, _) => {
304-
def.did.is_local()
305-
}
333+
ty::TyInfer(..) => match infer_is_local {
334+
InferIsLocal::No => false,
335+
InferIsLocal::Yes |
336+
InferIsLocal::BrokenYes => true
337+
},
306338

307-
ty::TyForeign(did) => {
308-
did.is_local()
309-
}
339+
ty::TyAdt(def, _) => def_id_is_local(def.did, infer_is_local),
340+
ty::TyForeign(did) => def_id_is_local(did, infer_is_local),
310341

311342
ty::TyDynamic(ref tt, ..) => {
312-
tt.principal().map_or(false, |p| p.def_id().is_local())
343+
tt.principal().map_or(false, |p| {
344+
def_id_is_local(p.def_id(), infer_is_local)
345+
})
313346
}
314347

315348
ty::TyError => {

‎src/librustc/traits/select.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -814,7 +814,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
814814
// terms of `Fn` etc, but we could probably make this more
815815
// precise still.
816816
let unbound_input_types = stack.fresh_trait_ref.input_types().any(|ty| ty.is_fresh());
817-
if unbound_input_types && self.intercrate {
817+
if unbound_input_types && self.intercrate && false {
818818
debug!("evaluate_stack({:?}) --> unbound argument, intercrate --> ambiguous",
819819
stack.fresh_trait_ref);
820820
// Heuristics: show the diagnostics when there are no candidates in crate.
@@ -1221,7 +1221,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
12211221
// bound regions
12221222
let trait_ref = predicate.skip_binder().trait_ref;
12231223

1224-
coherence::trait_ref_is_knowable(self.tcx(), trait_ref)
1224+
coherence::trait_ref_is_knowable(self.tcx(), trait_ref, false).is_none()
12251225
}
12261226

12271227
/// Returns true if the global caches can be used.

‎src/librustc/ty/maps/mod.rs

+3
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,9 @@ define_maps! { <'tcx>
169169
/// The result of unsafety-checking this def-id.
170170
[] fn unsafety_check_result: UnsafetyCheckResult(DefId) -> mir::UnsafetyCheckResult,
171171

172+
/// HACK: when evaluated, this reports a "unsafe derive on repr(packed)" error
173+
[] fn unsafe_derive_on_repr_packed: UnsafeDeriveOnReprPacked(DefId) -> (),
174+
172175
/// The signature of functions and closures.
173176
[] fn fn_sig: FnSignature(DefId) -> ty::PolyFnSig<'tcx>,
174177

‎src/librustc/ty/maps/plumbing.rs

+1
Original file line numberDiff line numberDiff line change
@@ -768,6 +768,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
768768
DepKind::BorrowCheck => { force!(borrowck, def_id!()); }
769769
DepKind::MirBorrowCheck => { force!(mir_borrowck, def_id!()); }
770770
DepKind::UnsafetyCheckResult => { force!(unsafety_check_result, def_id!()); }
771+
DepKind::UnsafeDeriveOnReprPacked => { force!(unsafe_derive_on_repr_packed, def_id!()); }
771772
DepKind::Reachability => { force!(reachable_set, LOCAL_CRATE); }
772773
DepKind::MirKeys => { force!(mir_keys, LOCAL_CRATE); }
773774
DepKind::CrateVariances => { force!(crate_variances, LOCAL_CRATE); }

‎src/librustc_lint/lib.rs

+5
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
243243
id: LintId::of(LATE_BOUND_LIFETIME_ARGUMENTS),
244244
reference: "issue #42868 <https://github.com/rust-lang/rust/issues/42868>",
245245
},
246+
FutureIncompatibleInfo {
247+
id: LintId::of(SAFE_PACKED_BORROWS),
248+
reference: "issue #46043 <https://github.com/rust-lang/rust/issues/46043>",
249+
},
250+
246251
]);
247252

248253
// Register renamed and removed lints

‎src/librustc_mir/shim.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ use syntax_pos::Span;
2727
use std::fmt;
2828
use std::iter;
2929

30-
use transform::{add_call_guards, no_landing_pads, simplify};
30+
use transform::{add_moves_for_packed_drops, add_call_guards};
31+
use transform::{no_landing_pads, simplify};
3132
use util::elaborate_drops::{self, DropElaborator, DropStyle, DropFlagMode};
3233
use util::patch::MirPatch;
3334

@@ -114,6 +115,8 @@ fn make_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
114115
}
115116
};
116117
debug!("make_shim({:?}) = untransformed {:?}", instance, result);
118+
add_moves_for_packed_drops::add_moves_for_packed_drops(
119+
tcx, &mut result, instance.def_id());
117120
no_landing_pads::no_landing_pads(tcx, &mut result);
118121
simplify::simplify_cfg(&mut result);
119122
add_call_guards::CriticalCallEdges.add_call_guards(&mut result);

0 commit comments

Comments
 (0)