Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Start emitting overflow lints for const promoted SHL & SHRs #119339

Closed
wants to merge 2 commits into from
Closed

Start emitting overflow lints for const promoted SHL & SHRs #119339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f6a80e
Emit `AssertLint` for overflow asserts found in MIR
gurry Dec 27, 2023
2d8b401
Remove the now redundant checks for shifts in check_binary_op()
gurry Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 28 additions & 41 deletions compiler/rustc_mir_transform/src/const_prop_lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::*;
use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout};
use rustc_middle::ty::GenericArgs;
use rustc_middle::ty::{
self, ConstInt, Instance, ParamEnv, ScalarInt, Ty, TyCtxt, TypeVisitableExt,
};
use rustc_middle::ty::{self, Instance, ParamEnv, Ty, TyCtxt, TypeVisitableExt};
use rustc_span::Span;
use rustc_target::abi::{HasDataLayout, Size, TargetDataLayout};

Expand Down Expand Up @@ -312,43 +310,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
right: &Operand<'tcx>,
location: Location,
) -> Option<()> {
if matches!(op, BinOp::Shr | BinOp::Shl) {
// Shifts are linted in check_assertion() not here
return None;
}

let r = self.use_ecx(location, |this| {
this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?)
});
let l = self
.use_ecx(location, |this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?));
// Check for exceeding shifts *even if* we cannot evaluate the LHS.
if matches!(op, BinOp::Shr | BinOp::Shl) {
let r = r.clone()?;
// We need the type of the LHS. We cannot use `place_layout` as that is the type
// of the result, which for checked binops is not the same!
let left_ty = left.ty(self.local_decls(), self.tcx);
let left_size = self.ecx.layout_of(left_ty).ok()?.size;
let right_size = r.layout.size;
let r_bits = r.to_scalar().to_bits(right_size).ok();
if r_bits.is_some_and(|b| b >= left_size.bits() as u128) {
debug!("check_binary_op: reporting assert for {:?}", location);
let source_info = self.body().source_info(location);
let panic = AssertKind::Overflow(
op,
match l {
Some(l) => l.to_const_int(),
// Invent a dummy value, the diagnostic ignores it anyway
None => ConstInt::new(
ScalarInt::try_from_uint(1_u8, left_size).unwrap(),
left_ty.is_signed(),
left_ty.is_ptr_sized_integral(),
),
},
r.to_const_int(),
);
self.report_assert_as_lint(
source_info,
AssertLint::ArithmeticOverflow(source_info.span, panic),
);
return None;
}
}

if let (Some(l), Some(r)) = (l, r) {
// The remaining operators are handled through `overflowing_binary_op`.
Expand Down Expand Up @@ -485,18 +456,30 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
.and_then(|op| self.ecx.read_immediate(&op).ok())
.map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int()))
};
let msg = match msg {
AssertKind::DivisionByZero(op) => AssertKind::DivisionByZero(eval_to_int(op)),
AssertKind::RemainderByZero(op) => AssertKind::RemainderByZero(eval_to_int(op)),
let (msg, emit_overflow_lint) = match msg {
AssertKind::DivisionByZero(op) => {
(AssertKind::DivisionByZero(eval_to_int(op)), false)
}
AssertKind::RemainderByZero(op) => {
(AssertKind::RemainderByZero(eval_to_int(op)), false)
}
AssertKind::Overflow(bin_op @ (BinOp::Div | BinOp::Rem), op1, op2) => {
// Division overflow is *UB* in the MIR, and different than the
// other overflow checks.
AssertKind::Overflow(*bin_op, eval_to_int(op1), eval_to_int(op2))
(AssertKind::Overflow(*bin_op, eval_to_int(op1), eval_to_int(op2)), false)
}
AssertKind::Overflow(bin_op @ (BinOp::Shl | BinOp::Shr), op1, op2) => {
// A hack that fixes #117949
// Ideally check_binary_op() should check these shift ops,
// but it can't because they are getting removed from the MIR
// during const promotion. So we check the associated asserts
// here instead as they are not removed by promotion.
(AssertKind::Overflow(*bin_op, eval_to_int(op1), eval_to_int(op2)), true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you emit the lint here then you should be able to remove the shift handling from check_binary_op. That should also fix the duplicate lints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that, but it eliminates only 6 of the 59 new duplicates added by this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly did you try? I should look something like this:

diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs
index 99eecb567f2..d36798518e2 100644
--- a/compiler/rustc_mir_transform/src/const_prop_lint.rs
+++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs
@@ -317,37 +317,9 @@ fn check_binary_op(
         });
         let l = self
             .use_ecx(location, |this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?));
-        // Check for exceeding shifts *even if* we cannot evaluate the LHS.
+        // Shifts are linted via the `Assert` terminator, not this function.
         if matches!(op, BinOp::Shr | BinOp::Shl) {
-            let r = r.clone()?;
-            // We need the type of the LHS. We cannot use `place_layout` as that is the type
-            // of the result, which for checked binops is not the same!
-            let left_ty = left.ty(self.local_decls(), self.tcx);
-            let left_size = self.ecx.layout_of(left_ty).ok()?.size;
-            let right_size = r.layout.size;
-            let r_bits = r.to_scalar().to_bits(right_size).ok();
-            if r_bits.is_some_and(|b| b >= left_size.bits() as u128) {
-                debug!("check_binary_op: reporting assert for {:?}", location);
-                let source_info = self.body().source_info(location);
-                let panic = AssertKind::Overflow(
-                    op,
-                    match l {
-                        Some(l) => l.to_const_int(),
-                        // Invent a dummy value, the diagnostic ignores it anyway
-                        None => ConstInt::new(
-                            ScalarInt::try_from_uint(1_u8, left_size).unwrap(),
-                            left_ty.is_signed(),
-                            left_ty.is_ptr_sized_integral(),
-                        ),
-                    },
-                    r.to_const_int(),
-                );
-                self.report_assert_as_lint(
-                    source_info,
-                    AssertLint::ArithmeticOverflow(source_info.span, panic),
-                );
-                return None;
-            }
+            return None;
         }
 
         if let (Some(l), Some(r)) = (l, r) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I just got rid of the entire if block. Didn't realize I must return None to disable shift handling.

I have done that now in the latest commit I pushed. It has eliminated all duplicates. However, we have the opposite problem now. Many of the shift lints in release builds have been silenced possibly because we don't emit asserts in those builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of the shift lints in release builds have been silenced possibly because we don't emit asserts in those builds.

Ah... right. Yeah for debug/release consistency we really want to lint on the operation (shl/shr/add/...), not the assert. But then we need to lint in promoteds as well.

}
AssertKind::BoundsCheck { ref len, ref index } => {
let len = eval_to_int(len);
let index = eval_to_int(index);
AssertKind::BoundsCheck { len, index }
(AssertKind::BoundsCheck { len, index }, false)
}
// Remaining overflow errors are already covered by checks on the binary operators.
AssertKind::Overflow(..) | AssertKind::OverflowNeg(_) => return None,
Expand All @@ -506,7 +489,11 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let source_info = self.body().source_info(location);
self.report_assert_as_lint(
source_info,
AssertLint::UnconditionalPanic(source_info.span, msg),
if emit_overflow_lint {
AssertLint::ArithmeticOverflow(source_info.span, msg)
} else {
AssertLint::UnconditionalPanic(source_info.span, msg)
},
);
}

