Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not borrow non-Sync data in constants #54424

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,7 @@ define_dep_nodes!( <'tcx>
[] IsCopy { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
[] IsSized { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
[] IsFreeze { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
[] IsSync { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
[] NeedsDrop { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },
[] Layout { param_env: ParamEnvAnd<'tcx, Ty<'tcx>> },

Expand Down
8 changes: 3 additions & 5 deletions src/librustc/middle/resolve_lifetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,6 @@ struct ElisionFailureInfo {

type ScopeRef<'a> = &'a Scope<'a>;

const ROOT_SCOPE: ScopeRef<'static> = &Scope::Root;

pub fn provide(providers: &mut ty::query::Providers<'_>) {
*providers = ty::query::Providers {
resolve_lifetimes,
Expand Down Expand Up @@ -431,7 +429,7 @@ fn krate<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>) -> NamedRegionMap {
let mut visitor = LifetimeContext {
tcx,
map: &mut map,
scope: ROOT_SCOPE,
scope: &Scope::Root,
trait_ref_hack: false,
is_in_fn_syntax: false,
labels_in_fn: vec![],
Expand Down Expand Up @@ -490,7 +488,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
// No lifetime parameters, but implied 'static.
let scope = Scope::Elision {
elide: Elide::Exact(Region::Static),
s: ROOT_SCOPE,
s: &Scope::Root,
};
self.with(scope, |_, this| intravisit::walk_item(this, item));
}
Expand Down Expand Up @@ -534,7 +532,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
next_early_index: index + type_count,
abstract_type_parent: true,
track_lifetime_uses,
s: ROOT_SCOPE,
s: &Scope::Root,
};
self.with(scope, |old_scope, this| {
this.check_lifetime_params(old_scope, &generics.params);
Expand Down
6 changes: 6 additions & 0 deletions src/librustc/ty/query/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ impl<'tcx> QueryDescription<'tcx> for queries::is_freeze_raw<'tcx> {
}
}

impl<'tcx> QueryDescription<'tcx> for queries::is_sync_raw<'tcx> {
fn describe(_tcx: TyCtxt<'_, '_, '_>, env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> String {
format!("computing whether `{}` is `Sync`", env.value)
}
}

impl<'tcx> QueryDescription<'tcx> for queries::needs_drop_raw<'tcx> {
fn describe(_tcx: TyCtxt<'_, '_, '_>, env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> String {
format!("computing whether `{}` needs drop", env.value)
Expand Down
5 changes: 5 additions & 0 deletions src/librustc/ty/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ define_queries! { <'tcx>
[] fn is_copy_raw: is_copy_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
[] fn is_sized_raw: is_sized_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
[] fn is_freeze_raw: is_freeze_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
[] fn is_sync_raw: is_sync_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
[] fn needs_drop_raw: needs_drop_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool,
[] fn layout_raw: layout_dep_node(ty::ParamEnvAnd<'tcx, Ty<'tcx>>)
-> Result<&'tcx ty::layout::LayoutDetails,
Expand Down Expand Up @@ -767,6 +768,10 @@ fn is_freeze_dep_node<'tcx>(param_env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> DepCo
DepConstructor::IsFreeze { param_env }
}

fn is_sync_dep_node<'tcx>(param_env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> DepConstructor<'tcx> {
DepConstructor::IsSync { param_env }
}

fn needs_drop_dep_node<'tcx>(param_env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> DepConstructor<'tcx> {
DepConstructor::NeedsDrop { param_env }
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc/ty/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
DepKind::IsCopy |
DepKind::IsSized |
DepKind::IsFreeze |
DepKind::IsSync |
DepKind::NeedsDrop |
DepKind::Layout |
DepKind::ConstEval |
Expand Down
69 changes: 68 additions & 1 deletion src/librustc/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use hir::map::DefPathData;
use hir::{self, Node};
use ich::NodeIdHashingMode;
use traits::{self, ObligationCause};
use ty::{self, Ty, TyCtxt, GenericParamDefKind, TypeFoldable};
use ty::{self, Ty, TyCtxt, GenericParamDefKind, TypeFoldable, Predicate};
use ty::subst::{Substs, UnpackedKind};
use ty::query::TyCtxtAt;
use ty::TyKind::*;
Expand Down Expand Up @@ -655,6 +655,14 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
tcx.at(span).is_freeze_raw(param_env.and(self))
}

pub fn is_sync(&'tcx self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
span: Span)-> bool
{
tcx.at(span).is_sync_raw(param_env.and(self))
}

/// If `ty.needs_drop(...)` returns `true`, then `ty` is definitely
/// non-copy and *might* have a destructor attached; if it returns
/// `false`, then `ty` definitely has no destructor (i.e. no drop glue).
Expand Down Expand Up @@ -853,6 +861,50 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
debug!("is_type_representable: {:?} is {:?}", self, r);
r
}

/// Returns a potentially different type that
/// can be used to check whether a 0-argument constructor of this type is
/// a `Sync` value.
/// Concretely, some type parameters get replaced by `()`. We assume that
/// if a type parameter has no bounds (except for `Sized`), and if the same
/// type is `Sync` for that type parameter replaced by `()`, then the constructor
/// valie is also `Sync` at the original type. This is a kind of parametricity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valie -> value

/// assumption.
/// For now, we only do anything if *no* type parameter has any bound other than
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anything -> something

/// `Sized`. That's just simpler to check.
pub fn generalize_for_value_based_sync(
&'tcx self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
) -> Ty<'tcx> {
match self.sty {
ty::Adt(def, substs) => {
for (bound, _) in def.predicates(tcx).predicates.iter() {
match bound {
Predicate::Trait(pred)
if Some(pred.def_id()) == tcx.lang_items().sized_trait() =>
{
// A `Sized` bound. Ignore.
continue;
}
_ => {}
}
// There is a relevant bound. Do not do anything.
debug!("Type {:?} not generalizable because of bound {:?}", self, bound);
return self;
}
// If we got here, there are no bounds and we can replace all
// type parameters by `()`.
let unit_substs = substs.iter()
.map(|param| match param.unpack() {
UnpackedKind::Lifetime(_) => param.clone(),
UnpackedKind::Type(_) => tcx.mk_unit().into(),
});
let unit_substs = tcx.mk_substs(unit_substs);
tcx.mk_ty(ty::Adt(def, unit_substs))
}
_ => self, // don't change
}
}
}

fn is_copy_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
Expand Down Expand Up @@ -897,6 +949,20 @@ fn is_freeze_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
DUMMY_SP))
}

fn is_sync_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>)
-> bool
{
let (param_env, ty) = query.into_parts();
let trait_def_id = tcx.require_lang_item(lang_items::SyncTraitLangItem);
tcx.infer_ctxt()
.enter(|infcx| traits::type_known_to_meet_bound(&infcx,
param_env,
ty,
trait_def_id,
DUMMY_SP))
}

