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

Various cleanups around scalar layout restrictions #103917

Merged
merged 17 commits into from
Nov 27, 2022
Merged
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
24 changes: 11 additions & 13 deletions compiler/rustc_abi/src/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,28 +382,26 @@ pub trait LayoutCalculator {
let (start, end) = scalar_valid_range;
match st.abi {
Abi::Scalar(ref mut scalar) | Abi::ScalarPair(ref mut scalar, _) => {
// the asserts ensure that we are not using the
// `#[rustc_layout_scalar_valid_range(n)]`
// attribute to widen the range of anything as that would probably
// result in UB somewhere
// FIXME(eddyb) the asserts are probably not needed,
// as larger validity ranges would result in missed
// Enlarging validity ranges would result in missed
// optimizations, *not* wrongly assuming the inner
// value is valid. e.g. unions enlarge validity ranges,
// value is valid. e.g. unions already enlarge validity ranges,
// because the values may be uninitialized.
//
// Because of that we only check that the start and end
// of the range is representable with this scalar type.

let max_value = scalar.size(dl).unsigned_int_max();
if let Bound::Included(start) = start {
// FIXME(eddyb) this might be incorrect - it doesn't
// account for wrap-around (end < start) ranges.
let valid_range = scalar.valid_range_mut();
assert!(valid_range.start <= start);
valid_range.start = start;
assert!(start <= max_value, "{start} > {max_value}");
scalar.valid_range_mut().start = start;
}
if let Bound::Included(end) = end {
// FIXME(eddyb) this might be incorrect - it doesn't
// account for wrap-around (end < start) ranges.
let valid_range = scalar.valid_range_mut();
assert!(valid_range.end >= end);
valid_range.end = end;
assert!(end <= max_value, "{end} > {max_value}");
scalar.valid_range_mut().end = end;
}

// Update `largest_niche` if we have introduced a larger niche.
Expand Down
16 changes: 4 additions & 12 deletions compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,18 +785,10 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
}
}
Abi::ScalarPair(a_layout, b_layout) => {
// There is no `rustc_layout_scalar_valid_range_start` for pairs, so
// we would validate these things as we descend into the fields,
// but that can miss bugs in layout computation. Layout computation
// is subtle due to enums having ScalarPair layout, where one field
// is the discriminant.
if cfg!(debug_assertions)
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
&& !a_layout.is_uninit_valid()
&& !b_layout.is_uninit_valid()
{
// We can only proceed if *both* scalars need to be initialized.
// FIXME: find a way to also check ScalarPair when one side can be uninit but
// the other must be init.
// We can only proceed if *both* scalars need to be initialized.
// FIXME: find a way to also check ScalarPair when one side can be uninit but
// the other must be init.
if !a_layout.is_uninit_valid() && !b_layout.is_uninit_valid() {
let (a, b) =
self.read_immediate(op, "initiailized scalar value")?.to_scalar_pair();
self.visit_scalar(a, a_layout)?;
Expand Down
11 changes: 6 additions & 5 deletions compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ struct TypeChecker<'a, 'tcx> {
}

impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
#[track_caller]
fn fail(&self, location: Location, msg: impl AsRef<str>) {
let span = self.body.source_info(location).span;
// We use `delay_span_bug` as we might see broken MIR when other errors have already
Expand Down Expand Up @@ -226,12 +227,12 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
let check_equal = |this: &Self, location, f_ty| {
if !this.mir_assign_valid_types(ty, f_ty) {
this.fail(
location,
format!(
"Field projection `{:?}.{:?}` specified type `{:?}`, but actual type is `{:?}`",
parent, f, ty, f_ty
location,
format!(
"Field projection `{:?}.{:?}` specified type `{:?}`, but actual type is `{:?}`",
parent, f, ty, f_ty
)
)
)
}
};

Expand Down
160 changes: 97 additions & 63 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ use rustc_span::edition::Edition;
use rustc_span::source_map::Spanned;
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::{BytePos, InnerSpan, Span};
use rustc_target::abi::VariantIdx;
use rustc_target::abi::{Abi, VariantIdx};
use rustc_trait_selection::traits::{self, misc::can_type_implement_copy};

use crate::nonstandard_style::{method_context, MethodLateContext};
Expand Down Expand Up @@ -2408,8 +2408,34 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
}

/// Information about why a type cannot be initialized this way.
/// Contains an error message and optionally a span to point at.
type InitError = (String, Option<Span>);
struct InitError {
message: String,
/// Spans from struct fields and similar that can be obtained from just the type.
span: Option<Span>,
/// Used to report a trace through adts.
nested: Option<Box<InitError>>,
}
impl InitError {
fn spanned(self, span: Span) -> InitError {
Self { span: Some(span), ..self }
}

fn nested(self, nested: impl Into<Option<InitError>>) -> InitError {
assert!(self.nested.is_none());
Self { nested: nested.into().map(Box::new), ..self }
}
}

impl<'a> From<&'a str> for InitError {
fn from(s: &'a str) -> Self {
s.to_owned().into()
}
}
impl From<String> for InitError {
fn from(message: String) -> Self {
Self { message, span: None, nested: None }
}
}

/// Test if this constant is all-0.
fn is_zero(expr: &hir::Expr<'_>) -> bool {
Expand Down Expand Up @@ -2465,25 +2491,54 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {

fn variant_find_init_error<'tcx>(
cx: &LateContext<'tcx>,
ty: Ty<'tcx>,
variant: &VariantDef,
substs: ty::SubstsRef<'tcx>,
descr: &str,
init: InitKind,
) -> Option<InitError> {
variant.fields.iter().find_map(|field| {
ty_find_init_error(cx, field.ty(cx.tcx, substs), init).map(|(mut msg, span)| {
if span.is_none() {
// Point to this field, should be helpful for figuring
// out where the source of the error is.
let span = cx.tcx.def_span(field.did);
write!(&mut msg, " (in this {descr})").unwrap();
(msg, Some(span))
let mut field_err = variant.fields.iter().find_map(|field| {
ty_find_init_error(cx, field.ty(cx.tcx, substs), init).map(|mut err| {
if !field.did.is_local() {
err
} else if err.span.is_none() {
err.span = Some(cx.tcx.def_span(field.did));
write!(&mut err.message, " (in this {descr})").unwrap();
err
} else {
// Just forward.
(msg, span)
InitError::from(format!("in this {descr}"))
.spanned(cx.tcx.def_span(field.did))
.nested(err)
}
})
})
});

// Check if this ADT has a constrained layout (like `NonNull` and friends).
if let Ok(layout) = cx.tcx.layout_of(cx.param_env.and(ty)) {
if let Abi::Scalar(scalar) | Abi::ScalarPair(scalar, _) = &layout.abi {
let range = scalar.valid_range(cx);
let msg = if !range.contains(0) {
"must be non-null"
} else if init == InitKind::Uninit && !scalar.is_always_valid(cx) {
// Prefer reporting on the fields over the entire struct for uninit,
// as the information bubbles out and it may be unclear why the type can't
// be null from just its outside signature.

"must be initialized inside its custom valid range"
} else {
return field_err;
};
if let Some(field_err) = &mut field_err {
// Most of the time, if the field error is the same as the struct error,
// the struct error only happens because of the field error.
if field_err.message.contains(msg) {
field_err.message = format!("because {}", field_err.message);
}
}
return Some(InitError::from(format!("`{ty}` {msg}")).nested(field_err));
}
}
field_err
}

/// Return `Some` only if we are sure this type does *not*
Expand All @@ -2496,63 +2551,36 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
use rustc_type_ir::sty::TyKind::*;
match ty.kind() {
// Primitive types that don't like 0 as a value.
Ref(..) => Some(("references must be non-null".to_string(), None)),
Adt(..) if ty.is_box() => Some(("`Box` must be non-null".to_string(), None)),
FnPtr(..) => Some(("function pointers must be non-null".to_string(), None)),
Never => Some(("the `!` type has no valid value".to_string(), None)),
Ref(..) => Some("references must be non-null".into()),
Adt(..) if ty.is_box() => Some("`Box` must be non-null".into()),
FnPtr(..) => Some("function pointers must be non-null".into()),
Never => Some("the `!` type has no valid value".into()),
RawPtr(tm) if matches!(tm.ty.kind(), Dynamic(..)) =>
// raw ptr to dyn Trait
{
Some(("the vtable of a wide raw pointer must be non-null".to_string(), None))
Some("the vtable of a wide raw pointer must be non-null".into())
}
// Primitive types with other constraints.
Bool if init == InitKind::Uninit => {
Some(("booleans must be either `true` or `false`".to_string(), None))
Some("booleans must be either `true` or `false`".into())
}
Char if init == InitKind::Uninit => {
Some(("characters must be a valid Unicode codepoint".to_string(), None))
Some("characters must be a valid Unicode codepoint".into())
}
Int(_) | Uint(_) if init == InitKind::Uninit => {
Some(("integers must not be uninitialized".to_string(), None))
}
Float(_) if init == InitKind::Uninit => {
Some(("floats must not be uninitialized".to_string(), None))
Some("integers must be initialized".into())
}
Float(_) if init == InitKind::Uninit => Some("floats must be initialized".into()),
RawPtr(_) if init == InitKind::Uninit => {
Some(("raw pointers must not be uninitialized".to_string(), None))
Some("raw pointers must be initialized".into())
}
// Recurse and checks for some compound types. (but not unions)
Adt(adt_def, substs) if !adt_def.is_union() => {
// First check if this ADT has a layout attribute (like `NonNull` and friends).
use std::ops::Bound;
match cx.tcx.layout_scalar_valid_range(adt_def.did()) {
// We exploit here that `layout_scalar_valid_range` will never
// return `Bound::Excluded`. (And we have tests checking that we
// handle the attribute correctly.)
// We don't add a span since users cannot declare such types anyway.
(Bound::Included(lo), Bound::Included(hi)) if 0 < lo && lo < hi => {
return Some((format!("`{}` must be non-null", ty), None));
}
(Bound::Included(lo), Bound::Unbounded) if 0 < lo => {
return Some((format!("`{}` must be non-null", ty), None));
}
(Bound::Included(_), _) | (_, Bound::Included(_))
if init == InitKind::Uninit =>
{
return Some((
format!(
"`{}` must be initialized inside its custom valid range",
ty,
),
None,
));
}
_ => {}
}
// Handle structs.
if adt_def.is_struct() {
return variant_find_init_error(
cx,
ty,
adt_def.non_enum_variant(),
substs,
"struct field",
Expand All @@ -2576,13 +2604,14 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
Some((variant, definitely_inhabited))
});
let Some(first_variant) = potential_variants.next() else {
return Some(("enums with no inhabited variants have no valid value".to_string(), Some(span)));
return Some(InitError::from("enums with no inhabited variants have no valid value").spanned(span));
};
// So we have at least one potentially inhabited variant. Might we have two?
let Some(second_variant) = potential_variants.next() else {
// There is only one potentially inhabited variant. So we can recursively check that variant!
return variant_find_init_error(
cx,
ty,
&first_variant.0,
substs,
"field of the only potentially inhabited enum variant",
Expand All @@ -2600,10 +2629,9 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
.filter(|(_variant, definitely_inhabited)| *definitely_inhabited)
.count();
if definitely_inhabited > 1 {
return Some((
"enums with multiple inhabited variants have to be initialized to a variant".to_string(),
Some(span),
));
return Some(InitError::from(
"enums with multiple inhabited variants have to be initialized to a variant",
).spanned(span));
}
}
// We couldn't find anything wrong here.
Expand Down Expand Up @@ -2632,8 +2660,7 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
// using zeroed or uninitialized memory.
// We are extremely conservative with what we warn about.
let conjured_ty = cx.typeck_results().expr_ty(expr);
if let Some((msg, span)) =
with_no_trimmed_paths!(ty_find_init_error(cx, conjured_ty, init))
if let Some(mut err) = with_no_trimmed_paths!(ty_find_init_error(cx, conjured_ty, init))
{
// FIXME(davidtwco): make translatable
cx.struct_span_lint(
Expand All @@ -2659,10 +2686,17 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
"help: use `MaybeUninit<T>` instead, \
and only call `assume_init` after initialization is done",
);
if let Some(span) = span {
lint.span_note(span, &msg);
} else {
lint.note(&msg);
loop {
if let Some(span) = err.span {
lint.span_note(span, &err.message);
} else {
lint.note(&err.message);
}
if let Some(e) = err.nested {
err = *e;
} else {
break;
}
}
lint
},
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_middle/src/ty/consts/int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,18 @@ impl ScalarInt {
self.to_bits(size)
}

// Tries to convert the `ScalarInt` to `bool`. Fails if the `size` of the `ScalarInt`
// in not equal to `Size { raw: 1 }` or if the value is not 0 or 1 and returns the `size`
// value of the `ScalarInt` in that case.
#[inline]
pub fn try_to_bool(self) -> Result<bool, Size> {
match self.try_to_u8()? {
0 => Ok(false),
1 => Ok(true),
_ => Err(self.size()),
}
}

// Tries to convert the `ScalarInt` to `u8`. Fails if the `size` of the `ScalarInt`
// in not equal to `Size { raw: 1 }` and returns the `size` value of the `ScalarInt` in
// that case.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_span/src/def_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ impl CrateNum {

impl fmt::Display for CrateNum {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(&self.private, f)
fmt::Display::fmt(&self.as_u32(), f)
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_span/src/hygiene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub struct ExpnId {
impl fmt::Debug for ExpnId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// Generate crate_::{{expn_}}.
write!(f, "{:?}::{{{{expn{}}}}}", self.krate, self.local_id.private)
write!(f, "{:?}::{{{{expn{}}}}}", self.krate, self.local_id.as_u32())
}
}

Expand Down
Loading