Stream: t-compiler/wg-mir-opt

Topic: Support Rvalue::{Ref,Len} and Deref #61532


Wesley Wiser (Jun 06 2019 at 12:05, on Zulip):

I'm trying to track down the ICE that lead me to add this change.

The minimal repro is:

fn main() {
    let _ = main as usize;
}

which ICEs with:

thread 'rustc' panicked at 'value should already be a valid const: EvalError { kind: type validation failed: encountered a pointer, but expected initialized plain (non-pointer) bytes, backtrace: None }', src/libcore/result.rs:999:5
stack backtrace:
   0: std::sys_common::backtrace::print
   1: std::panicking::default_hook::{{closure}}
   2: std::panicking::default_hook
   3: rustc::util::common::panic_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::continue_panic_fmt
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::result::unwrap_failed
   9: <rustc_mir::transform::const_prop::ConstPropagator as rustc::mir::visit::MutVisitor>::visit_statement
  10: rustc::mir::visit::MutVisitor::visit_body
  11: <rustc_mir::transform::const_prop::ConstProp as rustc_mir::transform::MirPass>::run_pass

I noticed in the call here to validate_operand(), we pass const_mode: true but in InterpretCtx::write_immediate() we pass false.

Should we be passing true or false in const prop? (If I pass false, the ICE goes away and the program seems to compile fine.)

oli (Jun 06 2019 at 12:07, on Zulip):

I think we should just turn the expect into .ok()?

Wesley Wiser (Jun 06 2019 at 12:07, on Zulip):

That would work too

oli (Jun 06 2019 at 12:08, on Zulip):

we can't really promise that const prop won't end up in some way emitting a constant that doesn't pass the validity checks

oli (Jun 06 2019 at 12:08, on Zulip):

while we could probably allow that, let's stay conservative

Wesley Wiser (Jun 06 2019 at 12:08, on Zulip):

Are we concerned that a Const that does't validate has already been put into self.places[local]?

Wesley Wiser (Jun 06 2019 at 12:09, on Zulip):

Ie, we might try to use that value when we go to const prop again in a later statement

oli (Jun 06 2019 at 12:09, on Zulip):

yes, and I believe it's perfectly alright to have them in self.places, just not in mir::Body

Wesley Wiser (Jun 06 2019 at 12:09, on Zulip):

Ok

Wesley Wiser (Jun 06 2019 at 12:10, on Zulip):

I'l change that first commit as you suggest. I'm going to leave the test in though since it was ICE-ing before

oli (Jun 06 2019 at 12:10, on Zulip):

you could have main as usize as fn() and the first _1 = main is ok, the _2 = _1 as usize will not get changed and the _3 = _2 as fn() will get replaced by _3 = some_constant_pointer_to_main

oli (Jun 06 2019 at 12:10, on Zulip):

yes, leave the test in, regression tests are important

Wesley Wiser (Jun 06 2019 at 12:11, on Zulip):

Ok, I'll do that

Wesley Wiser (Jun 06 2019 at 12:11, on Zulip):

Do you think the performance measurement needs to be done before landing the PR?

oli (Jun 06 2019 at 12:14, on Zulip):

you mean about my measureme comment?

Wesley Wiser (Jun 06 2019 at 12:16, on Zulip):

Yeah

oli (Jun 06 2019 at 12:23, on Zulip):

