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

Add needs_substs check to check_binary_op. #71386

Closed
wants to merge 5 commits into from
Closed

Add needs_substs check to check_binary_op. #71386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
32ff9a0
Also catch #71353
jumbatm Apr 20, 2020
c61c17c
Skip check_binary_op if rhs needs substs.
jumbatm Apr 21, 2020
cf3b744
Also add substs check before evaluating for lhs.
jumbatm Apr 21, 2020
2d25b2b
Also add substs check to check_unary_op.
jumbatm Apr 21, 2020
2e6fab9
Explain check_{unary,binary}_op needs-substs behaviour.
jumbatm Apr 21, 2020
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
102 changes: 56 additions & 46 deletions src/librustc_mir/transform/const_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,72 +521,82 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
None
}

/// Check for overflow for a unary op. Like check_binary_op, safe to call even if the operand
/// needs substs. In that case, no work is done.
fn check_unary_op(
&mut self,
op: UnOp,
arg: &Operand<'tcx>,
source_info: SourceInfo,
) -> Option<()> {
if self.use_ecx(|this| {
let val = this.ecx.read_immediate(this.ecx.eval_operand(arg, None)?)?;
let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, val)?;
Ok(overflow)
})? {
// `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_assert_as_lint(
lint::builtin::ARITHMETIC_OVERFLOW,
source_info,
"this arithmetic operation will overflow",
AssertKind::OverflowNeg,
)?;
if !arg.needs_subst() {
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid rightwards-drift, and instead do something like

if needs_subst { return Some(()); }

Copy link
Member

Choose a reason for hiding this comment

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

(Also applies to check_binary_op.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't all of these needs_subst changes become obsolete with #71386 (comment)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

I wrote these comments before you showed up in this PR.

if self.use_ecx(|this| {
let val = this.ecx.read_immediate(this.ecx.eval_operand(arg, None)?)?;
let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, val)?;
Ok(overflow)
})? {
// `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_assert_as_lint(
lint::builtin::ARITHMETIC_OVERFLOW,
source_info,
"this arithmetic operation will overflow",
AssertKind::OverflowNeg,
)?;
}
}

Some(())
}

/// Check a binary operand for overflow. Safe to call with values that need substs -- those
/// values just won't be checked.
fn check_binary_op(
&mut self,
op: BinOp,
left: &Operand<'tcx>,
right: &Operand<'tcx>,
source_info: SourceInfo,
) -> 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 {
// 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().ok();
// This is basically `force_bits`.
let r_bits = r_bits.and_then(|r| r.to_bits_or_ptr(right_size, &self.tcx).ok());
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),
)?;
if !right.needs_subst() {
Copy link
Member

Choose a reason for hiding this comment

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

This seems entirely ad-hoc, and also not necessarily complete. What about left? What about the other functions that your previous PR exposed to subst-carrying data which were previously guarded by that if you moved around?

And what is even happening that we see uninitialized data, and that no assertion anywhere fires to detect that we run the engine on code it cannot run? This is the most concerning part to me. Cc @oli-obk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about left?

Whoops, you're right. I've left left out.

What about the other functions that your previous PR exposed to subst-carrying data which were previously guarded by that if you moved around?

The only reason I felt comfortable with doing this is check_{unary, binary}_op is only called from the const_prop method, so only check_{unary, binary}_op (and their callees) are newly exposed to subst-carrying data (speaking of which, I see check_unary_op also needs to be guarded because it evals its operand).

And what is even happening that we see uninitialized data, and that no assertion anywhere fires to detect that we run the engine on code it cannot run?

Per #71353 (comment), isn't it just the error message raised during validation that's suspicious? I'm not sure that there's really uninitialised data here. I'll chuck a debugger on and see where the Err is being raised.

Copy link
Member

Choose a reason for hiding this comment

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

Per #71353 (comment), isn't it just the error message raised during validation that's suspicious? I'm not sure that there's really uninitialised data here. I'll chuck a debugger on and see where the Err is being raised.

That was my first theory. But in #71353 (comment) it doesn't look like that any more.
Also, raw ptr consts which are NULL otherwise work fine. So, ref_to_mplace probably actually accepts NULL ptrs like it should.

The only reason I felt comfortable with doing this is check_{unary, binary}op is only called from the const_prop method, so only check{unary, binary}_op (and their callees) are newly exposed to subst-carrying data (speaking of which, I see check_unary_op also needs to be guarded because it evals its operand).

If you guard left and check_unary_op, you fully un-did your previous PR and the bitshift warning will regress again. ;)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see where you are going... we can do the check for left after the bitshift check. Then the warning will still work fine and we fix the bug. Good idea.

