Skip to content

Commit dd582bd

Browse files
cjgillotcjgillot
committed
Implement checked Shl/Shr at MIR building.
1 parent 43ee4d1 commit dd582bd

File tree

7 files changed

+109
-107
lines changed

7 files changed

+109
-107
lines changed

‎compiler/rustc_codegen_cranelift/src/num.rs

-23
Original file line numberDiff line numberDiff line change
@@ -170,14 +170,6 @@ pub(crate) fn codegen_checked_int_binop<'tcx>(
170170
in_lhs: CValue<'tcx>,
171171
in_rhs: CValue<'tcx>,
172172
) -> CValue<'tcx> {
173-
if bin_op != BinOp::Shl && bin_op != BinOp::Shr {
174-
assert_eq!(
175-
in_lhs.layout().ty,
176-
in_rhs.layout().ty,
177-
"checked int binop requires lhs and rhs of same type"
178-
);
179-
}
180-
181173
let lhs = in_lhs.load_scalar(fx);
182174
let rhs = in_rhs.load_scalar(fx);
183175

@@ -271,21 +263,6 @@ pub(crate) fn codegen_checked_int_binop<'tcx>(
271263
_ => unreachable!("invalid non-integer type {}", ty),
272264
}
273265
}
274-
BinOp::Shl => {
275-
let val = fx.bcx.ins().ishl(lhs, rhs);
276-
let ty = fx.bcx.func.dfg.value_type(val);
277-
let max_shift = i64::from(ty.bits()) - 1;
278-
let has_overflow = fx.bcx.ins().icmp_imm(IntCC::UnsignedGreaterThan, rhs, max_shift);
279-
(val, has_overflow)
280-
}
281-
BinOp::Shr => {
282-
let val =
283-
if !signed { fx.bcx.ins().ushr(lhs, rhs) } else { fx.bcx.ins().sshr(lhs, rhs) };
284-
let ty = fx.bcx.func.dfg.value_type(val);
285-
let max_shift = i64::from(ty.bits()) - 1;
286-
let has_overflow = fx.bcx.ins().icmp_imm(IntCC::UnsignedGreaterThan, rhs, max_shift);
287-
(val, has_overflow)
288-
}
289266
_ => bug!("binop {:?} on checked int/uint lhs: {:?} rhs: {:?}", bin_op, in_lhs, in_rhs),
290267
};
291268

‎compiler/rustc_codegen_ssa/src/mir/rvalue.rs

-11
Original file line numberDiff line numberDiff line change
@@ -663,17 +663,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
663663
};
664664
bx.checked_binop(oop, input_ty, lhs, rhs)
665665
}
666-
mir::BinOp::Shl | mir::BinOp::Shr => {
667-
let lhs_llty = bx.cx().val_ty(lhs);
668-
let rhs_llty = bx.cx().val_ty(rhs);
669-
let invert_mask = common::shift_mask_val(bx, lhs_llty, rhs_llty, true);
670-
let outer_bits = bx.and(rhs, invert_mask);
671-
672-
let of = bx.icmp(IntPredicate::IntNE, outer_bits, bx.cx().const_null(rhs_llty));
673-
let val = self.codegen_scalar_binop(bx, op, lhs, rhs, input_ty);
674-
675-
(val, of)
676-
}
677666
_ => bug!("Operator `{:?}` is not a checkable operator", op),
678667
};
679668

‎compiler/rustc_const_eval/src/transform/validate.rs

-9
Original file line numberDiff line numberDiff line change
@@ -553,15 +553,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
553553
);
554554
}
555555
}
556-
Shl | Shr => {
557-
for x in [a, b] {
558-
check_kinds!(
559-
x,
560-
"Cannot perform checked shift on non-integer type {:?}",
561-
ty::Uint(..) | ty::Int(..)
562-
)
563-
}
564-
}
565556
_ => self.fail(location, format!("There is no checked version of {:?}", op)),
566557
}
567558
}

‎compiler/rustc_middle/src/ty/util.rs

+12-12
Original file line numberDiff line numberDiff line change
@@ -59,22 +59,13 @@ impl<'tcx> fmt::Display for Discr<'tcx> {
5959
}
6060
}
6161

