Skip to content

Commit

Permalink
Rollup merge of #69185 - RalfJung:const-prop-lints, r=oli-obk
Browse files Browse the repository at this point in the history
Unify and improve const-prop lints

Add a single helper method for all lints emitted by const-prop, and make that lint different from the CTFE `const_err` lint. Also consistently check overflow on *arithmetic*, not on the assertion, to make behavior the same for debug and release builds.

See [this summary comment](#69185 (comment)) for details and the latest status.

In terms of lint formatting, I went for what seems to be the better style: have a general message above the code, and then a specific message at the span:
```
error: this arithmetic operation will overflow
  --> $DIR/const-err2.rs:21:18
   |
LL |     let a_i128 = -std::i128::MIN;
   |                  ^^^^^^^^^^^^^^^ attempt to negate with overflow
```
We could also just have the specific message above and no text at the span if that is preferred.

I also converted some of the existing tests to use compiletest revisions, so that the same test can check a bunch of different compile flags.

Fixes #69020.
Helps with #69021: debug/release are now consistent, but the assoc-const test in that issue still fails (there is a FIXME in the PR for this). The reason seems to be that const-prop notices the assoc const in `T::N << 42` and does not even bother calling `const_prop` on that operation.
Has no effect on #61821; the duplication there has entirely different reasons.
  • Loading branch information
Centril authored Feb 20, 2020
2 parents b680a5e + 88d14bf commit d237e0f
Show file tree
Hide file tree
Showing 71 changed files with 1,613 additions and 1,109 deletions.
1 change: 1 addition & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
store.register_renamed("unstable_name_collision", "unstable_name_collisions");
store.register_renamed("unused_doc_comment", "unused_doc_comments");
store.register_renamed("async_idents", "keyword_idents");
store.register_renamed("exceeding_bitshifts", "arithmetic_overflow");
store.register_removed("unknown_features", "replaced by an error");
store.register_removed("unsigned_negation", "replaced by negate_unsigned feature gate");
store.register_removed("negate_unsigned", "cast a signed value instead");
Expand Down
177 changes: 86 additions & 91 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
use std::borrow::Cow;
use std::cell::Cell;

use rustc::mir::interpret::{InterpError, InterpResult, Scalar};
use rustc::lint;
use rustc::mir::interpret::{InterpResult, Scalar};
use rustc::mir::visit::{
MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor,
};
Expand Down Expand Up @@ -292,7 +293,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine {
struct ConstPropagator<'mir, 'tcx> {
ecx: InterpCx<'mir, 'tcx, ConstPropMachine>,
tcx: TyCtxt<'tcx>,
source: MirSource<'tcx>,
can_const_prop: IndexVec<Local, ConstPropMode>,
param_env: ParamEnv<'tcx>,
// FIXME(eddyb) avoid cloning these two fields more than once,
Expand Down Expand Up @@ -372,7 +372,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
ConstPropagator {
ecx,
tcx,
source,
param_env,
can_const_prop,
// FIXME(eddyb) avoid cloning these two fields more than once,
Expand Down Expand Up @@ -501,19 +500,20 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}
}

fn report_panic_as_lint(&self, source_info: SourceInfo, panic: AssertKind<u64>) -> Option<()> {
// Somewhat convoluted way to re-use the CTFE error reporting code.
fn report_assert_as_lint(
&self,
lint: &'static lint::Lint,
source_info: SourceInfo,
message: &'static str,
panic: AssertKind<u64>,
) -> Option<()> {
let lint_root = self.lint_root(source_info)?;
let error = InterpError::MachineStop(Box::new(format!("{:?}", panic)));
let mut diagnostic = error_to_const_error(&self.ecx, error.into());
diagnostic.span = source_info.span; // fix the span
diagnostic.report_as_lint(
self.tcx.at(source_info.span),
"this expression will panic at runtime",
lint_root,
None,
);
None
self.tcx.struct_span_lint_hir(lint, lint_root, source_info.span, |lint| {
let mut err = lint.build(message);
err.span_label(source_info.span, format!("{:?}", panic));
err.emit()
});
return None;
}

fn check_unary_op(
Expand All @@ -530,7 +530,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
// `AssertKind` only has an `OverflowNeg` variant, so make sure that is
// appropriate to use.
assert_eq!(op, UnOp::Neg, "Neg is the only UnOp that can overflow");
self.report_panic_as_lint(source_info, AssertKind::OverflowNeg)?;
self.report_assert_as_lint(
lint::builtin::ARITHMETIC_OVERFLOW,
source_info,
"this arithmetic operation will overflow",
AssertKind::OverflowNeg,
)?;
}

Some(())
Expand All @@ -542,27 +547,24 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
left: &Operand<'tcx>,
right: &Operand<'tcx>,
source_info: SourceInfo,
place_layout: TyLayout<'tcx>,
) -> Option<()> {
let r =
self.use_ecx(|this| this.ecx.read_immediate(this.ecx.eval_operand(right, None)?))?;
// Check for exceeding shifts *even if* we cannot evaluate the LHS.
if op == BinOp::Shr || op == BinOp::Shl {
let left_bits = place_layout.size.bits();
// 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_bits = self.ecx.layout_of(left_ty).ok()?.size.bits();
let right_size = r.layout.size;
let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size));
if r_bits.map_or(false, |b| b >= left_bits as u128) {
let lint_root = self.lint_root(source_info)?;
self.tcx.struct_span_lint_hir(
::rustc::lint::builtin::EXCEEDING_BITSHIFTS,
lint_root,
source_info.span,
|lint| {
let dir = if op == BinOp::Shr { "right" } else { "left" };
lint.build(&format!("attempt to shift {} with overflow", dir)).emit()
},
);
return None;
if r_bits.map_or(false, |b| b >= left_size_bits as u128) {
self.report_assert_as_lint(
lint::builtin::ARITHMETIC_OVERFLOW,
source_info,
"this arithmetic operation will overflow",
AssertKind::Overflow(op),
)?;
}
}

Expand All @@ -572,7 +574,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
Ok(overflow)
})? {
self.report_panic_as_lint(source_info, AssertKind::Overflow(op))?;
self.report_assert_as_lint(
lint::builtin::ARITHMETIC_OVERFLOW,
source_info,
"this arithmetic operation will overflow",
AssertKind::Overflow(op),
)?;
}