However, we still should figure out what is happening that we see uninitialized data coming back here. That is not a good failure mode. Instead we should have gotten an ICE somewhere saying "there are still substs here, this const cannot be evaluated".

Copy link
Contributor

Choose a reason for hiding this comment

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

And what is even happening that we see uninitialized data, and that no assertion anywhere fires to detect that we run the engine on code it cannot run? This is the most concerning part to me. Cc @oli-obk

I'm entirely confused by this issue. I don't like the fix in this PR, it doesn't address the issue, it just makes sure we don't run into it.

It should be perfectly possible to evaluate std::ptr::null() for unknown (but Sized!) T. I don't see why it's failing validation of all things. I'd understand it if we'd get a TooGeneric error due to layout computation bailing out or sth. I think this is a bug in validation.

After checking what causes "uninitialized raw ptr", I believe the fault is at

rust/src/librustc_mir/interpret/place.rs

Line 307 in c6b55ee

let layout = self.layout_of(pointee_type)?;

well, not really, this is where I'd expect the TooGeneric error to come from, but try_validation! turns every error into a validation error:

rust/src/librustc_mir/interpret/validity.rs

Line 54 in 2dc5b60

Err(_) => throw_validation_failure!($what, $where, $details),

We should probably not do that for any errors of the InvalidProgramError category: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/interpret/enum.InterpError.html#variant.InvalidProgram (and we can probably ICE for a few other categories)

Copy link
Contributor

Choose a reason for hiding this comment

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

nono, that would be very wrong. We often invoke const eval on too generic code with Reveal::UserFacing, and we need to keep doing that. But for Reveal::All we can indeed report an error

Copy link
Member

@RalfJung RalfJung Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, we should just forward it for now, then.

So, action items:

  • Equip try_validation! with a pattern of errors to catch, the rest is forwarded. (Figuring out all the right patterns will take a few runs of the test suite here.)
  • Forward InvalidProgram errors out of validation instead of ICEing.

Copy link
Member

Choose a reason for hiding this comment

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

@jumbatm do you want to work on this alternative solution? (It's okay if you don't, in that case we'll look for someone else to do it. :) )

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'm happy to keep going and work on the alternative solution. It definitely seems like the more robust way to fix the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks. :)

Probably best to make that a new PR, as most comments here will not apply any more.

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 {
// 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().ok();
// This is basically `force_bits`.
let r_bits = r_bits.and_then(|r| r.to_bits_or_ptr(right_size, &self.tcx).ok());
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),
)?;
}
}
}

// The remaining operators are handled through `overflowing_binary_op`.
if self.use_ecx(|this| {
let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?;
let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
Ok(overflow)
})? {
self.report_assert_as_lint(
lint::builtin::ARITHMETIC_OVERFLOW,
source_info,
"this arithmetic operation will overflow",
AssertKind::Overflow(op),
)?;
if !left.needs_subst() {
// The remaining operators are handled through `overflowing_binary_op`.
if self.use_ecx(|this| {
let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?;
let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
Ok(overflow)
})? {
self.report_assert_as_lint(
lint::builtin::ARITHMETIC_OVERFLOW,
source_info,
"this arithmetic operation will overflow",
AssertKind::Overflow(op),
)?;
}
}
}

Some(())
Expand Down
6 changes: 2 additions & 4 deletions src/test/ui/consts/const-eval/ice-generic-assoc-const.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// check-pass
// build-pass
#![crate_type = "lib"]

pub trait Nullable {
const NULL: Self;
Expand All @@ -13,6 +14,3 @@ impl<T> Nullable for *const T {
*self == Self::NULL
}
}

fn main() {
}