Auto merge of #51361 - oli-obk:sanity_check_consts, r=nikomatsakis

Do a basic sanity check for all constant values

## Motivation and high level overview

There has been some back and forth in this PR between @RalfJung and me in here about the motivation for this change and the stance it takes on unsafe coding guidelines.

The initial implementation ran its checks on every value read (so `*x`, `y = x`, ...). In unsafe code that isn't reasonable, because we might be invalidating invariants for a short time in order to build up a proper value.

The current implementation is a lint that runs its checks statics and constants. There is no need to check array lengths and enum variants, because it's a hard error to end up with anything but a number, and that one even has to have the required bits to be defined.

## What checks are done?

* Some type related checks
    * `char` needs to be a correct unicode character as defined by `char::from_u32`
    * A reference to a ZST must have the correct alignment (and be nonzero)
    * A reference to anything is dereferenced and its value is checked
* Layout checks use the information from `ty::Layout` to check
    * all fields of structs
    * all elements of arrays
    * enum discriminants
    * the fields of an enum variant (the variant is decided by the discriminant)
    * whether any union field succeeds in being checked (if none match the memory pattern, the check fails)
    * if the value is in the range described by the layout (e.g. for `NonZero*` types)

Changing the layout of a type will thus automatically cause the checks to check for the new layout.

fixes #51330
fixes #51471

cc @RalfJung

r? @eddyb

src/Cargo.lock

