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

UnsafeCell blocks niches inside its nested type from being available outside #99011

Merged
merged 19 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
8 changes: 3 additions & 5 deletions compiler/rustc_attr/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,6 @@ pub enum ReprAttr {
ReprSimd,
ReprTransparent,
ReprAlign(u32),
ReprNoNiche,
}

#[derive(Eq, PartialEq, Debug, Copy, Clone)]
Expand Down Expand Up @@ -904,7 +903,6 @@ pub fn parse_repr_attr(sess: &Session, attr: &Attribute) -> Vec<ReprAttr> {
sym::packed => Some(ReprPacked(1)),
sym::simd => Some(ReprSimd),
sym::transparent => Some(ReprTransparent),
sym::no_niche => Some(ReprNoNiche),
sym::align => {
let mut err = struct_span_err!(
diagnostic,
Expand Down Expand Up @@ -943,7 +941,7 @@ pub fn parse_repr_attr(sess: &Session, attr: &Attribute) -> Vec<ReprAttr> {
Ok(literal) => acc.push(ReprPacked(literal)),
Err(message) => literal_error = Some(message),
};
} else if matches!(name, sym::C | sym::simd | sym::transparent | sym::no_niche)
} else if matches!(name, sym::C | sym::simd | sym::transparent)
|| int_type_of_word(name).is_some()
{
recognised = true;
Expand Down Expand Up @@ -1001,7 +999,7 @@ pub fn parse_repr_attr(sess: &Session, attr: &Attribute) -> Vec<ReprAttr> {
} else {
if matches!(
meta_item.name_or_empty(),
sym::C | sym::simd | sym::transparent | sym::no_niche
sym::C | sym::simd | sym::transparent
) || int_type_of_word(meta_item.name_or_empty()).is_some()
{
recognised = true;
Expand Down Expand Up @@ -1039,7 +1037,7 @@ pub fn parse_repr_attr(sess: &Session, attr: &Attribute) -> Vec<ReprAttr> {
.emit();
} else if matches!(
meta_item.name_or_empty(),
sym::C | sym::simd | sym::transparent | sym::no_niche
sym::C | sym::simd | sym::transparent
) || int_type_of_word(meta_item.name_or_empty()).is_some()
{
recognised = true;
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory
}

if let Some(def) = mplace.layout.ty.ty_adt_def() {
if Some(def.did()) == self.ecx.tcx.lang_items().unsafe_cell_type() {
if def.is_unsafe_cell() {
// We are crossing over an `UnsafeCell`, we can mutate again. This means that
// References we encounter inside here are interned as pointing to mutable
// allocations.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
// Special check preventing `UnsafeCell` in the inner part of constants
if let Some(def) = op.layout.ty.ty_adt_def() {
if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { inner: true, .. }))
&& Some(def.did()) == self.ecx.tcx.lang_items().unsafe_cell_type()
&& def.is_unsafe_cell()
{
throw_validation_failure!(self.path, { "`UnsafeCell` in a `const`" });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ impl Qualif for HasMutInterior {
}

fn in_adt_inherently<'tcx>(
cx: &ConstCx<'_, 'tcx>,
_cx: &ConstCx<'_, 'tcx>,
adt: AdtDef<'tcx>,
_: SubstsRef<'tcx>,
) -> bool {
// Exactly one type, `UnsafeCell`, has the `HasMutInterior` qualif inherently.
// It arises structurally for all other types.
Some(adt.did()) == cx.tcx.lang_items().unsafe_cell_type()
adt.is_unsafe_cell()
}
}

Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_feature/src/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,6 @@ declare_features! (
(active, intrinsics, "1.0.0", None, None),
/// Allows using `#[lang = ".."]` attribute for linking items to special compiler logic.
(active, lang_items, "1.0.0", None, None),
/// Allows `#[repr(no_niche)]` (an implementation detail of `rustc`,
/// it is not on path for eventual stabilization).
(active, no_niche, "1.42.0", None, None),
/// Allows using `#[omit_gdb_pretty_printer_section]`.
(active, omit_gdb_pretty_printer_section, "1.5.0", None, None),
/// Allows using `#[prelude_import]` on glob `use` items.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ fn ty_is_known_nonnull<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, mode: CItemKi

// Types with a `#[repr(no_niche)]` attribute have their niche hidden.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// The attribute is used by the UnsafeCell for example (the only use so far).
if def.repr().hide_niche() {
if def.is_unsafe_cell() {
return false;
}

Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_middle/src/ty/adt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ bitflags! {
/// Indicates whether the variant list of this ADT is `#[non_exhaustive]`.
/// (i.e., this flag is never set unless this ADT is an enum).
const IS_VARIANT_LIST_NON_EXHAUSTIVE = 1 << 8;
/// Indicates whether the type is `UnsafeCell`.
const IS_UNSAFE_CELL = 1 << 9;
}
}

Expand Down Expand Up @@ -242,6 +244,9 @@ impl AdtDefData {
if Some(did) == tcx.lang_items().manually_drop() {
flags |= AdtFlags::IS_MANUALLY_DROP;
}
if Some(did) == tcx.lang_items().unsafe_cell_type() {
flags |= AdtFlags::IS_UNSAFE_CELL;
}

AdtDefData { did, variants, flags, repr }
}
Expand Down Expand Up @@ -328,6 +333,12 @@ impl<'tcx> AdtDef<'tcx> {
self.flags().contains(AdtFlags::IS_BOX)
}

/// Returns `true` if this is UnsafeCell<T>.
#[inline]
pub fn is_unsafe_cell(self) -> bool {
self.flags().contains(AdtFlags::IS_UNSAFE_CELL)
}

/// Returns `true` if this is `ManuallyDrop<T>`.
#[inline]
pub fn is_manually_drop(self) -> bool {
Expand Down
48 changes: 26 additions & 22 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -542,14 +542,12 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
debug!("univariant offset: {:?} field: {:#?}", offset, field);
offsets[i as usize] = offset;

if !repr.hide_niche() {
if let Some(mut niche) = field.largest_niche {
let available = niche.available(dl);
if available > largest_niche_available {
largest_niche_available = available;
niche.offset += offset;
largest_niche = Some(niche);
}
if let Some(mut niche) = field.largest_niche {
let available = niche.available(dl);
if available > largest_niche_available {
largest_niche_available = available;
niche.offset += offset;
largest_niche = Some(niche);
}
}

Expand Down Expand Up @@ -1104,23 +1102,29 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
assert!(valid_range.end >= end);
valid_range.end = end;
}

// Update `largest_niche` if we have introduced a larger niche.
let niche = if def.repr().hide_niche() {
None
if def.is_unsafe_cell() {
match scalar {
Scalar::Initialized { value, valid_range } => {
*valid_range = WrappingRange::full(value.size(dl))
}
// Already doesn't have any niches
Scalar::Union { .. } => {}
}
st.largest_niche = None;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the original code got misplaced into the #[rustc_layout_scalar_valid_range] handling, instead of being separate.
I think it should be moved out and made to exhaustively handle all Abis.

More specifically, Abi::{Scalar,ScalarPair,Vector}) all have Scalars in them - and even if ScalarPair appears to be handled, note that you're only operating its first Scalar.

So when testing, make sure to also check UnsafeCell around pairs with niches in both sides of the pair and custom #[repr(simd)] structs with niche'd element types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

custom #[repr(simd)] structs with niche'd element types.

I tried, we have checks preventing such datastructures from existing at all. So this can't happen.

Copy link
Member

Choose a reason for hiding this comment

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

The checks are not perfect: https://godbolt.org/z/W8o3Gxhdo.

} else {
Niche::from_scalar(dl, Size::ZERO, *scalar)
};
if let Some(niche) = niche {
match st.largest_niche {
Some(largest_niche) => {
// Replace the existing niche even if they're equal,
// because this one is at a lower offset.
if largest_niche.available(dl) <= niche.available(dl) {
st.largest_niche = Some(niche);
// Update `largest_niche` if we have introduced a larger niche.
let niche = Niche::from_scalar(dl, Size::ZERO, *scalar);
if let Some(niche) = niche {
match st.largest_niche {
Some(largest_niche) => {
// Replace the existing niche even if they're equal,
// because this one is at a lower offset.
if largest_niche.available(dl) <= niche.available(dl) {
st.largest_niche = Some(niche);
}
}
None => st.largest_niche = Some(niche),
}
None => st.largest_niche = Some(niche),
}
}
}
Expand Down
10 changes: 1 addition & 9 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1719,11 +1719,9 @@ bitflags! {
const IS_TRANSPARENT = 1 << 2;
// Internal only for now. If true, don't reorder fields.
const IS_LINEAR = 1 << 3;
// If true, don't expose any niche to type's context.
const HIDE_NICHE = 1 << 4;
// If true, the type's layout can be randomized using
// the seed stored in `ReprOptions.layout_seed`
const RANDOMIZE_LAYOUT = 1 << 5;
const RANDOMIZE_LAYOUT = 1 << 4;
// Any of these flags being set prevent field reordering optimisation.
const IS_UNOPTIMISABLE = ReprFlags::IS_C.bits
| ReprFlags::IS_SIMD.bits
Expand Down Expand Up @@ -1780,7 +1778,6 @@ impl ReprOptions {
ReprFlags::empty()
}
attr::ReprTransparent => ReprFlags::IS_TRANSPARENT,
attr::ReprNoNiche => ReprFlags::HIDE_NICHE,
attr::ReprSimd => ReprFlags::IS_SIMD,
attr::ReprInt(i) => {
size = Some(i);
Expand Down Expand Up @@ -1833,11 +1830,6 @@ impl ReprOptions {
self.flags.contains(ReprFlags::IS_LINEAR)
}

#[inline]
pub fn hide_niche(&self) -> bool {
self.flags.contains(ReprFlags::HIDE_NICHE)
}

/// Returns the discriminant type, given these `repr` options.
/// This must only be called on enums!
pub fn discr_type(&self) -> attr::IntType {
Expand Down
21 changes: 2 additions & 19 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1808,21 +1808,6 @@ impl CheckAttrVisitor<'_> {
_ => ("a", "struct, enum, or union"),
}
}
sym::no_niche => {
if !self.tcx.features().enabled(sym::no_niche) {
feature_err(
&self.tcx.sess.parse_sess,
sym::no_niche,
hint.span(),
"the attribute `repr(no_niche)` is currently unstable",
)
.emit();
}
match target {
Target::Struct | Target::Enum => continue,
_ => ("a", "struct or enum"),
}
}
sym::i8
| sym::u8
| sym::i16
Expand Down Expand Up @@ -1870,10 +1855,8 @@ impl CheckAttrVisitor<'_> {
// This is not ideal, but tracking precisely which ones are at fault is a huge hassle.
let hint_spans = hints.iter().map(|hint| hint.span());

// Error on repr(transparent, <anything else apart from no_niche>).
let non_no_niche = |hint: &&NestedMetaItem| hint.name_or_empty() != sym::no_niche;
let non_no_niche_count = hints.iter().filter(non_no_niche).count();
if is_transparent && non_no_niche_count > 1 {
// Error on repr(transparent, <anything else>).
if is_transparent && hints.len() > 1 {
let hint_spans: Vec<_> = hint_spans.clone().collect();
struct_span_err!(
self.tcx.sess,
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -979,7 +979,6 @@ symbols! {
no_link,
no_main,
no_mangle,
no_niche,
no_sanitize,
no_stack_check,
no_start,
Expand Down Expand Up @@ -1152,7 +1151,6 @@ symbols! {
repr128,
repr_align,
repr_align_enum,
repr_no_niche,
repr_packed,
repr_simd,
repr_transparent,
Expand Down
1 change: 0 additions & 1 deletion library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1856,7 +1856,6 @@ impl<T: ?Sized + fmt::Display> fmt::Display for RefMut<'_, T> {
#[lang = "unsafe_cell"]
#[stable(feature = "rust1", since = "1.0.0")]
#[repr(transparent)]
#[repr(no_niche)] // rust-lang/rust#68303.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
pub struct UnsafeCell<T: ?Sized> {
value: T,
}
Expand Down
1 change: 0 additions & 1 deletion library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@
#![feature(never_type)]
#![feature(no_core)]
#![feature(no_coverage)] // rust-lang/rust#84605
#![feature(no_niche)] // rust-lang/rust#68303
#![feature(platform_intrinsics)]
#![feature(prelude_import)]
#![feature(repr_simd)]
Expand Down
5 changes: 1 addition & 4 deletions src/test/ui/layout/unsafe-cell-hides-niche.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

// run-pass

#![feature(no_niche)]

use std::cell::UnsafeCell;
use std::mem::size_of;
use std::num::NonZeroU32 as N32;
Expand All @@ -16,8 +14,7 @@ struct Wrapper<T>(T);
#[repr(transparent)]
struct Transparent<T>(T);

#[repr(no_niche)]
struct NoNiche<T>(T);
struct NoNiche<T>(UnsafeCell<T>);

fn main() {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(size_of::<Option<Wrapper<u32>>>(), 8);
Expand Down
4 changes: 1 addition & 3 deletions src/test/ui/lint/clashing-extern-fn.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// check-pass
// aux-build:external_extern_fn.rs
#![crate_type = "lib"]
#![feature(no_niche)]
#![warn(clashing_extern_declarations)]

mod redeclared_different_signature {
Expand Down Expand Up @@ -400,9 +399,8 @@ mod hidden_niche {
#[repr(transparent)]
struct Transparent { x: NonZeroUsize }

#[repr(no_niche)]
#[repr(transparent)]
struct TransparentNoNiche { y: NonZeroUsize }
struct TransparentNoNiche { y: UnsafeCell<NonZeroUsize> }

extern "C" {
fn hidden_niche_transparent() -> Option<Transparent>;
Expand Down
Loading