hm. I don't really know if we have an official policy. You can open an issue with all info needed to repro (commit ids and link to the perf run) and assign it to me. I'll see if I can setup something to run these things conveniently (I really just want to be like measureme_helper.sh commit_id_1 commit_id_2 in some folder with a Cargo.toml and get output

Wesley Wiser (Jun 06 2019 at 12:24, on Zulip):

There's a open issue on measureme to add a built-in diff mode. Maybe I just need to go do that first :slight_smile:

oli (Jun 06 2019 at 12:27, on Zulip):

oh right, in this case, if we wait for the PR to get merged, we can download the rustcs via rustup-toolchain-master

oli (Jun 06 2019 at 12:27, on Zulip):

I was thinking we'd have to build both versions

Wesley Wiser (Jun 06 2019 at 12:28, on Zulip):

Yeah, that's true

oli (Jun 06 2019 at 12:32, on Zulip):

so I guess r=me with the cast bailout replaced by a validation bailout

Wesley Wiser (Jun 06 2019 at 13:37, on Zulip):

It looks like these changes cause some new const eval warnings to appear:

[01:00:06] ---- [ui] ui/consts/const-eval/promoted_errors.rs stdout ----
[01:00:06] diff of stderr:
[01:00:06]
[01:00:06] 16 LL |     println!("{}", 1/(1-1));
[01:00:06] 18
[01:00:06] + warning: this expression will panic at runtime
[01:00:06] +   --> $DIR/promoted_errors.rs:9:20
[01:00:06] +    |
[01:00:06] +    |
[01:00:06] + LL |     println!("{}", 1/(1-1));
[01:00:06] +
[01:00:06] 19 warning: attempt to divide by zero
[01:00:06] 20   --> $DIR/promoted_errors.rs:11:14
[01:00:06] 21    |
[01:00:06] 21    |
[01:00:06]
[01:00:06] 33    |
[01:00:06] 34 LL |     println!("{}", 1/(false as u32));
[01:00:06] +
[01:00:06] + warning: this expression will panic at runtime
[01:00:06] +   --> $DIR/promoted_errors.rs:14:20
[01:00:06] +    |
[01:00:06] +    |
[01:00:06] + LL |     println!("{}", 1/(false as u32));
[01:00:06] 36

Is that ok or is that a backwards compatibility issue?

centril (Jun 06 2019 at 14:52, on Zulip):

we can't really promise that const prop won't end up in some way emitting a constant that doesn't pass the validity checks

@oli This sounds like an optimization taking valid code and making it into invalid code...? That sounds like it violates type preservation to me ==> soundness hole.

oli (Jun 06 2019 at 14:53, on Zulip):

uhm... the validity checks are there exactly to prevent that ;)

oli (Jun 06 2019 at 14:53, on Zulip):

we don't prop if the constant would be invalid

centril (Jun 06 2019 at 14:54, on Zulip):

@oli so hmm; you take known-to-be-valid code and const prop on it and that may yield invalid code?

centril (Jun 06 2019 at 14:54, on Zulip):

that is later rejected...?

oli (Jun 06 2019 at 14:55, on Zulip):

heh well. main as usize is const-invalid

oli (Jun 06 2019 at 14:55, on Zulip):

so we don't const prop that

centril (Jun 06 2019 at 14:56, on Zulip):

Idk if that answers my question =P

oli (Jun 06 2019 at 14:57, on Zulip):

well. we prop known-to-be-valid code and durin const prop realize it's not const-valid, so we throw it away

centril (Jun 06 2019 at 15:01, on Zulip):

@oli ah, but you don't cause a compile time error?

oli (Jun 06 2019 at 15:01, on Zulip):

no

oli (Jun 06 2019 at 15:01, on Zulip):

we just silently do nothing

centril (Jun 06 2019 at 15:01, on Zulip):

:+1: so not a soundness issue

oli (Jun 06 2019 at 15:01, on Zulip):

optimizations can be opportunistic like that :D

oli (Jun 06 2019 at 15:01, on Zulip):

we may emit warnings if we encounter wrong code though

Wesley Wiser (Jun 06 2019 at 15:02, on Zulip):

Such as those :up: :up: :slight_smile:

Wesley Wiser (Jun 06 2019 at 15:04, on Zulip):

So I guess new warnings are ok then?

Wesley Wiser (Jun 06 2019 at 15:05, on Zulip):

Do we need to worry about people with #[deny(warnings)]?

oli (Jun 06 2019 at 15:06, on Zulip):

nope

oli (Jun 06 2019 at 15:06, on Zulip):

we can have as many new warnings as we want

oli (Jun 06 2019 at 15:06, on Zulip):

I even have an accepted RFC for that

oli (Jun 06 2019 at 15:06, on Zulip):

1229

Wesley Wiser (Jun 06 2019 at 15:08, on Zulip):

Ok. I'll bless the tests tonight when I get home and then r=you. I already made the cast -> validation ballout and test change we discussed.

Last update: Nov 17 2019 at 07:55UTC