From 0e674b3ec5e2c238e6756d512fa39efc1ceeb74a Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 29 Jun 2022 09:56:30 +0000 Subject: [PATCH 1/4] Some tracing cleanups --- compiler/rustc_codegen_llvm/src/builder.rs | 6 +++--- compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 2 +- compiler/rustc_codegen_ssa/src/mir/place.rs | 3 ++- compiler/rustc_codegen_ssa/src/mir/rvalue.rs | 3 +-- compiler/rustc_codegen_ssa/src/mir/statement.rs | 3 +-- compiler/rustc_mir_build/src/thir/cx/expr.rs | 5 +++-- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 8c1e865762ccd..70cdf90303825 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -28,7 +28,7 @@ use std::ffi::CStr; use std::iter; use std::ops::Deref; use std::ptr; -use tracing::debug; +use tracing::{debug, instrument}; // All Builders must have an llfn associated with them #[must_use] @@ -464,15 +464,15 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { } } + #[instrument(level = "debug", skip(self))] fn load_operand(&mut self, place: PlaceRef<'tcx, &'ll Value>) -> OperandRef<'tcx, &'ll Value> { - debug!("PlaceRef::load: {:?}", place); - assert_eq!(place.llextra.is_some(), place.layout.is_unsized()); if place.layout.is_zst() { return OperandRef::new_zst(self, place.layout); } + #[instrument(level = "trace", skip(bx))] fn scalar_load_metadata<'a, 'll, 'tcx>( bx: &mut Builder<'a, 'll, 'tcx>, load: &'ll Value, diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index b831423994f24..d8b09f8ec2e73 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1623,7 +1623,7 @@ extern "C" { B: &Builder<'a>, Val: &'a Value, DestTy: &'a Type, - IsSized: bool, + IsSigned: bool, ) -> &'a Value; // Comparisons diff --git a/compiler/rustc_codegen_ssa/src/mir/place.rs b/compiler/rustc_codegen_ssa/src/mir/place.rs index 5b88635982f54..95281b3d4c304 100644 --- a/compiler/rustc_codegen_ssa/src/mir/place.rs +++ b/compiler/rustc_codegen_ssa/src/mir/place.rs @@ -204,6 +204,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { } /// Obtain the actual discriminant of a value. + #[instrument(level = "trace", skip(bx))] pub fn codegen_get_discr>( self, bx: &mut Bx, @@ -420,12 +421,12 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { } impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { + #[instrument(level = "debug", skip(self, bx))] pub fn codegen_place( &mut self, bx: &mut Bx, place_ref: mir::PlaceRef<'tcx>, ) -> PlaceRef<'tcx, Bx::Value> { - debug!("codegen_place(place_ref={:?})", place_ref); let cx = self.cx; let tcx = self.cx.tcx(); diff --git a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs index 81c1897694ce6..6a87da3ba8910 100644 --- a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs +++ b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs @@ -15,14 +15,13 @@ use rustc_span::source_map::{Span, DUMMY_SP}; use rustc_target::abi::{Abi, Int, Variants}; impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { + #[instrument(level = "debug", skip(self, bx))] pub fn codegen_rvalue( &mut self, mut bx: Bx, dest: PlaceRef<'tcx, Bx::Value>, rvalue: &mir::Rvalue<'tcx>, ) -> Bx { - debug!("codegen_rvalue(dest.llval={:?}, rvalue={:?})", dest.llval, rvalue); - match *rvalue { mir::Rvalue::Use(ref operand) => { let cg_operand = self.codegen_operand(&mut bx, operand); diff --git a/compiler/rustc_codegen_ssa/src/mir/statement.rs b/compiler/rustc_codegen_ssa/src/mir/statement.rs index d9ebfc3e87143..f452f29883f93 100644 --- a/compiler/rustc_codegen_ssa/src/mir/statement.rs +++ b/compiler/rustc_codegen_ssa/src/mir/statement.rs @@ -6,9 +6,8 @@ use crate::traits::BuilderMethods; use crate::traits::*; impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { + #[instrument(level = "debug", skip(self, bx))] pub fn codegen_statement(&mut self, mut bx: Bx, statement: &mir::Statement<'tcx>) -> Bx { - debug!("codegen_statement(statement={:?})", statement); - self.set_debug_loc(&mut bx, statement.source_info); match statement.kind { mir::StatementKind::Assign(box (ref place, ref rvalue)) => { diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index 08b66d0abc702..e1ca813bda1c6 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -32,13 +32,14 @@ impl<'tcx> Cx<'tcx> { exprs.iter().map(|expr| self.mirror_expr_inner(expr)).collect() } + #[instrument(level = "debug", skip(self, hir_expr))] pub(super) fn mirror_expr_inner(&mut self, hir_expr: &'tcx hir::Expr<'tcx>) -> ExprId { let temp_lifetime = self.rvalue_scopes.temporary_scope(self.region_scope_tree, hir_expr.hir_id.local_id); let expr_scope = region::Scope { id: hir_expr.hir_id.local_id, data: region::ScopeData::Node }; - debug!("Expr::make_mirror(): id={}, span={:?}", hir_expr.hir_id, hir_expr.span); + debug!(?hir_expr.hir_id, ?hir_expr.span); let mut expr = self.make_mirror_unadjusted(hir_expr); @@ -49,7 +50,7 @@ impl<'tcx> Cx<'tcx> { // Now apply adjustments, if any. for adjustment in self.typeck_results.expr_adjustments(hir_expr) { - debug!("make_mirror: expr={:?} applying adjustment={:?}", expr, adjustment); + trace!(?expr, ?adjustment); let span = expr.span; expr = self.apply_adjustment(hir_expr, expr, adjustment, adjustment_span.unwrap_or(span)); From 7839cb963f747edff90b72efc6094e7184f6ef0e Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 29 Jun 2022 14:18:55 +0000 Subject: [PATCH 2/4] Change enum->int casts to not go through MIR casts. Instead we generate a discriminant rvalue and cast the result of that. --- compiler/rustc_codegen_cranelift/src/base.rs | 23 ------ compiler/rustc_codegen_ssa/src/mir/rvalue.rs | 78 ++----------------- .../rustc_const_eval/src/interpret/cast.rs | 25 +----- .../src/transform/validate.rs | 32 +++++++- .../src/build/expr/as_rvalue.rs | 29 ++++++- .../codegen/enum-bounds-check-derived-idx.rs | 6 +- .../codegen/enum-bounds-check-issue-13926.rs | 3 +- .../codegen/enum-bounds-check-issue-82871.rs | 9 ++- src/test/codegen/enum-bounds-check.rs | 3 +- src/test/mir-opt/enum_cast.bar.mir_map.0.mir | 13 ++++ src/test/mir-opt/enum_cast.boo.mir_map.0.mir | 13 ++++ .../mir-opt/enum_cast.droppy.mir_map.0.mir | 54 +++++++++++++ src/test/mir-opt/enum_cast.foo.mir_map.0.mir | 13 ++++ src/test/mir-opt/enum_cast.rs | 50 ++++++++++++ .../run-pass-valgrind/cast-enum-with-dtor.rs | 2 +- 15 files changed, 221 insertions(+), 132 deletions(-) create mode 100644 src/test/mir-opt/enum_cast.bar.mir_map.0.mir create mode 100644 src/test/mir-opt/enum_cast.boo.mir_map.0.mir create mode 100644 src/test/mir-opt/enum_cast.droppy.mir_map.0.mir create mode 100644 src/test/mir-opt/enum_cast.foo.mir_map.0.mir create mode 100644 src/test/mir-opt/enum_cast.rs diff --git a/compiler/rustc_codegen_cranelift/src/base.rs b/compiler/rustc_codegen_cranelift/src/base.rs index fbe830b2b1030..a07ed405aa4c6 100644 --- a/compiler/rustc_codegen_cranelift/src/base.rs +++ b/compiler/rustc_codegen_cranelift/src/base.rs @@ -635,29 +635,6 @@ fn codegen_stmt<'tcx>( let (ptr, _extra) = operand.load_scalar_pair(fx); lval.write_cvalue(fx, CValue::by_val(ptr, dest_layout)) } - } else if let ty::Adt(adt_def, _substs) = from_ty.kind() { - // enum -> discriminant value - assert!(adt_def.is_enum()); - match to_ty.kind() { - ty::Uint(_) | ty::Int(_) => {} - _ => unreachable!("cast adt {} -> {}", from_ty, to_ty), - } - let to_clif_ty = fx.clif_type(to_ty).unwrap(); - - let discriminant = crate::discriminant::codegen_get_discriminant( - fx, - operand, - fx.layout_of(operand.layout().ty.discriminant_ty(fx.tcx)), - ) - .load_scalar(fx); - - let res = crate::cast::clif_intcast( - fx, - discriminant, - to_clif_ty, - to_ty.is_signed(), - ); - lval.write_cvalue(fx, CValue::by_val(res, dest_layout)); } else { let to_clif_ty = fx.clif_type(to_ty).unwrap(); let from = operand.load_scalar(fx); diff --git a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs index 6a87da3ba8910..e3a055b619a3b 100644 --- a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs +++ b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs @@ -12,7 +12,6 @@ use rustc_middle::ty::cast::{CastTy, IntTy}; use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf}; use rustc_middle::ty::{self, adjustment::PointerCast, Instance, Ty, TyCtxt}; use rustc_span::source_map::{Span, DUMMY_SP}; -use rustc_target::abi::{Abi, Int, Variants}; impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { #[instrument(level = "debug", skip(self, bx))] @@ -283,74 +282,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { CastTy::from_ty(operand.layout.ty).expect("bad input type for cast"); let r_t_out = CastTy::from_ty(cast.ty).expect("bad output type for cast"); let ll_t_in = bx.cx().immediate_backend_type(operand.layout); - match operand.layout.variants { - Variants::Single { index } => { - if let Some(discr) = - operand.layout.ty.discriminant_for_variant(bx.tcx(), index) - { - let discr_layout = bx.cx().layout_of(discr.ty); - let discr_t = bx.cx().immediate_backend_type(discr_layout); - let discr_val = bx.cx().const_uint_big(discr_t, discr.val); - let discr_val = - bx.intcast(discr_val, ll_t_out, discr.ty.is_signed()); - - return ( - bx, - OperandRef { - val: OperandValue::Immediate(discr_val), - layout: cast, - }, - ); - } - } - Variants::Multiple { .. } => {} - } let llval = operand.immediate(); - let mut signed = false; - if let Abi::Scalar(scalar) = operand.layout.abi { - if let Int(_, s) = scalar.primitive() { - // We use `i1` for bytes that are always `0` or `1`, - // e.g., `#[repr(i8)] enum E { A, B }`, but we can't - // let LLVM interpret the `i1` as signed, because - // then `i1 1` (i.e., E::B) is effectively `i8 -1`. - signed = !scalar.is_bool() && s; - - if !scalar.is_always_valid(bx.cx()) - && scalar.valid_range(bx.cx()).end - >= scalar.valid_range(bx.cx()).start - { - // We want `table[e as usize ± k]` to not - // have bound checks, and this is the most - // convenient place to put the `assume`s. - if scalar.valid_range(bx.cx()).start > 0 { - let enum_value_lower_bound = bx.cx().const_uint_big( - ll_t_in, - scalar.valid_range(bx.cx()).start, - ); - let cmp_start = bx.icmp( - IntPredicate::IntUGE, - llval, - enum_value_lower_bound, - ); - bx.assume(cmp_start); - } - - let enum_value_upper_bound = bx - .cx() - .const_uint_big(ll_t_in, scalar.valid_range(bx.cx()).end); - let cmp_end = bx.icmp( - IntPredicate::IntULE, - llval, - enum_value_upper_bound, - ); - bx.assume(cmp_end); - } - } - } - let newval = match (r_t_in, r_t_out) { - (CastTy::Int(_), CastTy::Int(_)) => bx.intcast(llval, ll_t_out, signed), + (CastTy::Int(i), CastTy::Int(_)) => { + bx.intcast(llval, ll_t_out, matches!(i, IntTy::I)) + } (CastTy::Float, CastTy::Float) => { let srcsz = bx.cx().float_width(ll_t_in); let dstsz = bx.cx().float_width(ll_t_out); @@ -362,8 +299,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { llval } } - (CastTy::Int(_), CastTy::Float) => { - if signed { + (CastTy::Int(i), CastTy::Float) => { + if matches!(i, IntTy::I) { bx.sitofp(llval, ll_t_out) } else { bx.uitofp(llval, ll_t_out) @@ -372,8 +309,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { (CastTy::Ptr(_) | CastTy::FnPtr, CastTy::Ptr(_)) => { bx.pointercast(llval, ll_t_out) } - (CastTy::Int(_), CastTy::Ptr(_)) => { - let usize_llval = bx.intcast(llval, bx.cx().type_isize(), signed); + (CastTy::Int(i), CastTy::Ptr(_)) => { + let usize_llval = + bx.intcast(llval, bx.cx().type_isize(), matches!(i, IntTy::I)); bx.inttoptr(usize_llval, ll_t_out) } (CastTy::Float, CastTy::Int(IntTy::I)) => { diff --git a/compiler/rustc_const_eval/src/interpret/cast.rs b/compiler/rustc_const_eval/src/interpret/cast.rs index fb484fba9fd06..4835b7969b204 100644 --- a/compiler/rustc_const_eval/src/interpret/cast.rs +++ b/compiler/rustc_const_eval/src/interpret/cast.rs @@ -8,7 +8,7 @@ use rustc_middle::mir::CastKind; use rustc_middle::ty::adjustment::PointerCast; use rustc_middle::ty::layout::{IntegerExt, LayoutOf, TyAndLayout}; use rustc_middle::ty::{self, FloatTy, Ty, TypeAndMut}; -use rustc_target::abi::{Integer, Variants}; +use rustc_target::abi::Integer; use rustc_type_ir::sty::TyKind::*; use super::{ @@ -127,12 +127,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Float(FloatTy::F64) => { return Ok(self.cast_from_float(src.to_scalar()?.to_f64()?, cast_ty).into()); } - // The rest is integer/pointer-"like", including fn ptr casts and casts from enums that - // are represented as integers. + // The rest is integer/pointer-"like", including fn ptr casts _ => assert!( src.layout.ty.is_bool() || src.layout.ty.is_char() - || src.layout.ty.is_enum() || src.layout.ty.is_integral() || src.layout.ty.is_any_ptr(), "Unexpected cast from type {:?}", @@ -142,25 +140,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // # First handle non-scalar source values. - // Handle cast from a ZST enum (0 or 1 variants). - match src.layout.variants { - Variants::Single { index } => { - if src.layout.abi.is_uninhabited() { - // This is dead code, because an uninhabited enum is UB to - // instantiate. - throw_ub!(Unreachable); - } - if let Some(discr) = src.layout.ty.discriminant_for_variant(*self.tcx, index) { - assert!(src.layout.is_zst()); - let discr_layout = self.layout_of(discr.ty)?; - - let scalar = Scalar::from_uint(discr.val, discr_layout.layout.size()); - return Ok(self.cast_from_int_like(scalar, discr_layout, cast_ty)?.into()); - } - } - Variants::Multiple { .. } => {} - } - // Handle casting any ptr to raw ptr (might be a fat ptr). if src.layout.ty.is_any_ptr() && cast_ty.is_unsafe_ptr() { let dest_layout = self.layout_of(cast_ty)?; diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 66d66ea951033..dce4a68e811b0 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -7,9 +7,9 @@ use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::visit::NonUseContext::VarDebugInfo; use rustc_middle::mir::visit::{PlaceContext, Visitor}; use rustc_middle::mir::{ - traversal, AggregateKind, BasicBlock, BinOp, Body, BorrowKind, Local, Location, MirPass, - MirPhase, Operand, Place, PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, - StatementKind, Terminator, TerminatorKind, UnOp, START_BLOCK, + traversal, AggregateKind, BasicBlock, BinOp, Body, BorrowKind, CastKind, Local, Location, + MirPass, MirPhase, Operand, Place, PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, + Statement, StatementKind, Terminator, TerminatorKind, UnOp, START_BLOCK, }; use rustc_middle::ty::fold::BottomUpFolder; use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable}; @@ -361,6 +361,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ); } } + Rvalue::Ref(..) => {} Rvalue::Len(p) => { let pty = p.ty(&self.body.local_decls, self.tcx).ty; check_kinds!( @@ -503,7 +504,30 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { let a = operand.ty(&self.body.local_decls, self.tcx); check_kinds!(a, "Cannot shallow init type {:?}", ty::RawPtr(..)); } - _ => {} + Rvalue::Cast(kind, operand, target_type) => { + match kind { + CastKind::Misc => { + let op_ty = operand.ty(self.body, self.tcx); + if op_ty.is_enum() { + self.fail( + location, + format!( + "enum -> int casts should go through `Rvalue::Discriminant`: {operand:?}:{op_ty} as {target_type}", + ), + ); + } + } + // Nothing to check here + CastKind::PointerFromExposedAddress + | CastKind::PointerExposeAddress + | CastKind::Pointer(_) => {} + } + } + Rvalue::Repeat(_, _) + | Rvalue::ThreadLocalRef(_) + | Rvalue::AddressOf(_, _) + | Rvalue::NullaryOp(_, _) + | Rvalue::Discriminant(_) => {} } self.super_rvalue(rvalue, location); } diff --git a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs index 8e87ecd27d285..e3a383f86a76a 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs @@ -1,6 +1,7 @@ //! See docs in `build/expr/mod.rs`. use rustc_index::vec::Idx; +use rustc_middle::ty::util::IntTypeExt; use crate::build::expr::as_place::PlaceBase; use crate::build::expr::category::{Category, RvalueFunc}; @@ -190,7 +191,30 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } ExprKind::Cast { source } => { let source = &this.thir[source]; - let from_ty = CastTy::from_ty(source.ty); + + // Casting an enum to an integer is equivalent to computing the discriminant and casting the + // discriminant. Previously every backend had to repeat the logic for this operation. Now we + // create all the steps directly in MIR with operations all backends need to support anyway. + let (source, ty) = if let ty::Adt(adt_def, ..) = source.ty.kind() && adt_def.is_enum() { + let discr_ty = adt_def.repr().discr_type().to_ty(this.tcx); + let place = unpack!(block = this.as_place(block, source)); + let discr = this.temp(discr_ty, source.span); + this.cfg.push_assign( + block, + source_info, + discr, + Rvalue::Discriminant(place), + ); + + (Operand::Move(discr), discr_ty) + } else { + let ty = source.ty; + let source = unpack!( + block = this.as_operand(block, scope, source, None, NeedsTemporary::No) + ); + (source, ty) + }; + let from_ty = CastTy::from_ty(ty); let cast_ty = CastTy::from_ty(expr.ty); let cast_kind = match (from_ty, cast_ty) { (Some(CastTy::Ptr(_) | CastTy::FnPtr), Some(CastTy::Int(_))) => { @@ -201,9 +225,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } (_, _) => CastKind::Misc, }; - let source = unpack!( - block = this.as_operand(block, scope, source, None, NeedsTemporary::No) - ); block.and(Rvalue::Cast(cast_kind, source, expr.ty)) } ExprKind::Pointer { cast, source } => { diff --git a/src/test/codegen/enum-bounds-check-derived-idx.rs b/src/test/codegen/enum-bounds-check-derived-idx.rs index aa66c2ed08edb..fe02aeb5f6252 100644 --- a/src/test/codegen/enum-bounds-check-derived-idx.rs +++ b/src/test/codegen/enum-bounds-check-derived-idx.rs @@ -12,13 +12,15 @@ pub enum Bar { // CHECK-LABEL: @lookup_inc #[no_mangle] pub fn lookup_inc(buf: &[u8; 5], f: Bar) -> u8 { - // CHECK-NOT: panic_bounds_check + // FIXME: panic check can be removed by adding the assumes back after https://github.com/rust-lang/rust/pull/98332 + // CHECK: panic_bounds_check buf[f as usize + 1] } // CHECK-LABEL: @lookup_dec #[no_mangle] pub fn lookup_dec(buf: &[u8; 5], f: Bar) -> u8 { - // CHECK-NOT: panic_bounds_check + // FIXME: panic check can be removed by adding the assumes back after https://github.com/rust-lang/rust/pull/98332 + // CHECK: panic_bounds_check buf[f as usize - 1] } diff --git a/src/test/codegen/enum-bounds-check-issue-13926.rs b/src/test/codegen/enum-bounds-check-issue-13926.rs index b26945bc54940..1aec41d54411b 100644 --- a/src/test/codegen/enum-bounds-check-issue-13926.rs +++ b/src/test/codegen/enum-bounds-check-issue-13926.rs @@ -13,6 +13,7 @@ pub enum Exception { // CHECK-LABEL: @access #[no_mangle] pub fn access(array: &[usize; 12], exc: Exception) -> usize { - // CHECK-NOT: panic_bounds_check + // FIXME: panic check can be removed by adding the assumes back after https://github.com/rust-lang/rust/pull/98332 + // CHECK: panic_bounds_check array[(exc as u8 - 4) as usize] } diff --git a/src/test/codegen/enum-bounds-check-issue-82871.rs b/src/test/codegen/enum-bounds-check-issue-82871.rs index a1fa1387d9441..32fdc4a5f4fab 100644 --- a/src/test/codegen/enum-bounds-check-issue-82871.rs +++ b/src/test/codegen/enum-bounds-check-issue-82871.rs @@ -1,4 +1,4 @@ -// compile-flags: -O +// compile-flags: -C opt-level=0 #![crate_type = "lib"] @@ -9,7 +9,10 @@ pub enum E { // CHECK-LABEL: @index #[no_mangle] -pub fn index(x: &[u32; 3], ind: E) -> u32{ - // CHECK-NOT: panic_bounds_check +pub fn index(x: &[u32; 3], ind: E) -> u32 { + // Canary: we should be able to optimize out the bounds check, but we need + // to track the range of the discriminant result in order to be able to do that. + // oli-obk tried to add that, but that caused miscompilations all over the place. + // CHECK: panic_bounds_check x[ind as usize] } diff --git a/src/test/codegen/enum-bounds-check.rs b/src/test/codegen/enum-bounds-check.rs index 17322d5911b92..f85c6817deda3 100644 --- a/src/test/codegen/enum-bounds-check.rs +++ b/src/test/codegen/enum-bounds-check.rs @@ -21,6 +21,7 @@ pub enum Bar { // CHECK-LABEL: @lookup_unmodified #[no_mangle] pub fn lookup_unmodified(buf: &[u8; 5], f: Bar) -> u8 { - // CHECK-NOT: panic_bounds_check + // FIXME: panic check can be removed by adding the assumes back after https://github.com/rust-lang/rust/pull/98332 + // CHECK: panic_bounds_check buf[f as usize] } diff --git a/src/test/mir-opt/enum_cast.bar.mir_map.0.mir b/src/test/mir-opt/enum_cast.bar.mir_map.0.mir new file mode 100644 index 0000000000000..1b4a469135cb6 --- /dev/null +++ b/src/test/mir-opt/enum_cast.bar.mir_map.0.mir @@ -0,0 +1,13 @@ +// MIR for `bar` 0 mir_map + +fn bar(_1: Bar) -> usize { + debug bar => _1; // in scope 0 at $DIR/enum_cast.rs:22:8: 22:11 + let mut _0: usize; // return place in scope 0 at $DIR/enum_cast.rs:22:21: 22:26 + let mut _2: isize; // in scope 0 at $DIR/enum_cast.rs:23:5: 23:8 + + bb0: { + _2 = discriminant(_1); // scope 0 at $DIR/enum_cast.rs:23:5: 23:17 + _0 = move _2 as usize (Misc); // scope 0 at $DIR/enum_cast.rs:23:5: 23:17 + return; // scope 0 at $DIR/enum_cast.rs:24:2: 24:2 + } +} diff --git a/src/test/mir-opt/enum_cast.boo.mir_map.0.mir b/src/test/mir-opt/enum_cast.boo.mir_map.0.mir new file mode 100644 index 0000000000000..7724e89a22854 --- /dev/null +++ b/src/test/mir-opt/enum_cast.boo.mir_map.0.mir @@ -0,0 +1,13 @@ +// MIR for `boo` 0 mir_map + +fn boo(_1: Boo) -> usize { + debug boo => _1; // in scope 0 at $DIR/enum_cast.rs:26:8: 26:11 + let mut _0: usize; // return place in scope 0 at $DIR/enum_cast.rs:26:21: 26:26 + let mut _2: u8; // in scope 0 at $DIR/enum_cast.rs:27:5: 27:8 + + bb0: { + _2 = discriminant(_1); // scope 0 at $DIR/enum_cast.rs:27:5: 27:17 + _0 = move _2 as usize (Misc); // scope 0 at $DIR/enum_cast.rs:27:5: 27:17 + return; // scope 0 at $DIR/enum_cast.rs:28:2: 28:2 + } +} diff --git a/src/test/mir-opt/enum_cast.droppy.mir_map.0.mir b/src/test/mir-opt/enum_cast.droppy.mir_map.0.mir new file mode 100644 index 0000000000000..a9dcfadae75c7 --- /dev/null +++ b/src/test/mir-opt/enum_cast.droppy.mir_map.0.mir @@ -0,0 +1,54 @@ +// MIR for `droppy` 0 mir_map + +fn droppy() -> () { + let mut _0: (); // return place in scope 0 at $DIR/enum_cast.rs:39:13: 39:13 + let _1: (); // in scope 0 at $DIR/enum_cast.rs:40:5: 45:6 + let _2: Droppy; // in scope 0 at $DIR/enum_cast.rs:41:13: 41:14 + let mut _4: isize; // in scope 0 at $DIR/enum_cast.rs:44:17: 44:18 + let _5: Droppy; // in scope 0 at $DIR/enum_cast.rs:46:9: 46:10 + scope 1 { + debug x => _2; // in scope 1 at $DIR/enum_cast.rs:41:13: 41:14 + scope 2 { + debug y => _3; // in scope 2 at $DIR/enum_cast.rs:44:13: 44:14 + } + scope 3 { + let _3: usize; // in scope 3 at $DIR/enum_cast.rs:44:13: 44:14 + } + } + scope 4 { + debug z => _5; // in scope 4 at $DIR/enum_cast.rs:46:9: 46:10 + } + + bb0: { + StorageLive(_1); // scope 0 at $DIR/enum_cast.rs:40:5: 45:6 + StorageLive(_2); // scope 0 at $DIR/enum_cast.rs:41:13: 41:14 + _2 = Droppy::C; // scope 0 at $DIR/enum_cast.rs:41:17: 41:26 + FakeRead(ForLet(None), _2); // scope 0 at $DIR/enum_cast.rs:41:13: 41:14 + StorageLive(_3); // scope 3 at $DIR/enum_cast.rs:44:13: 44:14 + _4 = discriminant(_2); // scope 3 at $DIR/enum_cast.rs:44:17: 44:27 + _3 = move _4 as usize (Misc); // scope 3 at $DIR/enum_cast.rs:44:17: 44:27 + FakeRead(ForLet(None), _3); // scope 3 at $DIR/enum_cast.rs:44:13: 44:14 + _1 = const (); // scope 0 at $DIR/enum_cast.rs:40:5: 45:6 + StorageDead(_3); // scope 1 at $DIR/enum_cast.rs:45:5: 45:6 + drop(_2) -> [return: bb1, unwind: bb3]; // scope 0 at $DIR/enum_cast.rs:45:5: 45:6 + } + + bb1: { + StorageDead(_2); // scope 0 at $DIR/enum_cast.rs:45:5: 45:6 + StorageDead(_1); // scope 0 at $DIR/enum_cast.rs:45:5: 45:6 + StorageLive(_5); // scope 0 at $DIR/enum_cast.rs:46:9: 46:10 + _5 = Droppy::B; // scope 0 at $DIR/enum_cast.rs:46:13: 46:22 + FakeRead(ForLet(None), _5); // scope 0 at $DIR/enum_cast.rs:46:9: 46:10 + _0 = const (); // scope 0 at $DIR/enum_cast.rs:39:13: 47:2 + drop(_5) -> [return: bb2, unwind: bb3]; // scope 0 at $DIR/enum_cast.rs:47:1: 47:2 + } + + bb2: { + StorageDead(_5); // scope 0 at $DIR/enum_cast.rs:47:1: 47:2 + return; // scope 0 at $DIR/enum_cast.rs:47:2: 47:2 + } + + bb3 (cleanup): { + resume; // scope 0 at $DIR/enum_cast.rs:39:1: 47:2 + } +} diff --git a/src/test/mir-opt/enum_cast.foo.mir_map.0.mir b/src/test/mir-opt/enum_cast.foo.mir_map.0.mir new file mode 100644 index 0000000000000..d89dc9519239b --- /dev/null +++ b/src/test/mir-opt/enum_cast.foo.mir_map.0.mir @@ -0,0 +1,13 @@ +// MIR for `foo` 0 mir_map + +fn foo(_1: Foo) -> usize { + debug foo => _1; // in scope 0 at $DIR/enum_cast.rs:18:8: 18:11 + let mut _0: usize; // return place in scope 0 at $DIR/enum_cast.rs:18:21: 18:26 + let mut _2: isize; // in scope 0 at $DIR/enum_cast.rs:19:5: 19:8 + + bb0: { + _2 = discriminant(_1); // scope 0 at $DIR/enum_cast.rs:19:5: 19:17 + _0 = move _2 as usize (Misc); // scope 0 at $DIR/enum_cast.rs:19:5: 19:17 + return; // scope 0 at $DIR/enum_cast.rs:20:2: 20:2 + } +} diff --git a/src/test/mir-opt/enum_cast.rs b/src/test/mir-opt/enum_cast.rs new file mode 100644 index 0000000000000..090142aaf3559 --- /dev/null +++ b/src/test/mir-opt/enum_cast.rs @@ -0,0 +1,50 @@ +// EMIT_MIR enum_cast.foo.mir_map.0.mir +// EMIT_MIR enum_cast.bar.mir_map.0.mir +// EMIT_MIR enum_cast.boo.mir_map.0.mir + +enum Foo { + A +} + +enum Bar { + A, B +} + +#[repr(u8)] +enum Boo { + A, B +} + +fn foo(foo: Foo) -> usize { + foo as usize +} + +fn bar(bar: Bar) -> usize { + bar as usize +} + +fn boo(boo: Boo) -> usize { + boo as usize +} + +// EMIT_MIR enum_cast.droppy.mir_map.0.mir +enum Droppy { + A, B, C +} + +impl Drop for Droppy { + fn drop(&mut self) {} +} + +fn droppy() { + { + let x = Droppy::C; + // remove this entire test once `cenum_impl_drop_cast` becomes a hard error + #[allow(cenum_impl_drop_cast)] + let y = x as usize; + } + let z = Droppy::B; +} + +fn main() { +} diff --git a/src/test/run-pass-valgrind/cast-enum-with-dtor.rs b/src/test/run-pass-valgrind/cast-enum-with-dtor.rs index 998d202b11736..f29bc50e84c4e 100644 --- a/src/test/run-pass-valgrind/cast-enum-with-dtor.rs +++ b/src/test/run-pass-valgrind/cast-enum-with-dtor.rs @@ -30,5 +30,5 @@ fn main() { assert_eq!(e as u32, 2); assert_eq!(FLAG.load(Ordering::SeqCst), 0); } - assert_eq!(FLAG.load(Ordering::SeqCst), 0); + assert_eq!(FLAG.load(Ordering::SeqCst), 1); } From c3aec3056e3c88bfc63db5cd4dc24624be7c576a Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 5 Jul 2022 09:26:45 +0000 Subject: [PATCH 3/4] Add a helper method with an explicit name instead of hand rolling a match 3x --- compiler/rustc_codegen_ssa/src/mir/rvalue.rs | 6 +++--- compiler/rustc_middle/src/ty/cast.rs | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs index e3a055b619a3b..953787ab6c586 100644 --- a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs +++ b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs @@ -286,7 +286,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let newval = match (r_t_in, r_t_out) { (CastTy::Int(i), CastTy::Int(_)) => { - bx.intcast(llval, ll_t_out, matches!(i, IntTy::I)) + bx.intcast(llval, ll_t_out, i.is_signed()) } (CastTy::Float, CastTy::Float) => { let srcsz = bx.cx().float_width(ll_t_in); @@ -300,7 +300,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { } } (CastTy::Int(i), CastTy::Float) => { - if matches!(i, IntTy::I) { + if i.is_signed() { bx.sitofp(llval, ll_t_out) } else { bx.uitofp(llval, ll_t_out) @@ -311,7 +311,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { } (CastTy::Int(i), CastTy::Ptr(_)) => { let usize_llval = - bx.intcast(llval, bx.cx().type_isize(), matches!(i, IntTy::I)); + bx.intcast(llval, bx.cx().type_isize(), i.is_signed()); bx.inttoptr(usize_llval, ll_t_out) } (CastTy::Float, CastTy::Int(IntTy::I)) => { diff --git a/compiler/rustc_middle/src/ty/cast.rs b/compiler/rustc_middle/src/ty/cast.rs index 20a6af5f6c13b..c4b743dd46701 100644 --- a/compiler/rustc_middle/src/ty/cast.rs +++ b/compiler/rustc_middle/src/ty/cast.rs @@ -15,6 +15,12 @@ pub enum IntTy { Char, } +impl IntTy { + pub fn is_signed(self) -> bool { + matches!(self, Self::I) + } +} + // Valid types for the result of a non-coercion cast #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum CastTy<'tcx> { From 82c73af4a60c5c9b4ac92813a9f5e902ada67e6b Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 5 Jul 2022 09:27:06 +0000 Subject: [PATCH 4/4] Prefer trace level instrumentation for the new noisy instrument attributes --- compiler/rustc_codegen_llvm/src/builder.rs | 2 +- compiler/rustc_codegen_ssa/src/mir/place.rs | 2 +- compiler/rustc_codegen_ssa/src/mir/rvalue.rs | 2 +- compiler/rustc_mir_build/src/thir/cx/expr.rs | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 70cdf90303825..4a4cccb490d1c 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -464,7 +464,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { } } - #[instrument(level = "debug", skip(self))] + #[instrument(level = "trace", skip(self))] fn load_operand(&mut self, place: PlaceRef<'tcx, &'ll Value>) -> OperandRef<'tcx, &'ll Value> { assert_eq!(place.llextra.is_some(), place.layout.is_unsized()); diff --git a/compiler/rustc_codegen_ssa/src/mir/place.rs b/compiler/rustc_codegen_ssa/src/mir/place.rs index 95281b3d4c304..58cee0c8bb0db 100644 --- a/compiler/rustc_codegen_ssa/src/mir/place.rs +++ b/compiler/rustc_codegen_ssa/src/mir/place.rs @@ -421,7 +421,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { } impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { - #[instrument(level = "debug", skip(self, bx))] + #[instrument(level = "trace", skip(self, bx))] pub fn codegen_place( &mut self, bx: &mut Bx, diff --git a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs index 953787ab6c586..bb14268b1834e 100644 --- a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs +++ b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs @@ -14,7 +14,7 @@ use rustc_middle::ty::{self, adjustment::PointerCast, Instance, Ty, TyCtxt}; use rustc_span::source_map::{Span, DUMMY_SP}; impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { - #[instrument(level = "debug", skip(self, bx))] + #[instrument(level = "trace", skip(self, bx))] pub fn codegen_rvalue( &mut self, mut bx: Bx, diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index e1ca813bda1c6..4bc3d216a40d6 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -32,14 +32,14 @@ impl<'tcx> Cx<'tcx> { exprs.iter().map(|expr| self.mirror_expr_inner(expr)).collect() } - #[instrument(level = "debug", skip(self, hir_expr))] + #[instrument(level = "trace", skip(self, hir_expr))] pub(super) fn mirror_expr_inner(&mut self, hir_expr: &'tcx hir::Expr<'tcx>) -> ExprId { let temp_lifetime = self.rvalue_scopes.temporary_scope(self.region_scope_tree, hir_expr.hir_id.local_id); let expr_scope = region::Scope { id: hir_expr.hir_id.local_id, data: region::ScopeData::Node }; - debug!(?hir_expr.hir_id, ?hir_expr.span); + trace!(?hir_expr.hir_id, ?hir_expr.span); let mut expr = self.make_mirror_unadjusted(hir_expr);