Skip to content

Commit e4686a6

Browse files
borsbors
committed
Auto merge of #54424 - RalfJung:sync-promote, r=<try>
WIP: do not borrow non-Sync data in constants We cannot share that data across threads. non-Sync is as bad as non-Freeze in that regard. This is currently WIP because it ignores a test that is broken by #54419. But it is good enough ti get crater going. Fixes #49206. Cc @eddyb @nikomatsakis
2 parents 2fa1390 + f6a5c9c commit e4686a6

18 files changed

+269
-25
lines changed

Diff for: ‎src/librustc/dep_graph/dep_node.rs

+1
Original file line numberDiff line numberDiff line change
@@ -538,6 +538,7 @@ define_dep_nodes!( <'tcx>
538538
[] IsCopy { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
539539
[] IsSized { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
540540
[] IsFreeze { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
541+
[] IsSync { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
541542
[] NeedsDrop { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
542543
[] Layout { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
543544

Diff for: ‎src/librustc/ty/query/config.rs

+6
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,12 @@ impl<'tcx> QueryDescription<'tcx> for queries::is_freeze_raw<'tcx> {
179179
}
180180
}
181181

182+
impl<'tcx> QueryDescription<'tcx> for queries::is_sync_raw<'tcx> {
183+
fn describe(_tcx: TyCtxt, env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> String {
184+
format!("computing whether `{}` is `Sync`", env.value)
185+
}
186+
}
187+
182188
impl<'tcx> QueryDescription<'tcx> for queries::needs_drop_raw<'tcx> {
183189
fn describe(_tcx: TyCtxt, env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> String {
184190
format!("computing whether `{}` needs drop", env.value)

Diff for: ‎src/librustc/ty/query/mod.rs

+5
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,7 @@ define_queries! { <'tcx>
366366
[] fn is_copy_raw: is_copy_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
367367
[] fn is_sized_raw: is_sized_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
368368
[] fn is_freeze_raw: is_freeze_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
369+
[] fn is_sync_raw: is_sync_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
369370
[] fn needs_drop_raw: needs_drop_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
370371
[] fn layout_raw: layout_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>)
371372
-> Result<&'tcx ty::layout::LayoutDetails,
@@ -767,6 +768,10 @@ fn is_freeze_dep_node<'tcx>(param_env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> DepCo
767768
DepConstructor::IsFreeze { param_env }
768769
}
769770

771+
fn is_sync_dep_node<'tcx>(param_env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> DepConstructor<'tcx> {
772+
DepConstructor::IsSync { param_env }
773+
}
774+
770775
fn needs_drop_dep_node<'tcx>(param_env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> DepConstructor<'tcx> {
771776
DepConstructor::NeedsDrop { param_env }
772777
}

Diff for: ‎src/librustc/ty/query/plumbing.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1057,6 +1057,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
10571057
DepKind::IsCopy |
10581058
DepKind::IsSized |
10591059
DepKind::IsFreeze |
1060+
DepKind::IsSync |
10601061
DepKind::NeedsDrop |
10611062
DepKind::Layout |
10621063
DepKind::ConstEval |

Diff for: ‎src/librustc/ty/util.rs

+23
Original file line numberDiff line numberDiff line change
@@ -655,6 +655,14 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
655655
tcx.at(span).is_freeze_raw(param_env.and(self))
656656
}
657657

658+
pub fn is_sync(&'tcx self,
659+
tcx: TyCtxt<'a, 'tcx, 'tcx>,
660+
param_env: ty::ParamEnv<'tcx>,
661+
span: Span)-> bool
662+
{
663+
tcx.at(span).is_sync_raw(param_env.and(self))
664+
}
665+
658666
/// If `ty.needs_drop(...)` returns `true`, then `ty` is definitely
659667
/// non-copy and *might* have a destructor attached; if it returns
660668
/// `false`, then `ty` definitely has no destructor (i.e. no drop glue).
@@ -897,6 +905,20 @@ fn is_freeze_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
897905
DUMMY_SP))
898906
}
899907

908+
fn is_sync_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
909+
query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>)
910+
-> bool
911+
{
912+
let (param_env, ty) = query.into_parts();
913+
let trait_def_id = tcx.require_lang_item(lang_items::SyncTraitLangItem);
914+
tcx.infer_ctxt()
915+
.enter(|infcx| traits::type_known_to_meet_bound(&infcx,
916+
param_env,
917+
ty,
918+
trait_def_id,
919+
DUMMY_SP))
920+
}
921+
900922
fn needs_drop_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
901923
query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>)
902924
-> bool
@@ -1044,6 +1066,7 @@ pub fn provide(providers: &mut ty::query::Providers) {
10441066
is_copy_raw,
10451067
is_sized_raw,
10461068
is_freeze_raw,
1069+
is_sync_raw,
10471070
needs_drop_raw,
10481071
..*providers
10491072
};