Expand Down
140 changes: 1 addition & 139 deletions tests/ui/lint/lint-exceeding-bitshifts.opt.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,143 +10,5 @@ note: the lint level is defined here
LL | #![warn(arithmetic_overflow)]
| ^^^^^^^^^^^^^^^^^^^

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:22:13
|
LL | let _ = x << 42;
| ^^^^^^^ attempt to shift left by `42_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:27:15
|
LL | let n = 1u8 << 8;
| ^^^^^^^^ attempt to shift left by `8_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:29:15
|
LL | let n = 1u16 << 16;
| ^^^^^^^^^^ attempt to shift left by `16_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:31:15
|
LL | let n = 1u32 << 32;
| ^^^^^^^^^^ attempt to shift left by `32_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:33:15
|
LL | let n = 1u64 << 64;
| ^^^^^^^^^^ attempt to shift left by `64_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:35:15
|
LL | let n = 1i8 << 8;
| ^^^^^^^^ attempt to shift left by `8_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:37:15
|
LL | let n = 1i16 << 16;
| ^^^^^^^^^^ attempt to shift left by `16_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:39:15
|
LL | let n = 1i32 << 32;
| ^^^^^^^^^^ attempt to shift left by `32_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:41:15
|
LL | let n = 1i64 << 64;
| ^^^^^^^^^^ attempt to shift left by `64_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:44:15
|
LL | let n = 1u8 >> 8;
| ^^^^^^^^ attempt to shift right by `8_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:46:15
|
LL | let n = 1u16 >> 16;
| ^^^^^^^^^^ attempt to shift right by `16_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:48:15
|
LL | let n = 1u32 >> 32;
| ^^^^^^^^^^ attempt to shift right by `32_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:50:15
|
LL | let n = 1u64 >> 64;
| ^^^^^^^^^^ attempt to shift right by `64_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:52:15
|
LL | let n = 1i8 >> 8;
| ^^^^^^^^ attempt to shift right by `8_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:54:15
|
LL | let n = 1i16 >> 16;
| ^^^^^^^^^^ attempt to shift right by `16_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:56:15
|
LL | let n = 1i32 >> 32;
| ^^^^^^^^^^ attempt to shift right by `32_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:58:15
|
LL | let n = 1i64 >> 64;
| ^^^^^^^^^^ attempt to shift right by `64_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:62:15
|
LL | let n = n << 8;
| ^^^^^^ attempt to shift left by `8_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:64:15
|
LL | let n = 1u8 << -8;
| ^^^^^^^^^ attempt to shift left by `-8_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:69:15
|
LL | let n = 1u8 << (4+4);
| ^^^^^^^^^^^^ attempt to shift left by `8_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:71:15
|
LL | let n = 1i64 >> [64][0];
| ^^^^^^^^^^^^^^^ attempt to shift right by `64_i32`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:77:15
|
LL | let n = 1_isize << BITS;
| ^^^^^^^^^^^^^^^ attempt to shift left by `%BITS%`, which would overflow

warning: this arithmetic operation will overflow
--> $DIR/lint-exceeding-bitshifts.rs:78:15
|
LL | let n = 1_usize << BITS;
| ^^^^^^^^^^^^^^^ attempt to shift left by `%BITS%`, which would overflow

warning: 24 warnings emitted
warning: 1 warning emitted

Loading