fn needs_drop_raw<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>)
-> bool
Expand Down Expand Up @@ -1044,6 +1110,7 @@ pub fn provide(providers: &mut ty::query::Providers<'_>) {
is_copy_raw,
is_sized_raw,
is_freeze_raw,
is_sync_raw,
needs_drop_raw,
..*providers
};
Expand Down
64 changes: 44 additions & 20 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,10 @@ bitflags! {
// they have none of these qualifications, with
// the exception of `STATIC_REF` (in statics only).
struct Qualif: u8 {
// Constant containing interior mutability (UnsafeCell).
const MUTABLE_INTERIOR = 1 << 0;
// Constant containing interior mutability (UnsafeCell) or non-Sync data.
// Both of these prevent sound re-use of the same global static memory for
// the same data across multiple threads.
const UNSHAREABLE_INTERIOR = 1 << 0;

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

// Const items can only have MUTABLE_INTERIOR
// Const items can only have UNSHAREABLE_INTERIOR
// and NOT_PROMOTABLE without producing an error.
const CONST_ERROR = !Qualif::MUTABLE_INTERIOR.bits &
const CONST_ERROR = !Qualif::UNSHAREABLE_INTERIOR.bits &
!Qualif::NOT_PROMOTABLE.bits;
}
}
Expand All @@ -75,8 +77,8 @@ impl<'a, 'tcx> Qualif {
fn restrict(&mut self, ty: Ty<'tcx>,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
param_env: ty::ParamEnv<'tcx>) {
if ty.is_freeze(tcx, param_env, DUMMY_SP) {
*self = *self - Qualif::MUTABLE_INTERIOR;
if ty.is_freeze(tcx, param_env, DUMMY_SP) && ty.is_sync(tcx, param_env, DUMMY_SP) {
*self = *self - Qualif::UNSHAREABLE_INTERIOR;
}
if !ty.needs_drop(tcx, param_env) {
*self = *self - Qualif::NEEDS_DROP;
Expand Down Expand Up @@ -206,7 +208,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {

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

Expand Down Expand Up @@ -679,18 +681,19 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
// Constants cannot be borrowed if they contain interior mutability as
// it means that our "silent insertion of statics" could change
// initializer values (very bad).
if self.qualif.contains(Qualif::MUTABLE_INTERIOR) {
// A reference of a MUTABLE_INTERIOR place is instead
if self.qualif.contains(Qualif::UNSHAREABLE_INTERIOR) {
// A reference of a UNSHAREABLE_INTERIOR place is instead
// NOT_CONST (see `if forbidden_mut` below), to avoid
// duplicate errors (from reborrowing, for example).
self.qualif = self.qualif - Qualif::MUTABLE_INTERIOR;
self.qualif = self.qualif - Qualif::UNSHAREABLE_INTERIOR;
if self.mode != Mode::Fn {
span_err!(self.tcx.sess, self.span, E0492,
"cannot borrow a constant which may contain \
interior mutability, create a static instead");
interior mutability or non-`Sync` data. If your \
data is `Sync`, create a static instead");
}
} else {
// We allow immutable borrows of frozen data.
// We allow immutable borrows of frozen non-Sync data.
forbidden_mut = false;
}
}
Expand All @@ -712,11 +715,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
if self.mir.local_kind(local) == LocalKind::Temp {
if let Some(qualif) = self.local_qualif[local] {
// `forbidden_mut` is false, so we can safely ignore
// `MUTABLE_INTERIOR` from the local's qualifications.
// `UNSHAREABLE_INTERIOR` from the local's qualifications.
// This allows borrowing fields which don't have
// `MUTABLE_INTERIOR`, from a type that does, e.g.:
// `UNSHAREABLE_INTERIOR`, from a type that does, e.g.:
// `let _: &'static _ = &(Cell::new(1), 2).1;`
if (qualif - Qualif::MUTABLE_INTERIOR).is_empty() {
if (qualif - Qualif::UNSHAREABLE_INTERIOR).is_empty() {
self.promotion_candidates.push(candidate);
}
}
Expand Down Expand Up @@ -788,16 +791,36 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
}
}

Rvalue::Aggregate(ref kind, _) => {
Rvalue::Aggregate(ref kind, ref args) => {
if let AggregateKind::Adt(def, ..) = **kind {
if def.has_dtor(self.tcx) {
self.add(Qualif::NEEDS_DROP);
}

if Some(def.did) == self.tcx.lang_items().unsafe_cell_type() {
let ty = rvalue.ty(self.mir, self.tcx);
self.add_type(ty);
assert!(self.qualif.contains(Qualif::MUTABLE_INTERIOR));
let ty = rvalue.ty(self.mir, self.tcx);
// Value-based reasoning:
// We are looking at a concrete type constructor, and we know
// the only way to construct "fresh" non-Freeze data is `UnsafeCell`.
// So we can check for that instead of `Freeze`.
let freeze = Some(def.did) != self.tcx.lang_items().unsafe_cell_type();
// For Sync, we generally have to fall back on the type, but have a
// special exception for 0-argument ADT cosntructors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cosntructors -> constructors

let sync = {
let generalized_ty = if args.len() == 0 {
// A type cosntructor with no arguments is directly a value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cosntructor

// We generalize its type before checking Sync to allow
// `None` to pass no matter the type parameter.
ty.generalize_for_value_based_sync(self.tcx)
} else {
ty
};
generalized_ty.is_sync(self.tcx, self.param_env, DUMMY_SP)
};
if !(freeze && sync)
{
// Not freeze and sync? Be careful.
debug!("Rvalue {:?} has unsharable interior", rvalue);
self.add(Qualif::UNSHAREABLE_INTERIOR);
}
}
}
Expand Down Expand Up @@ -1248,6 +1271,7 @@ impl MirPass for QualifyAndPromoteConstants {
}
}
let ty = mir.return_ty();
// Not using ty.is_sync() to get the right kind of error message
tcx.infer_ctxt().enter(|infcx| {
let param_env = ty::ParamEnv::empty();
let cause = traits::ObligationCause::new(mir.span, id, traits::SharedStatic);
Expand Down
37 changes: 33 additions & 4 deletions src/librustc_passes/rvalue_promotion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> {
debug!("type_promotability({})", ty);

if ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) &&
ty.is_sync(self.tcx, self.param_env, DUMMY_SP) &&
!ty.needs_drop(self.tcx, self.param_env) {
Promotable
} else {
Expand Down Expand Up @@ -375,8 +376,20 @@ fn check_expr_kind<'a, 'tcx>(
hir::ExprKind::Path(ref qpath) => {
let def = v.tables.qpath_def(qpath, e.hir_id);
match def {
Def::VariantCtor(..) | Def::StructCtor(..) |
Def::Fn(..) | Def::Method(..) | Def::SelfCtor(..) => Promotable,
Def::Fn(..) | Def::Method(..) |
Def::VariantCtor(..) | Def::StructCtor(..) | Def::SelfCtor(..) => {
// This is a 0-argument constructor. Do some value-based-reasoning.
let ty = v.tables.expr_ty(e);
let ty = ty.generalize_for_value_based_sync(v.tcx);
if ty.is_sync(v.tcx, v.param_env, DUMMY_SP) {
Promotable
} else {
debug!("Constructor ({:?}) not promotable because generalized type \
({:?}) is not Sync",
e, ty);
NotPromotable
}
}

// References to a static that are themselves within a static
// are inherently promotable with the exception
Expand Down Expand Up @@ -442,7 +455,16 @@ fn check_expr_kind<'a, 'tcx>(
let def_result = match def {
Def::StructCtor(_, CtorKind::Fn) |
Def::VariantCtor(_, CtorKind::Fn) |
Def::SelfCtor(..) => Promotable,
Def::SelfCtor(..) => {
let ty = v.tables.expr_ty(e);
if ty.is_sync(v.tcx, v.param_env, DUMMY_SP) {
Promotable
} else {
debug!("Constructor ({:?}) not promotable because type ({:?}) is not Sync",
e, ty);
NotPromotable
}
}
Def::Fn(did) => {
v.handle_const_fn_call(did, node_ty, e.span)
}
Expand Down Expand Up @@ -485,12 +507,18 @@ fn check_expr_kind<'a, 'tcx>(
if let Some(ref expr) = *option_expr {
struct_result &= v.check_expr(&expr);
}
if let ty::Adt(adt, ..) = v.tables.expr_ty(e).sty {
let ty = v.tables.expr_ty(e);
if let ty::Adt(adt, ..) = ty.sty {
// unsafe_cell_type doesn't necessarily exist with no_core
if Some(adt.did) == v.tcx.lang_items().unsafe_cell_type() {
return NotPromotable;
}
}
if !ty.is_sync(v.tcx, v.param_env, DUMMY_SP) {
debug!("Constructor ({:?}) not promotable because type ({:?}) is not Sync",
e, ty);
return NotPromotable;
}
struct_result
}

Expand All @@ -509,6 +537,7 @@ fn check_expr_kind<'a, 'tcx>(
if v.tcx.with_freevars(e.id, |fv| !fv.is_empty()) {
NotPromotable
} else {
// Nothing captured, so the environment type is trivially Freeze+Sync
nested_body_promotable
}
}
Expand Down
Loading