Some(())
Expand All @@ -595,8 +602,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
return None;
}

let overflow_check = self.tcx.sess.overflow_checks();

// Perform any special handling for specific Rvalue types.
// Generally, checks here fall into one of two categories:
// 1. Additional checking to provide useful lints to the user
Expand All @@ -606,20 +611,25 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
// - In this case, we'll return `None` from this function to stop evaluation.
match rvalue {
// Additional checking: give lints to the user if an overflow would occur.
// If `overflow_check` is set, running const-prop on the `Assert` terminators
// will already generate the appropriate messages.
Rvalue::UnaryOp(op, arg) if !overflow_check => {
// We do this here and not in the `Assert` terminator as that terminator is
// only sometimes emitted (overflow checks can be disabled), but we want to always
// lint.
Rvalue::UnaryOp(op, arg) => {
trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg);
self.check_unary_op(*op, arg, source_info)?;
}

// Additional checking: check for overflows on integer binary operations and report
// them to the user as lints.
// If `overflow_check` is set, running const-prop on the `Assert` terminators
// will already generate the appropriate messages.
Rvalue::BinaryOp(op, left, right) if !overflow_check => {
Rvalue::BinaryOp(op, left, right) => {
trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right);
self.check_binary_op(*op, left, right, source_info, place_layout)?;
self.check_binary_op(*op, left, right, source_info)?;
}
Rvalue::CheckedBinaryOp(op, left, right) => {
trace!(
"checking CheckedBinaryOp(op = {:?}, left = {:?}, right = {:?})",
op,
left,
right
);
self.check_binary_op(*op, left, right, source_info)?;
}

// Do not try creating references (#67862)
Expand Down Expand Up @@ -898,54 +908,39 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
}
Operand::Constant(_) => {}
}
let span = terminator.source_info.span;
let hir_id = self
.tcx
.hir()
.as_local_hir_id(self.source.def_id())
.expect("some part of a failing const eval must be local");
self.tcx.struct_span_lint_hir(
::rustc::lint::builtin::CONST_ERR,
hir_id,
span,
|lint| {
let msg = match msg {
AssertKind::Overflow(_)
| AssertKind::OverflowNeg
| AssertKind::DivisionByZero
| AssertKind::RemainderByZero => msg.description().to_owned(),
AssertKind::BoundsCheck { ref len, ref index } => {
let len = self
.eval_operand(len, source_info)
.expect("len must be const");
let len = match self.ecx.read_scalar(len) {
Ok(ScalarMaybeUndef::Scalar(Scalar::Raw {
data,
..
})) => data,
other => bug!("const len not primitive: {:?}", other),
};
let index = self
.eval_operand(index, source_info)
.expect("index must be const");
let index = match self.ecx.read_scalar(index) {
Ok(ScalarMaybeUndef::Scalar(Scalar::Raw {
data,
..
})) => data,
other => bug!("const index not primitive: {:?}", other),
};
format!(
"index out of bounds: \
the len is {} but the index is {}",
len, index,
)
}
// Need proper const propagator for these
_ => return,
};
lint.build(&msg).emit()
},
let msg = match msg {
AssertKind::DivisionByZero => AssertKind::DivisionByZero,
AssertKind::RemainderByZero => AssertKind::RemainderByZero,
AssertKind::BoundsCheck { ref len, ref index } => {
let len =
self.eval_operand(len, source_info).expect("len must be const");
let len = self
.ecx
.read_scalar(len)
.unwrap()
.to_machine_usize(&self.tcx)
.unwrap();
let index = self
.eval_operand(index, source_info)
.expect("index must be const");
let index = self
.ecx
.read_scalar(index)
.unwrap()
.to_machine_usize(&self.tcx)
.unwrap();
AssertKind::BoundsCheck { len, index }
}
// Overflow is are already covered by checks on the binary operators.
AssertKind::Overflow(_) | AssertKind::OverflowNeg => return,
// Need proper const propagator for these.
_ => return,
};
self.report_assert_as_lint(
lint::builtin::UNCONDITIONAL_PANIC,
source_info,
"this operation will panic at runtime",
msg,
);
} else {
if self.should_const_prop(value) {
Expand Down
13 changes: 10 additions & 3 deletions src/librustc_session/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,15 @@ declare_lint! {
}

declare_lint! {
pub EXCEEDING_BITSHIFTS,
pub ARITHMETIC_OVERFLOW,
Deny,
"shift exceeds the type's number of bits"
"arithmetic operation overflows"
}

declare_lint! {
pub UNCONDITIONAL_PANIC,
Deny,
"operation will cause a panic at runtime"
}

declare_lint! {
Expand Down Expand Up @@ -495,7 +501,8 @@ declare_lint_pass! {
/// that are used by other parts of the compiler.
HardwiredLints => [
ILLEGAL_FLOATING_POINT_LITERAL_PATTERN,
EXCEEDING_BITSHIFTS,
ARITHMETIC_OVERFLOW,
UNCONDITIONAL_PANIC,
UNUSED_IMPORTS,
UNUSED_EXTERN_CRATES,
UNUSED_QUALIFICATIONS,
Expand Down
2 changes: 1 addition & 1 deletion src/test/codegen/issue-56927.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub fn test1(s: &mut S) {

// CHECK-LABEL: @test2
// CHECK: store i32 4, i32* %{{.+}}, align 4
#[allow(const_err)]
#[allow(unconditional_panic)]
#[no_mangle]
pub fn test2(s: &mut S) {
s.arr[usize::MAX / 4 + 1] = 4;
Expand Down
20 changes: 0 additions & 20 deletions src/test/compile-fail/consts/const-err3.rs

This file was deleted.

4 changes: 2 additions & 2 deletions src/test/incremental/warnings-reemitted.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
// compile-flags: -Coverflow-checks=on
// build-pass (FIXME(62277): could be check-pass?)

#![warn(const_err)]
#![warn(arithmetic_overflow)]

fn main() {
let _ = 255u8 + 1; //~ WARNING attempt to add with overflow
let _ = 255u8 + 1; //~ WARNING operation will overflow
}
2 changes: 1 addition & 1 deletion src/test/run-fail/mir_indexing_oob_1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const C: [u32; 5] = [0; 5];

#[allow(const_err)]
#[allow(unconditional_panic)]
fn test() -> u32 {
C[10]
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-fail/mir_indexing_oob_2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const C: &'static [u8; 5] = b"hello";

#[allow(const_err)]
#[allow(unconditional_panic)]
fn test() -> u8 {
C[10]
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-fail/mir_indexing_oob_3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const C: &'static [u8; 5] = b"hello";

#[allow(const_err)]
#[allow(unconditional_panic)]
fn mir() -> u8 {
C[10]
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-fail/overflowing-add.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// error-pattern:thread 'main' panicked at 'attempt to add with overflow'
// compile-flags: -C debug-assertions

#![allow(const_err)]
#![allow(arithmetic_overflow)]

fn main() {
let _x = 200u8 + 200u8 + 200u8;
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-fail/overflowing-lsh-1.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// error-pattern:thread 'main' panicked at 'attempt to shift left with overflow'
// compile-flags: -C debug-assertions

#![warn(exceeding_bitshifts)]
#![warn(arithmetic_overflow)]
#![warn(const_err)]

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-fail/overflowing-lsh-2.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// error-pattern:thread 'main' panicked at 'attempt to shift left with overflow'
// compile-flags: -C debug-assertions

#![warn(exceeding_bitshifts)]
#![warn(arithmetic_overflow)]
#![warn(const_err)]

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-fail/overflowing-lsh-3.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// error-pattern:thread 'main' panicked at 'attempt to shift left with overflow'
// compile-flags: -C debug-assertions

#![warn(exceeding_bitshifts)]
#![warn(arithmetic_overflow)]
#![warn(const_err)]

fn main() {
Expand Down
Loading

0 comments on commit d237e0f

Please sign in to comment.