Add needs_substs check to `check_binary_op`. #71386

jumbatm · 2020-04-21T14:18:00Z

Fixes a regression introduced in #70566 by adding a needs_substs check to check_binary_op for the rhs. We need this check now because const_prop no longer bail outs of generic contexts before check_binary_op is reached.

Also amends the Nullable test case to catch this issue in the future.

r? @RalfJung

RalfJung · 2020-04-21T17:28:11Z

src/librustc_mir/transform/const_prop.rs

-            // 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) {
+        if !right.needs_subst() {


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

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.

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. ;)

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".

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)

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

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.

@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. :) )

I'm happy to keep going and work on the alternative solution. It definitely seems like the more robust way to fix the issue.

Awesome, thanks. :)

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

RalfJung · 2020-04-22T09:24:47Z

src/librustc_mir/transform/const_prop.rs

-                "this arithmetic operation will overflow",
-                AssertKind::OverflowNeg,
-            )?;
+        if !arg.needs_subst() {


Please avoid rightwards-drift, and instead do something like

if needs_subst { return Some(()); }

(Also applies to check_binary_op.)

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

Yes.

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

bors · 2020-04-25T14:17:20Z

☔ The latest upstream changes (presumably #71549) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2020-04-26T09:56:25Z

I'm closing this PR as we're going with a completely different approach as per #71386 (comment)

jumbatm added 2 commits April 21, 2020 21:40

Also catch rust-lang#71353

32ff9a0

Skip check_binary_op if rhs needs substs.

c61c17c

rust-highfive assigned RalfJung Apr 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2020

RalfJung reviewed Apr 21, 2020

View reviewed changes

jumbatm added 3 commits April 22, 2020 19:08

Also add substs check before evaluating for lhs.

cf3b744

Also add substs check to check_unary_op.

2d25b2b

Explain check_{unary,binary}_op needs-substs behaviour.

2e6fab9

RalfJung reviewed Apr 22, 2020

View reviewed changes

RalfJung mentioned this pull request Apr 22, 2020

Type validation mistreats layout errors #71353

Closed

oli-obk closed this Apr 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add needs_substs check to `check_binary_op`. #71386

Add needs_substs check to `check_binary_op`. #71386

jumbatm commented Apr 21, 2020

RalfJung Apr 21, 2020

jumbatm Apr 21, 2020

RalfJung Apr 21, 2020

RalfJung Apr 22, 2020

oli-obk Apr 22, 2020

oli-obk Apr 22, 2020

RalfJung Apr 22, 2020 •

edited

Loading

RalfJung Apr 22, 2020

jumbatm Apr 22, 2020

RalfJung Apr 23, 2020

RalfJung Apr 22, 2020

RalfJung Apr 22, 2020

oli-obk Apr 22, 2020

RalfJung Apr 22, 2020

bors commented Apr 25, 2020

oli-obk commented Apr 26, 2020

Add needs_substs check to check_binary_op. #71386

Add needs_substs check to check_binary_op. #71386

Conversation

jumbatm commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 25, 2020

oli-obk commented Apr 26, 2020

Add needs_substs check to `check_binary_op`. #71386

Add needs_substs check to `check_binary_op`. #71386

RalfJung Apr 22, 2020 •

edited

Loading