Diff for: ‎src/librustc_mir/transform/qualify_consts.rs

+30-19
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,10 @@ bitflags! {
4646
// they have none of these qualifications, with
4747
// the exception of `STATIC_REF` (in statics only).
4848
struct Qualif: u8 {
49-
// Constant containing interior mutability (UnsafeCell).
50-
const MUTABLE_INTERIOR = 1 << 0;
49+
// Constant containing interior mutability (UnsafeCell) or non-Sync data.
50+
// Both of these prevent sound re-use of the same global static memory for
51+
// the same data across multiple threads.
52+
const UNSHAREABLE_INTERIOR = 1 << 0;
5153

5254
// Constant containing an ADT that implements Drop.
5355
const NEEDS_DROP = 1 << 1;
@@ -63,9 +65,9 @@ bitflags! {
6365
// promote_consts decided they weren't simple enough.
6466
const NOT_PROMOTABLE = 1 << 4;
6567

66-
// Const items can only have MUTABLE_INTERIOR
68+
// Const items can only have UNSHAREABLE_INTERIOR
6769
// and NOT_PROMOTABLE without producing an error.
68-
const CONST_ERROR = !Qualif::MUTABLE_INTERIOR.bits &
70+
const CONST_ERROR = !Qualif::UNSHAREABLE_INTERIOR.bits &
6971
!Qualif::NOT_PROMOTABLE.bits;
7072
}
7173
}
@@ -75,8 +77,8 @@ impl<'a, 'tcx> Qualif {
7577
fn restrict(&mut self, ty: Ty<'tcx>,
7678
tcx: TyCtxt<'a, 'tcx, 'tcx>,
7779
param_env: ty::ParamEnv<'tcx>) {
78-
if ty.is_freeze(tcx, param_env, DUMMY_SP) {
79-
*self = *self - Qualif::MUTABLE_INTERIOR;
80+
if ty.is_freeze(tcx, param_env, DUMMY_SP) && ty.is_sync(tcx, param_env, DUMMY_SP) {
81+
*self = *self - Qualif::UNSHAREABLE_INTERIOR;
8082
}
8183
if !ty.needs_drop(tcx, param_env) {
8284
*self = *self - Qualif::NEEDS_DROP;
@@ -206,7 +208,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
206208

207209
/// Add the given type's qualification to self.qualif.
208210
fn add_type(&mut self, ty: Ty<'tcx>) {
209-
self.add(Qualif::MUTABLE_INTERIOR | Qualif::NEEDS_DROP);
211+
self.add(Qualif::UNSHAREABLE_INTERIOR | Qualif::NEEDS_DROP);
210212
self.qualif.restrict(ty, self.tcx, self.param_env);
211213
}
212214

@@ -679,18 +681,19 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
679681
// Constants cannot be borrowed if they contain interior mutability as
680682
// it means that our "silent insertion of statics" could change
681683
// initializer values (very bad).
682-
if self.qualif.contains(Qualif::MUTABLE_INTERIOR) {
683-
// A reference of a MUTABLE_INTERIOR place is instead
684+
if self.qualif.contains(Qualif::UNSHAREABLE_INTERIOR) {
685+
// A reference of a UNSHAREABLE_INTERIOR place is instead
684686
// NOT_CONST (see `if forbidden_mut` below), to avoid
685687
// duplicate errors (from reborrowing, for example).
686-
self.qualif = self.qualif - Qualif::MUTABLE_INTERIOR;
688+
self.qualif = self.qualif - Qualif::UNSHAREABLE_INTERIOR;
687689
if self.mode != Mode::Fn {
688690
span_err!(self.tcx.sess, self.span, E0492,
689691
"cannot borrow a constant which may contain \
690-
interior mutability, create a static instead");
692+
interior mutability or non-`Sync` data. If your \
693+
data is `Sync`, create a static instead");
691694
}
692695
} else {
693-
// We allow immutable borrows of frozen data.
696+
// We allow immutable borrows of frozen non-Sync data.
694697
forbidden_mut = false;
695698
}
696699
}
@@ -712,11 +715,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
712715
if self.mir.local_kind(local) == LocalKind::Temp {
713716
if let Some(qualif) = self.local_qualif[local] {
714717
// `forbidden_mut` is false, so we can safely ignore
715-
// `MUTABLE_INTERIOR` from the local's qualifications.
718+
// `UNSHAREABLE_INTERIOR` from the local's qualifications.
716719
// This allows borrowing fields which don't have
717-
// `MUTABLE_INTERIOR`, from a type that does, e.g.:
720+
// `UNSHAREABLE_INTERIOR`, from a type that does, e.g.:
718721
// `let _: &'static _ = &(Cell::new(1), 2).1;`
719-
if (qualif - Qualif::MUTABLE_INTERIOR).is_empty() {
722+
if (qualif - Qualif::UNSHAREABLE_INTERIOR).is_empty() {
720723
self.promotion_candidates.push(candidate);
721724
}
722725
}
@@ -794,10 +797,17 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
794797
self.add(Qualif::NEEDS_DROP);
795798
}
796799

797-
if Some(def.did) == self.tcx.lang_items().unsafe_cell_type() {
798-
let ty = rvalue.ty(self.mir, self.tcx);
799-
self.add_type(ty);
800-
assert!(self.qualif.contains(Qualif::MUTABLE_INTERIOR));
800+
// We are looking at a concrete type constructor, and we know
801+
// the only way to construct "fresh" non-Freeze data is `UnsafeCell`.
802+
// So we can check for that instead of `Freeze`.
803+
// There is no similar shortcut for Sync, though.
804+
let ty = rvalue.ty(self.mir, self.tcx);
805+
let freeze = Some(def.did) != self.tcx.lang_items().unsafe_cell_type();
806+
let sync = ty.is_sync(self.tcx, self.param_env, DUMMY_SP);
807+
if !(freeze && sync)
808+
{
809+
// Not freeze and sync? Be careful.
810+
self.add(Qualif::UNSHAREABLE_INTERIOR);
801811
}
802812
}
803813
}
@@ -1247,6 +1257,7 @@ impl MirPass for QualifyAndPromoteConstants {
12471257
}
12481258
}
12491259
let ty = mir.return_ty();
1260+
// Not using ty.is_sync() to get the right kind of error message
12501261
tcx.infer_ctxt().enter(|infcx| {
12511262
let param_env = ty::ParamEnv::empty();
12521263
let cause = traits::ObligationCause::new(mir.span, id, traits::SharedStatic);

Diff for: ‎src/librustc_passes/rvalue_promotion.rs

+6
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> {
153153
debug!("type_promotability({})", ty);
154154

155155
if ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) &&
156+
ty.is_sync(self.tcx, self.param_env, DUMMY_SP) &&
156157
!ty.needs_drop(self.tcx, self.param_env) {
157158
Promotable
158159
} else {
@@ -289,6 +290,11 @@ impl<'a, 'tcx> CheckCrateVisitor<'a, 'tcx> {
289290
let mut outer = check_expr_kind(self, ex, node_ty);
290291
outer &= check_adjustments(self, ex);
291292

293+
// Avoid non-Sync types
294+
if !self.tables.expr_ty(ex).is_sync(self.tcx, self.param_env, DUMMY_SP) {
295+
outer = NotPromotable;
296+
}
297+
292298
// Handle borrows on (or inside the autorefs of) this expression.
293299
if self.mut_rvalue_borrows.remove(&ex.id) {
294300
outer = NotPromotable

Diff for: ‎src/test/ui/consts/const-eval/dont_promote_non_sync.rs

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
#![feature(optin_builtin_traits)]
2+
3+
struct Foo;
4+
impl !Sync for Foo {}
5+
6+
struct Bar(i32);
7+
impl !Sync for Bar {}
8+
9+
struct Baz { field: i32 }
10+
impl !Sync for Baz {}
11+
12+
enum Bla { T(i32), S { field: i32 } }
13+
impl !Sync for Bla {}
14+
15+
// Known values of the given types
16+
fn mk_foo() -> &'static Foo { &Foo } //~ ERROR does not live long enough
17+
fn mk_bar() -> &'static Bar { &Bar(0) } //~ ERROR does not live long enough
18+
fn mk_baz() -> &'static Baz { &Baz { field: 0 } } //~ ERROR does not live long enough
19+
fn mk_bla_t() -> &'static Bla { &Bla::T(0) } //~ ERROR does not live long enough
20+
fn mk_bla_s() -> &'static Bla { &Bla::S { field: 0 } } //~ ERROR does not live long enough
21+
22+
// Unknown values of the given types (test a ZST and a non-ZST)
23+
trait FooT { const C: Foo; }
24+
fn mk_foo2<T: FooT>() -> &'static Foo { &T::C } //~ ERROR does not live long enough
25+
26+
trait BarT { const C: Bar; }
27+
fn mk_bar2<T: BarT>() -> &'static Bar { &T::C } //~ ERROR does not live long enough
28+
29+
fn main() {}

Diff for: ‎src/test/ui/consts/const-eval/dont_promote_non_sync.stderr

+73
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
error[E0597]: borrowed value does not live long enough
2+
--> $DIR/dont_promote_non_sync.rs:16:32
3+
|
4+
LL | fn mk_foo() -> &'static Foo { &Foo } //~ ERROR does not live long enough
5+
| ^^^ - temporary value only lives until here
6+
| |
7+
| temporary value does not live long enough
8+
|
9+
= note: borrowed value must be valid for the static lifetime...
10+
11+
error[E0597]: borrowed value does not live long enough
12+
--> $DIR/dont_promote_non_sync.rs:17:32
13+
|
14+
LL | fn mk_bar() -> &'static Bar { &Bar(0) } //~ ERROR does not live long enough
15+
| ^^^^^^ - temporary value only lives until here
16+
| |
17+
| temporary value does not live long enough
18+
|
19+
= note: borrowed value must be valid for the static lifetime...
20+
21+
error[E0597]: borrowed value does not live long enough
22+
--> $DIR/dont_promote_non_sync.rs:18:32
23+
|
24+
LL | fn mk_baz() -> &'static Baz { &Baz { field: 0 } } //~ ERROR does not live long enough
25+
| ^^^^^^^^^^^^^^^^ - temporary value only lives until here
26+
| |
27+
| temporary value does not live long enough
28+
|
29+
= note: borrowed value must be valid for the static lifetime...
30+
31+
error[E0597]: borrowed value does not live long enough
32+
--> $DIR/dont_promote_non_sync.rs:19:34
33+
|
34+
LL | fn mk_bla_t() -> &'static Bla { &Bla::T(0) } //~ ERROR does not live long enough
35+
| ^^^^^^^^^ - temporary value only lives until here
36+
| |
37+
| temporary value does not live long enough
38+
|
39+
= note: borrowed value must be valid for the static lifetime...
40+
41+
error[E0597]: borrowed value does not live long enough
42+
--> $DIR/dont_promote_non_sync.rs:20:34
43+
|
44+
LL | fn mk_bla_s() -> &'static Bla { &Bla::S { field: 0 } } //~ ERROR does not live long enough
45+
| ^^^^^^^^^^^^^^^^^^^ - temporary value only lives until here
46+
| |
47+
| temporary value does not live long enough
48+
|
49+
= note: borrowed value must be valid for the static lifetime...
50+
51+
error[E0597]: borrowed value does not live long enough
52+
--> $DIR/dont_promote_non_sync.rs:24:42
53+
|
54+
LL | fn mk_foo2<T: FooT>() -> &'static Foo { &T::C } //~ ERROR does not live long enough
55+
| ^^^^ - temporary value only lives until here
56+
| |
57+
| temporary value does not live long enough
58+
|
59+
= note: borrowed value must be valid for the static lifetime...
60+
61+
error[E0597]: borrowed value does not live long enough
62+
--> $DIR/dont_promote_non_sync.rs:27:42
63+
|
64+
LL | fn mk_bar2<T: BarT>() -> &'static Bar { &T::C } //~ ERROR does not live long enough
65+
| ^^^^ - temporary value only lives until here
66+
| |
67+
| temporary value does not live long enough
68+
|
69+
= note: borrowed value must be valid for the static lifetime...
70+
71+
error: aborting due to 7 previous errors
72+
73+
For more information about this error, try `rustc --explain E0597`.

Diff for: ‎src/test/ui/consts/const-eval/non_sync_const.rs

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#![feature(optin_builtin_traits)]
2+
3+
struct Foo;
4+
impl !Sync for Foo {}
5+
6+
struct Bar(i32);
7+
impl !Sync for Bar {}
8+
9+
const FOO : &Foo = &Foo; //~ ERROR cannot borrow
10+
const FOO2 : Option<&Foo> = Some(&Foo); //~ ERROR cannot borrow
11+
//~^ ERROR borrowed value does not live long enough
12+
const FOO3 : &Option<Foo> = &Some(Foo); //~ ERROR cannot borrow
13+
14+
const BAR : &Bar = &Bar(42); //~ ERROR cannot borrow
15+
const BAR2 : Option<&Bar> = Some(&Bar(42)); //~ ERROR cannot borrow
16+
//~^ ERROR borrowed value does not live long enough
17+
const BAR3 : &Option<Bar> = &Some(Bar(42)); //~ ERROR cannot borrow
18+
19+
fn main() {}

0 commit comments

Comments
 (0)