Skip to content

Commit 28a0b25

Browse files
borsbors
committed
Auto merge of #23536 - pnkfelix:arith-oflo-shifts, r=nikomatsakis
overflow-checking for rhs of shift operators Subtask of #22020 ([RFC 560](https://github.com/rust-lang/rfcs/blob/master/text/0560-integer-overflow.md))
2 parents 809a554 + bb9d210 commit 28a0b25

13 files changed

+449
-30
lines changed

‎src/librustc_trans/trans/base.rs

+9-9
Original file line numberDiff line numberDiff line change
@@ -756,7 +756,7 @@ pub fn iter_structural_ty<'blk, 'tcx, F>(cx: Block<'blk, 'tcx>,
756756
}
757757

758758
pub fn cast_shift_expr_rhs(cx: Block,
759-
op: ast::BinOp,
759+
op: ast::BinOp_,
760760
lhs: ValueRef,
761761
rhs: ValueRef)
762762
-> ValueRef {
@@ -765,24 +765,24 @@ pub fn cast_shift_expr_rhs(cx: Block,
765765
|a,b| ZExt(cx, a, b))
766766
}
767767

768-
pub fn cast_shift_const_rhs(op: ast::BinOp,
768+
pub fn cast_shift_const_rhs(op: ast::BinOp_,
769769
lhs: ValueRef, rhs: ValueRef) -> ValueRef {
770770
cast_shift_rhs(op, lhs, rhs,
771771
|a, b| unsafe { llvm::LLVMConstTrunc(a, b.to_ref()) },
772772
|a, b| unsafe { llvm::LLVMConstZExt(a, b.to_ref()) })
773773
}
774774

775-
pub fn cast_shift_rhs<F, G>(op: ast::BinOp,
776-
lhs: ValueRef,
777-
rhs: ValueRef,
778-
trunc: F,
779-
zext: G)
780-
-> ValueRef where
775+
fn cast_shift_rhs<F, G>(op: ast::BinOp_,
776+
lhs: ValueRef,
777+
rhs: ValueRef,
778+
trunc: F,
779+
zext: G)
780+
-> ValueRef where
781781
F: FnOnce(ValueRef, Type) -> ValueRef,
782782
G: FnOnce(ValueRef, Type) -> ValueRef,
783783
{
784784
// Shifts may have any size int on the rhs
785-
if ast_util::is_shift_binop(op.node) {
785+
if ast_util::is_shift_binop(op) {
786786
let mut rhs_llty = val_ty(rhs);
787787
let mut lhs_llty = val_ty(lhs);
788788
if rhs_llty.kind() == Vector { rhs_llty = rhs_llty.element_type() }

‎src/librustc_trans/trans/consts.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,7 @@ fn const_expr_unadjusted<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
376376
let signed = ty::type_is_signed(intype);
377377

378378
let (te2, _) = const_expr(cx, &**e2, param_substs);
379-
let te2 = base::cast_shift_const_rhs(b, te1, te2);
379+
let te2 = base::cast_shift_const_rhs(b.node, te1, te2);
380380

381381
match b.node {
382382
ast::BiAdd => {

‎src/librustc_trans/trans/expr.rs

+156-19
Original file line numberDiff line numberDiff line change
@@ -1770,7 +1770,6 @@ fn trans_eager_binop<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
17701770
};
17711771
let is_float = ty::type_is_fp(intype);
17721772
let is_signed = ty::type_is_signed(intype);
1773-
let rhs = base::cast_shift_expr_rhs(bcx, op, lhs, rhs);
17741773
let info = expr_info(binop_expr);
17751774

17761775
let binop_debug_loc = binop_expr.debug_loc();
@@ -1843,13 +1842,17 @@ fn trans_eager_binop<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
18431842
ast::BiBitOr => Or(bcx, lhs, rhs, binop_debug_loc),
18441843
ast::BiBitAnd => And(bcx, lhs, rhs, binop_debug_loc),
18451844
ast::BiBitXor => Xor(bcx, lhs, rhs, binop_debug_loc),
1846-
ast::BiShl => Shl(bcx, lhs, rhs, binop_debug_loc),
1845+
ast::BiShl => {
1846+
let (newbcx, res) = with_overflow_check(
1847+
bcx, OverflowOp::Shl, info, lhs_t, lhs, rhs, binop_debug_loc);
1848+
bcx = newbcx;
1849+
res
1850+
}
18471851
ast::BiShr => {
1848-
if is_signed {
1849-
AShr(bcx, lhs, rhs, binop_debug_loc)
1850-
} else {
1851-
LShr(bcx, lhs, rhs, binop_debug_loc)
1852-
}
1852+
let (newbcx, res) = with_overflow_check(
1853+
bcx, OverflowOp::Shr, info, lhs_t, lhs, rhs, binop_debug_loc);
1854+
bcx = newbcx;
1855+
res
18531856
}
18541857
ast::BiEq | ast::BiNe | ast::BiLt | ast::BiGe | ast::BiLe | ast::BiGt => {
18551858
if is_simd {
@@ -2389,9 +2392,38 @@ enum OverflowOp {
23892392
Add,
23902393
Sub,
23912394
Mul,
2395+
Shl,
2396+
Shr,
23922397
}
23932398

23942399
impl OverflowOp {
2400+
fn codegen_strategy(&self) -> OverflowCodegen {
2401+
use self::OverflowCodegen::{ViaIntrinsic, ViaInputCheck};
2402+
match *self {
2403+
OverflowOp::Add => ViaIntrinsic(OverflowOpViaIntrinsic::Add),
2404+
OverflowOp::Sub => ViaIntrinsic(OverflowOpViaIntrinsic::Sub),
2405+
OverflowOp::Mul => ViaIntrinsic(OverflowOpViaIntrinsic::Mul),
2406+
2407+
OverflowOp::Shl => ViaInputCheck(OverflowOpViaInputCheck::Shl),
2408+
OverflowOp::Shr => ViaInputCheck(OverflowOpViaInputCheck::Shr),
2409+
}
2410+
}
2411+
}
2412+
2413+
enum OverflowCodegen {
2414+
ViaIntrinsic(OverflowOpViaIntrinsic),
2415+
ViaInputCheck(OverflowOpViaInputCheck),
2416+
}
2417+
2418+
enum OverflowOpViaInputCheck { Shl, Shr, }
2419+
2420+
enum OverflowOpViaIntrinsic { Add, Sub, Mul, }
2421+
2422+
impl OverflowOpViaIntrinsic {
2423+
fn to_intrinsic<'blk, 'tcx>(&self, bcx: Block<'blk, 'tcx>, lhs_ty: Ty) -> ValueRef {
2424+
let name = self.to_intrinsic_name(bcx.tcx(), lhs_ty);
2425+
bcx.ccx().get_intrinsic(&name)
2426+
}
23952427
fn to_intrinsic_name(&self, tcx: &ty::ctxt, ty: Ty) -> &'static str {
23962428
use syntax::ast::IntTy::*;
23972429
use syntax::ast::UintTy::*;
@@ -2413,7 +2445,7 @@ impl OverflowOp {
24132445
};
24142446

24152447
match *self {
2416-
OverflowOp::Add => match new_sty {
2448+
OverflowOpViaIntrinsic::Add => match new_sty {
24172449
ty_int(TyI8) => "llvm.sadd.with.overflow.i8",
24182450
ty_int(TyI16) => "llvm.sadd.with.overflow.i16",
24192451
ty_int(TyI32) => "llvm.sadd.with.overflow.i32",
@@ -2426,7 +2458,7 @@ impl OverflowOp {
24262458

24272459
_ => unreachable!(),
24282460
},
2429-
OverflowOp::Sub => match new_sty {
2461+
OverflowOpViaIntrinsic::Sub => match new_sty {
24302462
ty_int(TyI8) => "llvm.ssub.with.overflow.i8",
24312463
ty_int(TyI16) => "llvm.ssub.with.overflow.i16",
24322464
ty_int(TyI32) => "llvm.ssub.with.overflow.i32",
@@ -2439,7 +2471,7 @@ impl OverflowOp {
24392471

24402472
_ => unreachable!(),
24412473
},
2442-
OverflowOp::Mul => match new_sty {
2474+
OverflowOpViaIntrinsic::Mul => match new_sty {
24432475
ty_int(TyI8) => "llvm.smul.with.overflow.i8",
24442476
ty_int(TyI16) => "llvm.smul.with.overflow.i16",
24452477
ty_int(TyI32) => "llvm.smul.with.overflow.i32",
@@ -2454,16 +2486,14 @@ impl OverflowOp {
24542486
},
24552487
}
24562488
}
2457-
}
24582489

2459-
2460-
fn with_overflow_check<'a, 'b>(bcx: Block<'a, 'b>, oop: OverflowOp, info: NodeIdAndSpan,
2461-
lhs_t: Ty, lhs: ValueRef, rhs: ValueRef, binop_debug_loc: DebugLoc)
2462-
-> (Block<'a, 'b>, ValueRef) {
2463-
if bcx.unreachable.get() { return (bcx, _Undef(lhs)); }
2464-
if bcx.ccx().check_overflow() {
2465-
let name = oop.to_intrinsic_name(bcx.tcx(), lhs_t);
2466-
let llfn = bcx.ccx().get_intrinsic(&name);
2490+
fn build_intrinsic_call<'blk, 'tcx>(&self, bcx: Block<'blk, 'tcx>,
2491+
info: NodeIdAndSpan,
2492+
lhs_t: Ty<'tcx>, lhs: ValueRef,
2493+
rhs: ValueRef,
2494+
binop_debug_loc: DebugLoc)
2495+
-> (Block<'blk, 'tcx>, ValueRef) {
2496+
let llfn = self.to_intrinsic(bcx, lhs_t);
24672497

24682498
let val = Call(bcx, llfn, &[lhs, rhs], None, binop_debug_loc);
24692499
let result = ExtractValue(bcx, val, 0); // iN operation result
@@ -2482,11 +2512,118 @@ fn with_overflow_check<'a, 'b>(bcx: Block<'a, 'b>, oop: OverflowOp, info: NodeId
24822512
InternedString::new("arithmetic operation overflowed")));
24832513

24842514
(bcx, result)
2515+
}
2516+
}
2517+
2518+
impl OverflowOpViaInputCheck {
2519+
fn build_with_input_check<'blk, 'tcx>(&self,
2520+
bcx: Block<'blk, 'tcx>,
2521+
info: NodeIdAndSpan,
2522+
lhs_t: Ty<'tcx>,
2523+
lhs: ValueRef,
2524+
rhs: ValueRef,
2525+
binop_debug_loc: DebugLoc)
2526+
-> (Block<'blk, 'tcx>, ValueRef)
2527+
{
2528+
let lhs_llty = val_ty(lhs);
2529+
let rhs_llty = val_ty(rhs);
2530+
2531+
// Panic if any bits are set outside of bits that we always
2532+
// mask in.
2533+
//
2534+
// Note that the mask's value is derived from the LHS type
2535+
// (since that is where the 32/64 distinction is relevant) but
2536+
// the mask's type must match the RHS type (since they will
2537+
// both be fed into a and-binop)
2538+
let invert_mask = !shift_mask_val(lhs_llty);
2539+
let invert_mask = C_integral(rhs_llty, invert_mask, true);
2540+
2541+
let outer_bits = And(bcx, rhs, invert_mask, binop_debug_loc);
2542+
let cond = ICmp(bcx, llvm::IntNE, outer_bits,
2543+
C_integral(rhs_llty, 0, false), binop_debug_loc);
2544+
let result = match *self {
2545+
OverflowOpViaInputCheck::Shl =>
2546+
build_unchecked_lshift(bcx, lhs, rhs, binop_debug_loc),
2547+
OverflowOpViaInputCheck::Shr =>
2548+
build_unchecked_rshift(bcx, lhs_t, lhs, rhs, binop_debug_loc),
2549+
};
2550+
let bcx =
2551+
base::with_cond(bcx, cond, |bcx|
2552+
controlflow::trans_fail(bcx, info,
2553+
InternedString::new("shift operation overflowed")));
2554+
2555+
(bcx, result)
2556+
}
2557+
}
2558+
2559+
fn shift_mask_val(llty: Type) -> u64 {
2560+
// i8/u8 can shift by at most 7, i16/u16 by at most 15, etc.
2561+
llty.int_width() - 1
2562+
}
2563+
2564+
// To avoid UB from LLVM, these two functions mask RHS with an
2565+
// appropriate mask unconditionally (i.e. the fallback behavior for
2566+
// all shifts). For 32- and 64-bit types, this matches the semantics
2567+
// of Java. (See related discussion on #1877 and #10183.)
2568+
2569+
fn build_unchecked_lshift<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
2570+
lhs: ValueRef,
2571+
rhs: ValueRef,
2572+
binop_debug_loc: DebugLoc) -> ValueRef {
2573+
let rhs = base::cast_shift_expr_rhs(bcx, ast::BinOp_::BiShl, lhs, rhs);
2574+
// #1877, #10183: Ensure that input is always valid
2575+
let rhs = shift_mask_rhs(bcx, rhs, binop_debug_loc);
2576+
Shl(bcx, lhs, rhs, binop_debug_loc)
2577+
}
2578+
2579+
fn build_unchecked_rshift<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
2580+
lhs_t: Ty<'tcx>,
2581+
lhs: ValueRef,
2582+
rhs: ValueRef,
2583+
binop_debug_loc: DebugLoc) -> ValueRef {
2584+
let rhs = base::cast_shift_expr_rhs(bcx, ast::BinOp_::BiShr, lhs, rhs);
2585+
// #1877, #10183: Ensure that input is always valid
2586+
let rhs = shift_mask_rhs(bcx, rhs, binop_debug_loc);
2587+
let is_signed = ty::type_is_signed(lhs_t);
2588+
if is_signed {
2589+
AShr(bcx, lhs, rhs, binop_debug_loc)
2590+
} else {
2591+
LShr(bcx, lhs, rhs, binop_debug_loc)
2592+
}
2593+
}
2594+
2595+
fn shift_mask_rhs<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
2596+
rhs: ValueRef,
2597+
debug_loc: DebugLoc) -> ValueRef {
2598+
let rhs_llty = val_ty(rhs);
2599+
let mask = shift_mask_val(rhs_llty);
2600+
And(bcx, rhs, C_integral(rhs_llty, mask, false), debug_loc)
2601+
}
2602+
2603+
fn with_overflow_check<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, oop: OverflowOp, info: NodeIdAndSpan,
2604+
lhs_t: Ty<'tcx>, lhs: ValueRef,
2605+
rhs: ValueRef,
2606+
binop_debug_loc: DebugLoc)
2607+
-> (Block<'blk, 'tcx>, ValueRef) {
2608+
if bcx.unreachable.get() { return (bcx, _Undef(lhs)); }
2609+
if bcx.ccx().check_overflow() {
2610+
2611+
match oop.codegen_strategy() {
2612+
OverflowCodegen::ViaIntrinsic(oop) =>
2613+
oop.build_intrinsic_call(bcx, info, lhs_t, lhs, rhs, binop_debug_loc),
2614+
OverflowCodegen::ViaInputCheck(oop) =>
2615+
oop.build_with_input_check(bcx, info, lhs_t, lhs, rhs, binop_debug_loc),
2616+
}
24852617
} else {
24862618
let res = match oop {
24872619
OverflowOp::Add => Add(bcx, lhs, rhs, binop_debug_loc),
24882620
OverflowOp::Sub => Sub(bcx, lhs, rhs, binop_debug_loc),
24892621
OverflowOp::Mul => Mul(bcx, lhs, rhs, binop_debug_loc),
2622+
2623+
OverflowOp::Shl =>
2624+
build_unchecked_lshift(bcx, lhs, rhs, binop_debug_loc),
2625+
OverflowOp::Shr =>
2626+
build_unchecked_rshift(bcx, lhs_t, lhs, rhs, binop_debug_loc),
24902627
};
24912628
(bcx, res)
24922629
}

‎src/test/run-fail/overflowing-lsh-1.rs

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// error-pattern:thread '<main>' panicked at 'shift operation overflowed'
12+
// compile-flags: -C debug-assertions
13+
14+
// (Work around constant-evaluation)
15+
fn id<T>(x: T) -> T { x }
16+
17+
fn main() {
18+
let _x = 1_i32 << id(32);
19+
}

‎src/test/run-fail/overflowing-lsh-2.rs

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// error-pattern:thread '<main>' panicked at 'shift operation overflowed'
12+
// compile-flags: -C debug-assertions
13+
14+
// (Work around constant-evaluation)
15+
fn id<T>(x: T) -> T { x }
16+
17+
fn main() {
18+
let _x = 1 << id(-1);
19+
}

‎src/test/run-fail/overflowing-lsh-3.rs

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// error-pattern:thread '<main>' panicked at 'shift operation overflowed'
12+
// compile-flags: -C debug-assertions
13+
14+
// (Work around constant-evaluation)
15+
fn id<T>(x: T) -> T { x }
16+
17+
fn main() {
18+
let _x = 1_u64 << id(64);
19+
}

‎src/test/run-fail/overflowing-lsh-4.rs

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// error-pattern:thread '<main>' panicked at 'shift operation overflowed'
12+
// compile-flags: -C debug-assertions
13+
14+
// This function is checking that our automatic truncation does not
15+
// sidestep the overflow checking.
16+
17+
// (Work around constant-evaluation)
18+
fn id<T>(x: T) -> T { x }
19+
20+
fn main() {
21+
// this signals overflow when checking is on
22+
let x = 1_i8 << id(17);
23+
24+
// ... but when checking is off, the fallback will truncate the
25+
// input to its lower three bits (= 1). Note that this is *not*
26+
// the behavior of the x86 processor for 8- and 16-bit types,
27+
// but it is necessary to avoid undefined behavior from LLVM.
28+
//
29+
// We check that here, by ensuring the result has only been
30+
// shifted by one place; if overflow checking is turned off, then
31+
// this assertion will pass (and the compiletest driver will
32+
// report that the test did not produce the error expected above).
33+
assert_eq!(x, 2_i8);
34+
}

0 commit comments

Comments
 (0)