@@ -355,7 +355,7 @@ dependencies = [
  "cargo_metadata 0.5.8 (registry+https://github.com/rust-lang/crates.io-index)",
  "clippy-mini-macro-test 0.2.0",
  "clippy_lints 0.0.212",
- "compiletest_rs 0.3.11 (registry+https://github.com/rust-lang/crates.io-index)",
+ "compiletest_rs 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)",
  "derive-new 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "lazy_static 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "num-traits 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -461,7 +461,7 @@ dependencies = [
 
 [[package]]
 name = "compiletest_rs"
-version = "0.3.11"
+version = "0.3.13"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "diff 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -1319,7 +1319,7 @@ dependencies = [
  "byteorder 1.2.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "cargo_metadata 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "colored 1.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
- "compiletest_rs 0.3.11 (registry+https://github.com/rust-lang/crates.io-index)",
+ "compiletest_rs 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)",
  "env_logger 0.5.10 (registry+https://github.com/rust-lang/crates.io-index)",
  "lazy_static 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "log 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -3118,7 +3118,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 "checksum colored 1.6.0 (registry+https://github.com/rust-lang/crates.io-index)" = "b0aa3473e85a3161b59845d6096b289bb577874cafeaf75ea1b1beaa6572c7fc"
 "checksum commoncrypto 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "d056a8586ba25a1e4d61cb090900e495952c7886786fc55f909ab2f819b69007"
 "checksum commoncrypto-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "1fed34f46747aa73dfaa578069fd8279d2818ade2b55f38f22a9401c7f4083e2"
-"checksum compiletest_rs 0.3.11 (registry+https://github.com/rust-lang/crates.io-index)" = "04cea0fe8b8aaca8143af607ad69076866c9f08b83c4b7faca0e993e5486831b"
+"checksum compiletest_rs 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)" = "d3064bc712922596dd5ab449fca9261d411893356581fe5297b96aa8f53bb1b8"
 "checksum core-foundation 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "286e0b41c3a20da26536c6000a280585d519fd07b3956b43aed8a79e9edce980"
 "checksum core-foundation 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)" = "cc3532ec724375c7cb7ff0a097b714fde180bb1f6ed2ab27cfcd99ffca873cd2"
 "checksum core-foundation-sys 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "716c271e8613ace48344f723b60b900a93150271e5be206212d052bbc0883efa"

src/librustc/lib.rs

@@ -73,6 +73,7 @@
 #![feature(in_band_lifetimes)]
 #![feature(macro_at_most_once_rep)]
 #![feature(inclusive_range_methods)]
+#![feature(crate_in_paths)]
 
 #![recursion_limit="512"]
 

src/librustc/mir/interpret/error.rs

@@ -47,7 +47,7 @@ impl<'a, 'gcx, 'tcx> ConstEvalErr<'tcx> {
         tcx: TyCtxtAt<'a, 'gcx, 'tcx>,
         message: &str
     ) {
-        let err = self.struct_generic(tcx, message, None);
+        let err = self.struct_error(tcx, message);
         if let Some(mut err) = err {
             err.emit();
         }

src/librustc/ty/layout.rs

@@ -1599,7 +1599,7 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
             // Potentially-fat pointers.
             ty::TyRef(_, pointee, _) |
             ty::TyRawPtr(ty::TypeAndMut { ty: pointee, .. }) => {
-                assert!(i < 2);
+                assert!(i < this.fields.count());
 
                 // Reuse the fat *T type as its own thin pointer data field.
                 // This provides information about e.g. DST struct pointees
@@ -1621,10 +1621,25 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
                 match tcx.struct_tail(pointee).sty {
                     ty::TySlice(_) |
                     ty::TyStr => tcx.types.usize,
-                    ty::TyDynamic(..) => {
-                        // FIXME(eddyb) use an usize/fn() array with
-                        // the correct number of vtables slots.
-                        tcx.mk_imm_ref(tcx.types.re_static, tcx.mk_nil())
+                    ty::TyDynamic(data, _) => {
+                        let trait_def_id = data.principal().unwrap().def_id();
+                        let num_fns: u64 = crate::traits::supertrait_def_ids(tcx, trait_def_id)
+                            .map(|trait_def_id| {
+                                tcx.associated_items(trait_def_id)
+                                    .filter(|item| item.kind == ty::AssociatedKind::Method)
+                                    .count() as u64
+                            })
+                            .sum();
+                        tcx.mk_imm_ref(
+                            tcx.types.re_static,
+                            tcx.mk_array(tcx.types.usize, 3 + num_fns),
+                        )
+                        /* FIXME use actual fn pointers
+                        tcx.mk_tup(&[
+                            tcx.mk_array(tcx.types.usize, 3),
+                            tcx.mk_array(Option<fn()>),
+                        ])
+                        */
                     }
                     _ => bug!("TyLayout::field_type({:?}): not applicable", this)
                 }

src/librustc_lint/builtin.rs

@@ -40,6 +40,7 @@ use lint::{LateContext, LintContext, LintArray};
 use lint::{LintPass, LateLintPass, EarlyLintPass, EarlyContext};
 
 use std::collections::HashSet;
+use rustc::util::nodemap::FxHashSet;
 
 use syntax::tokenstream::{TokenTree, TokenStream};
 use syntax::ast;
@@ -1576,20 +1577,77 @@ impl LintPass for UnusedBrokenConst {
     }
 }
 
+fn validate_const<'a, 'tcx>(
+    tcx: ty::TyCtxt<'a, 'tcx, 'tcx>,
+    constant: &ty::Const<'tcx>,
+    param_env: ty::ParamEnv<'tcx>,
+    gid: ::rustc::mir::interpret::GlobalId<'tcx>,
+    what: &str,
+) {
+    let mut ecx = ::rustc_mir::interpret::mk_eval_cx(tcx, gid.instance, param_env).unwrap();
+    let result = (|| {
+        let val = ecx.const_to_value(constant.val)?;
+        use rustc_target::abi::LayoutOf;
+        let layout = ecx.layout_of(constant.ty)?;
+        let place = ecx.allocate_place_for_value(val, layout, None)?;
+        let ptr = place.to_ptr()?;
+        let mut todo = vec![(ptr, layout.ty, String::new())];
+        let mut seen = FxHashSet();
+        seen.insert((ptr, layout.ty));
+        while let Some((ptr, ty, path)) = todo.pop() {
+            let layout = ecx.layout_of(ty)?;
+            ecx.validate_ptr_target(
+                ptr,
+                layout.align,
+                layout,
+                path,
+                &mut seen,
+                &mut todo,
+            )?;
+        }
+        Ok(())
+    })();
+    if let Err(err) = result {
+        let (trace, span) = ecx.generate_stacktrace(None);
+        let err = ::rustc::mir::interpret::ConstEvalErr {
+            error: err,
+            stacktrace: trace,
+            span,
+        };
+        let err = err.struct_error(
+            tcx.at(span),
+            &format!("this {} likely exhibits undefined behavior", what),
+        );
+        if let Some(mut err) = err {
+            err.note("The rules on what exactly is undefined behavior aren't clear, \
+                so this check might be overzealous. Please open an issue on the rust compiler \
+                repository if you believe it should not be considered undefined behavior",
+            );
+            err.emit();
+        }
+    }
+}
+
 fn check_const(cx: &LateContext, body_id: hir::BodyId, what: &str) {
     let def_id = cx.tcx.hir.body_owner_def_id(body_id);
     let param_env = cx.tcx.param_env(def_id);
     let cid = ::rustc::mir::interpret::GlobalId {
         instance: ty::Instance::mono(cx.tcx, def_id),
         promoted: None
     };
-    if let Err(err) = cx.tcx.const_eval(param_env.and(cid)) {
-        let span = cx.tcx.def_span(def_id);
-        err.report_as_lint(
-            cx.tcx.at(span),
-            &format!("this {} cannot be used", what),
-            cx.current_lint_root(),
-        );
+    match cx.tcx.const_eval(param_env.and(cid)) {
+        Ok(val) => validate_const(cx.tcx, val, param_env, cid, what),
+        Err(err) => {
+            // errors for statics are already reported directly in the query
+            if cx.tcx.is_static(def_id).is_none() {
+                let span = cx.tcx.def_span(def_id);
+                err.report_as_lint(
+                    cx.tcx.at(span),
+                    &format!("this {} cannot be used", what),
+                    cx.current_lint_root(),
+                );
+            }
+        },
     }
 }
 
@@ -1610,6 +1668,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedBrokenConst {
             hir::ItemKind::Const(_, body_id) => {
                 check_const(cx, body_id, "constant");
             },
+            hir::ItemKind::Static(_, _, body_id) => {
+                check_const(cx, body_id, "static");
+            },
             hir::ItemKind::Ty(ref ty, _) => hir::intravisit::walk_ty(
                 &mut UnusedBrokenConstVisitor(cx),
                 ty

src/librustc_mir/interpret/const_eval.rs

@@ -5,7 +5,7 @@ use rustc::hir;
 use rustc::mir::interpret::{ConstEvalErr};
 use rustc::mir;
 use rustc::ty::{self, TyCtxt, Ty, Instance};
-use rustc::ty::layout::{self, LayoutOf, Primitive};
+use rustc::ty::layout::{self, LayoutOf, Primitive, TyLayout};
 use rustc::ty::subst::Subst;
 
 use syntax::ast::Mutability;
@@ -16,7 +16,7 @@ use rustc::mir::interpret::{
     EvalResult, EvalError, EvalErrorKind, GlobalId,
     Value, Scalar, AllocId, Allocation, ConstValue,
 };
-use super::{Place, EvalContext, StackPopCleanup, ValTy, PlaceExtra, Memory, MemoryKind};
+use super::{Place, EvalContext, StackPopCleanup, ValTy, Memory, MemoryKind};
 
 pub fn mk_borrowck_eval_cx<'a, 'mir, 'tcx>(
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
@@ -63,7 +63,7 @@ pub fn eval_promoted<'a, 'mir, 'tcx>(
     cid: GlobalId<'tcx>,
     mir: &'mir mir::Mir<'tcx>,
     param_env: ty::ParamEnv<'tcx>,
-) -> EvalResult<'tcx, (Value, Scalar, Ty<'tcx>)> {
+) -> EvalResult<'tcx, (Value, Scalar, TyLayout<'tcx>)> {
     ecx.with_fresh_body(|ecx| {
         eval_body_using_ecx(ecx, cid, Some(mir), param_env)
     })
@@ -121,7 +121,7 @@ fn eval_body_and_ecx<'a, 'mir, 'tcx>(
     cid: GlobalId<'tcx>,
     mir: Option<&'mir mir::Mir<'tcx>>,
     param_env: ty::ParamEnv<'tcx>,
-) -> (EvalResult<'tcx, (Value, Scalar, Ty<'tcx>)>, EvalContext<'a, 'mir, 'tcx, CompileTimeEvaluator>) {
+) -> (EvalResult<'tcx, (Value, Scalar, TyLayout<'tcx>)>, EvalContext<'a, 'mir, 'tcx, CompileTimeEvaluator>) {
     debug!("eval_body_and_ecx: {:?}, {:?}", cid, param_env);
     // we start out with the best span we have
     // and try improving it down the road when more information is available
@@ -137,7 +137,7 @@ fn eval_body_using_ecx<'a, 'mir, 'tcx>(
     cid: GlobalId<'tcx>,
     mir: Option<&'mir mir::Mir<'tcx>>,
     param_env: ty::ParamEnv<'tcx>,
-) -> EvalResult<'tcx, (Value, Scalar, Ty<'tcx>)> {
+) -> EvalResult<'tcx, (Value, Scalar, TyLayout<'tcx>)> {
     debug!("eval_body: {:?}, {:?}", cid, param_env);
     let tcx = ecx.tcx.tcx;
     let mut mir = match mir {
@@ -182,7 +182,7 @@ fn eval_body_using_ecx<'a, 'mir, 'tcx>(
         // point at the allocation
         _ => Value::ByRef(ptr, layout.align),
     };
-    Ok((value, ptr, layout.ty))
+    Ok((value, ptr, layout))
 }
 
 #[derive(Debug, Clone, Eq, PartialEq, Hash)]