62-
fn int_size_and_signed<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> (Size, bool) {
63-
let (int, signed) = match *ty.kind() {
64-
ty::Int(ity) => (Integer::from_int_ty(&tcx, ity), true),
65-
ty::Uint(uty) => (Integer::from_uint_ty(&tcx, uty), false),
66-
_ => bug!("non integer discriminant"),
67-
};
68-
(int.size(), signed)
69-
}
70-
7162
impl<'tcx> Discr<'tcx> {
7263
/// Adds `1` to the value and wraps around if the maximum for the type is reached.
7364
pub fn wrap_incr(self, tcx: TyCtxt<'tcx>) -> Self {
7465
self.checked_add(tcx, 1).0
7566
}
7667
pub fn checked_add(self, tcx: TyCtxt<'tcx>, n: u128) -> (Self, bool) {
77-
let (size, signed) = int_size_and_signed(tcx, self.ty);
68+
let (size, signed) = self.ty.int_size_and_signed(tcx);
7869
let (val, oflo) = if signed {
7970
let min = size.signed_int_min();
8071
let max = size.signed_int_max();
@@ -922,12 +913,21 @@ impl<'tcx> TypeFolder<TyCtxt<'tcx>> for OpaqueTypeExpander<'tcx> {
922913
}
923914

924915
impl<'tcx> Ty<'tcx> {
916+
pub fn int_size_and_signed(self, tcx: TyCtxt<'tcx>) -> (Size, bool) {
917+
let (int, signed) = match *self.kind() {
918+
ty::Int(ity) => (Integer::from_int_ty(&tcx, ity), true),
919+
ty::Uint(uty) => (Integer::from_uint_ty(&tcx, uty), false),
920+
_ => bug!("non integer discriminant"),
921+
};
922+
(int.size(), signed)
923+
}
924+
925925
/// Returns the maximum value for the given numeric type (including `char`s)
926926
/// or returns `None` if the type is not numeric.
927927
pub fn numeric_max_val(self, tcx: TyCtxt<'tcx>) -> Option<ty::Const<'tcx>> {
928928
let val = match self.kind() {
929929
ty::Int(_) | ty::Uint(_) => {
930-
let (size, signed) = int_size_and_signed(tcx, self);
930+
let (size, signed) = self.int_size_and_signed(tcx);
931931
let val =
932932
if signed { size.signed_int_max() as u128 } else { size.unsigned_int_max() };
933933
Some(val)
@@ -948,7 +948,7 @@ impl<'tcx> Ty<'tcx> {
948948
pub fn numeric_min_val(self, tcx: TyCtxt<'tcx>) -> Option<ty::Const<'tcx>> {
949949
let val = match self.kind() {
950950
ty::Int(_) | ty::Uint(_) => {
951-
let (size, signed) = int_size_and_signed(tcx, self);
951+
let (size, signed) = self.int_size_and_signed(tcx);
952952
let val = if signed { size.truncate(size.signed_int_min() as u128) } else { 0 };
953953
Some(val)
954954
}

‎compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

+63-22
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use crate::build::expr::category::{Category, RvalueFunc};
99
use crate::build::{BlockAnd, BlockAndExtension, Builder, NeedsTemporary};
1010
use rustc_hir::lang_items::LangItem;
1111
use rustc_middle::middle::region;
12+
use rustc_middle::mir::interpret::Scalar;
1213
use rustc_middle::mir::AssertKind;
1314
use rustc_middle::mir::Place;
1415
use rustc_middle::mir::*;
@@ -519,30 +520,68 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
519520
) -> BlockAnd<Rvalue<'tcx>> {
520521
let source_info = self.source_info(span);
521522
let bool_ty = self.tcx.types.bool;
522-
if self.check_overflow && op.is_checkable() && ty.is_integral() {
523-
let result_tup = self.tcx.mk_tup(&[ty, bool_ty]);
524-
let result_value = self.temp(result_tup, span);
523+
let rvalue = match op {
524+
BinOp::Add | BinOp::Sub | BinOp::Mul if self.check_overflow && ty.is_integral() => {
525+
let result_tup = self.tcx.mk_tup(&[ty, bool_ty]);
526+
let result_value = self.temp(result_tup, span);
525527

526-
self.cfg.push_assign(
527-
block,
528-
source_info,
529-
result_value,
530-
Rvalue::CheckedBinaryOp(op, Box::new((lhs.to_copy(), rhs.to_copy()))),
531-
);
532-
let val_fld = Field::new(0);
533-
let of_fld = Field::new(1);
528+
self.cfg.push_assign(
529+
block,
530+
source_info,
531+
result_value,
532+
Rvalue::CheckedBinaryOp(op, Box::new((lhs.to_copy(), rhs.to_copy()))),
533+
);
534+
let val_fld = Field::new(0);
535+
let of_fld = Field::new(1);
536+
537+
let tcx = self.tcx;
538+
let val = tcx.mk_place_field(result_value, val_fld, ty);
539+
let of = tcx.mk_place_field(result_value, of_fld, bool_ty);
534540

535-
let tcx = self.tcx;
536-
let val = tcx.mk_place_field(result_value, val_fld, ty);
537-
let of = tcx.mk_place_field(result_value, of_fld, bool_ty);
541+
let err = AssertKind::Overflow(op, lhs, rhs);
542+
block = self.assert(block, Operand::Move(of), false, err, span);
538543

539-
let err = AssertKind::Overflow(op, lhs, rhs);
544+
Rvalue::Use(Operand::Move(val))
545+
}
546+
BinOp::Shl | BinOp::Shr if self.check_overflow && ty.is_integral() => {
547+
// Consider that the shift overflows if `rhs < 0` or `rhs >= bits`.
548+
// This can be encoded as a single operation as `(rhs & -bits) != 0`.
549+
let (size, _) = ty.int_size_and_signed(self.tcx);
550+
let bits = size.bits();
551+
debug_assert!(bits.is_power_of_two());
552+
let mask = !((bits - 1) as u128);
553+
554+
let rhs_ty = rhs.ty(&self.local_decls, self.tcx);
555+
let (rhs_size, _) = rhs_ty.int_size_and_signed(self.tcx);
556+
let mask = Operand::const_from_scalar(
557+
self.tcx,
558+
rhs_ty,
559+
Scalar::from_uint(rhs_size.truncate(mask), rhs_size),
560+
span,
561+
);
540562

541-
block = self.assert(block, Operand::Move(of), false, err, span);
563+
let outer_bits = self.temp(rhs_ty, span);
564+
self.cfg.push_assign(
565+
block,
566+
source_info,
567+
outer_bits,
568+
Rvalue::BinaryOp(BinOp::BitAnd, Box::new((rhs.to_copy(), mask))),
569+
);
542570

543-
block.and(Rvalue::Use(Operand::Move(val)))
544-
} else {
545-
if ty.is_integral() && (op == BinOp::Div || op == BinOp::Rem) {
571+
let overflows = self.temp(bool_ty, span);
572+
let zero = self.zero_literal(span, rhs_ty);
573+
self.cfg.push_assign(
574+
block,
575+
source_info,
576+
overflows,
577+
Rvalue::BinaryOp(BinOp::Ne, Box::new((Operand::Move(outer_bits), zero))),
578+
);
579+
580+
let overflow_err = AssertKind::Overflow(op, lhs.to_copy(), rhs.to_copy());
581+
block = self.assert(block, Operand::Move(overflows), false, overflow_err, span);
582+
Rvalue::BinaryOp(op, Box::new((lhs, rhs)))
583+
}
584+
BinOp::Div | BinOp::Rem if ty.is_integral() => {
546585
// Checking division and remainder is more complex, since we 1. always check
547586
// and 2. there are two possible failure cases, divide-by-zero and overflow.
548587

@@ -601,10 +640,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
601640

602641
block = self.assert(block, Operand::Move(of), false, overflow_err, span);
603642
}
604-
}
605643

606-
block.and(Rvalue::BinaryOp(op, Box::new((lhs, rhs))))
607-
}
644+
Rvalue::BinaryOp(op, Box::new((lhs, rhs)))
645+
}
646+
_ => Rvalue::BinaryOp(op, Box::new((lhs, rhs))),
647+
};
648+
block.and(rvalue)
608649
}
609650

610651
fn build_zero_repeat(

‎tests/mir-opt/issue_101973.inner.ConstProp.diff

+31-27
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,14 @@
1212
let mut _7: u32; // in scope 0 at $DIR/issue_101973.rs:+1:31: +1:52
1313
let mut _8: u32; // in scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
1414
let mut _9: u32; // in scope 0 at $DIR/issue_101973.rs:+1:33: +1:39
15-
let mut _10: (u32, bool); // in scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
16-
let mut _11: (u32, bool); // in scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
15+
let mut _10: i32; // in scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
16+
let mut _11: bool; // in scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
17+
let mut _12: i32; // in scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
18+
let mut _13: bool; // in scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
1719
scope 1 (inlined imm8) { // at $DIR/issue_101973.rs:14:5: 14:17
1820
debug x => _1; // in scope 1 at $DIR/issue_101973.rs:5:13: 5:14
19-
let mut _12: u32; // in scope 1 at $DIR/issue_101973.rs:7:12: 7:27
20-
let mut _13: u32; // in scope 1 at $DIR/issue_101973.rs:7:12: 7:20
21-
let mut _14: (u32, bool); // in scope 1 at $DIR/issue_101973.rs:7:12: 7:20
21+
let mut _14: u32; // in scope 1 at $DIR/issue_101973.rs:7:12: 7:27
22+
let mut _15: u32; // in scope 1 at $DIR/issue_101973.rs:7:12: 7:20
2223
scope 2 {
2324
debug out => _4; // in scope 2 at $DIR/issue_101973.rs:6:9: 6:16
2425
}
@@ -32,43 +33,46 @@
3233
StorageLive(_2); // scope 0 at $DIR/issue_101973.rs:+1:5: +1:65
3334
StorageLive(_3); // scope 0 at $DIR/issue_101973.rs:+1:5: +1:58
3435
StorageLive(_4); // scope 0 at $DIR/issue_101973.rs:+1:5: +1:17
35-
StorageLive(_12); // scope 2 at $DIR/issue_101973.rs:7:12: 7:27
36-
StorageLive(_13); // scope 2 at $DIR/issue_101973.rs:7:12: 7:20
37-
_14 = CheckedShr(_1, const 0_i32); // scope 2 at $DIR/issue_101973.rs:7:12: 7:20
38-
assert(!move (_14.1: bool), "attempt to shift right by `{}`, which would overflow", const 0_i32) -> bb3; // scope 2 at $DIR/issue_101973.rs:7:12: 7:20
36+
StorageLive(_14); // scope 2 at $DIR/issue_101973.rs:7:12: 7:27
37+
StorageLive(_15); // scope 2 at $DIR/issue_101973.rs:7:12: 7:20
38+
_15 = Shr(_1, const 0_i32); // scope 2 at $DIR/issue_101973.rs:7:12: 7:20
39+
_14 = BitAnd(move _15, const 255_u32); // scope 2 at $DIR/issue_101973.rs:7:12: 7:27
40+
StorageDead(_15); // scope 2 at $DIR/issue_101973.rs:7:26: 7:27
41+
_4 = BitOr(const 0_u32, move _14); // scope 2 at $DIR/issue_101973.rs:7:5: 7:27
42+
StorageDead(_14); // scope 2 at $DIR/issue_101973.rs:7:26: 7:27
43+
StorageLive(_6); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
44+
StorageLive(_7); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:52
45+
StorageLive(_8); // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
46+
- _10 = BitAnd(const 8_i32, const -32_i32); // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
47+
- _11 = Ne(move _10, const 0_i32); // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
48+
- assert(!move _11, "attempt to shift right by `{}`, which would overflow", const 8_i32) -> bb1; // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
49+
+ _10 = const 0_i32; // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
50+
+ _11 = const false; // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
51+
+ assert(!const false, "attempt to shift right by `{}`, which would overflow", const 8_i32) -> bb1; // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
3952
}
4053

4154
bb1: {
42-
_8 = move (_10.0: u32); // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
55+
_8 = Shr(_1, const 8_i32); // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
4356
_7 = BitAnd(move _8, const 15_u32); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:52
4457
StorageDead(_8); // scope 0 at $DIR/issue_101973.rs:+1:51: +1:52
45-
_11 = CheckedShl(_7, const 1_i32); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
46-
assert(!move (_11.1: bool), "attempt to shift left by `{}`, which would overflow", const 1_i32) -> bb2; // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
58+
- _12 = BitAnd(const 1_i32, const -32_i32); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
59+
- _13 = Ne(move _12, const 0_i32); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
60+
- assert(!move _13, "attempt to shift left by `{}`, which would overflow", const 1_i32) -> bb2; // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
61+
+ _12 = const 0_i32; // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
62+
+ _13 = const false; // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
63+
+ assert(!const false, "attempt to shift left by `{}`, which would overflow", const 1_i32) -> bb2; // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
4764
}
4865

4966
bb2: {
50-
_6 = move (_11.0: u32); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
67+
_6 = Shl(move _7, const 1_i32); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
5168
StorageDead(_7); // scope 0 at $DIR/issue_101973.rs:+1:56: +1:57
52-
_3 = rotate_right::<u32>(_4, _6) -> bb4; // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
69+
_3 = rotate_right::<u32>(_4, _6) -> bb3; // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
5370
// mir::Constant
5471
// + span: $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
5572
// + literal: Const { ty: extern "rust-intrinsic" fn(u32, u32) -> u32 {rotate_right::<u32>}, val: Value(<ZST>) }
5673
}
5774

5875
bb3: {
59-
_13 = move (_14.0: u32); // scope 2 at $DIR/issue_101973.rs:7:12: 7:20
60-
_12 = BitAnd(move _13, const 255_u32); // scope 2 at $DIR/issue_101973.rs:7:12: 7:27
61-
StorageDead(_13); // scope 2 at $DIR/issue_101973.rs:7:26: 7:27
62-
_4 = BitOr(const 0_u32, move _12); // scope 2 at $DIR/issue_101973.rs:7:5: 7:27
63-
StorageDead(_12); // scope 2 at $DIR/issue_101973.rs:7:26: 7:27
64-
StorageLive(_6); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:57
65-
StorageLive(_7); // scope 0 at $DIR/issue_101973.rs:+1:31: +1:52
66-
StorageLive(_8); // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
67-
_10 = CheckedShr(_1, const 8_i32); // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
68-
assert(!move (_10.1: bool), "attempt to shift right by `{}`, which would overflow", const 8_i32) -> bb1; // scope 0 at $DIR/issue_101973.rs:+1:32: +1:45
69-
}
70-
71-
bb4: {
7276
StorageDead(_6); // scope 0 at $DIR/issue_101973.rs:+1:57: +1:58
7377
StorageDead(_4); // scope 0 at $DIR/issue_101973.rs:+1:57: +1:58
7478
_2 = move _3 as i32 (IntToInt); // scope 0 at $DIR/issue_101973.rs:+1:5: +1:65

‎tests/ui/consts/offset_from_ub.stderr

+3-3
Original file line numberDiff line numberDiff line change
@@ -39,19 +39,19 @@ error[E0080]: evaluation of constant value failed
3939
--> $DIR/offset_from_ub.rs:53:14
4040
|
4141
LL | unsafe { ptr_offset_from(end_ptr, start_ptr) }
42-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds offset_from: alloc18 has size 4, so pointer to 10 bytes starting at offset 0 is out-of-bounds
42+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds offset_from: alloc17 has size 4, so pointer to 10 bytes starting at offset 0 is out-of-bounds
4343

4444
error[E0080]: evaluation of constant value failed
4545
--> $DIR/offset_from_ub.rs:62:14
4646
|
4747
LL | unsafe { ptr_offset_from(start_ptr, end_ptr) }
48-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds offset_from: alloc21 has size 4, so pointer to 10 bytes starting at offset 0 is out-of-bounds
48+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds offset_from: alloc20 has size 4, so pointer to 10 bytes starting at offset 0 is out-of-bounds
4949

5050
error[E0080]: evaluation of constant value failed
5151
--> $DIR/offset_from_ub.rs:70:14
5252
|
5353
LL | unsafe { ptr_offset_from(end_ptr, end_ptr) }
54-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds offset_from: alloc24 has size 4, so pointer at offset 10 is out-of-bounds
54+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds offset_from: alloc23 has size 4, so pointer at offset 10 is out-of-bounds
5555

5656
error[E0080]: evaluation of constant value failed
5757
--> $DIR/offset_from_ub.rs:79:14

0 commit comments

Comments
 (0)