Skip to content

Commit

Permalink
Auto merge of #103917 - oli-obk:layout_math, r=RalfJung,lcnr
Browse files Browse the repository at this point in the history
Various cleanups around scalar layout restrictions

Pulled out of #103724
  • Loading branch information
bors committed Nov 27, 2022
2 parents 5ac7e08 + 208bb93 commit df04d28
Show file tree
Hide file tree
Showing 16 changed files with 261 additions and 129 deletions.
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)
&& !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 @@ -2413,8 +2413,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 @@ -2470,25 +2496,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 @@ -2501,63 +2556,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 @@ -2581,13 +2609,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 @@ -2605,10 +2634,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 @@ -2637,8 +2665,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 @@ -2664,10 +2691,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

0 comments on commit df04d28

Please sign in to comment.