@@ -434,19 +434,7 @@ pub fn const_val_field<'a, 'tcx>(
         let ty = value.ty;
         let value = ecx.const_to_value(value.val)?;
         let layout = ecx.layout_of(ty)?;
-        let (ptr, align) = match value {
-            Value::ByRef(ptr, align) => (ptr, align),
-            Value::ScalarPair(..) | Value::Scalar(_) => {
-                let ptr = ecx.alloc_ptr(ty)?.into();
-                ecx.write_value_to_ptr(value, ptr, layout.align, ty)?;
-                (ptr, layout.align)
-            },
-        };
-        let place = Place::Ptr {
-            ptr,
-            align,
-            extra: variant.map_or(PlaceExtra::None, PlaceExtra::DowncastVariant),
-        };
+        let place = ecx.allocate_place_for_value(value, layout, variant)?;
         let (place, layout) = ecx.place_field(place, field, layout)?;
         let (ptr, align) = place.to_ptr_align();
         let mut new_value = Value::ByRef(ptr, align);
@@ -484,17 +472,17 @@ pub fn const_variant_index<'a, 'tcx>(
     trace!("const_variant_index: {:?}, {:?}", instance, val);
     let mut ecx = mk_eval_cx(tcx, instance, param_env).unwrap();
     let value = ecx.const_to_value(val.val)?;
+    let layout = ecx.layout_of(val.ty)?;
     let (ptr, align) = match value {
         Value::ScalarPair(..) | Value::Scalar(_) => {
-            let layout = ecx.layout_of(val.ty)?;
             let ptr = ecx.memory.allocate(layout.size, layout.align, MemoryKind::Stack)?.into();
             ecx.write_value_to_ptr(value, ptr, layout.align, val.ty)?;
             (ptr, layout.align)
         },
         Value::ByRef(ptr, align) => (ptr, align),
     };
     let place = Place::from_scalar_ptr(ptr, align);
-    ecx.read_discriminant_as_variant_index(place, val.ty)
+    ecx.read_discriminant_as_variant_index(place, layout)
 }
 
 pub fn const_value_to_allocation_provider<'a, 'tcx>(
@@ -560,11 +548,11 @@ pub fn const_eval_provider<'a, 'tcx>(
     };
 
     let (res, ecx) = eval_body_and_ecx(tcx, cid, None, key.param_env);
-    res.and_then(|(mut val, _, miri_ty)| {
+    res.and_then(|(mut val, _, layout)| {
         if tcx.is_static(def_id).is_none() && cid.promoted.is_none() {
-            val = ecx.try_read_by_ref(val, miri_ty)?;
+            val = ecx.try_read_by_ref(val, layout.ty)?;
         }
-        Ok(value_to_const_value(&ecx, val, miri_ty))
+        Ok(value_to_const_value(&ecx, val, layout.ty))
     }).map_err(|err| {
         let (trace, span) = ecx.generate_stacktrace(None);
         let err = ConstEvalErr {