Deduplicate some code between miri and const prop #64419

wesleywiser · 2019-09-13T03:08:51Z

r? @oli-obk

src/librustc_mir/interpret/eval_context.rs

src/librustc_mir/interpret/place.rs

src/librustc_mir/interpret/step.rs

src/librustc_mir/transform/const_prop.rs

oli-obk · 2019-09-14T11:24:17Z

@bors r+

bors · 2019-09-14T11:24:18Z

📌 Commit 13bb10a17c2572b0c5ecdda9aad4f792c300c28a has been approved by oli-obk

oli-obk · 2019-09-14T12:40:28Z

@bors r-

wesleywiser · 2019-09-14T16:38:26Z

Fixed by adding #![allow(const_err)] to the offending run-fail tests.

oli-obk · 2019-09-14T16:49:08Z

@bors r+

bors · 2019-09-14T16:49:09Z

📌 Commit 0b333ef has been approved by oli-obk

@oli-obk

…li-obk Deduplicate some code between miri and const prop r? @oli-obk

Centril · 2019-09-14T20:15:35Z

Failed in #64461 (comment), @bors r-

wesleywiser · 2019-09-15T18:34:09Z

I think I understand what's going on but not what to do about it:

The ICE is happening on wasm32-unkown-unknown
The ICE occurs when we we process this MIR statement: _4 = &mut (_1.1: core::option::Option<process::ChildStdin>)
process:ChildStdin is just an AnonPipe

rust/src/libstd/process.rs

Lines 230 to 232 in 60895fd

pub struct ChildStdin {

inner: AnonPipe

}
AnonPipe is an uninhabited Void type on wasm32

rust/src/libstd/sys/wasm/pipe.rs

Line 4 in 60895fd

pub struct AnonPipe(Void);
So InterpCx::mplace_field(base, field) sees this as base.layout

TyLayout { 
    ty: process:Child,
    details: LayoutDetails { 
        variants: Single { index: 0 }, 
        fields: Union(0), 
        abi: Uninhabited, 
        largest_niche: None, 
        align: AbiAndPrefAlign { abi: Align { pow2: 0 }, pref: Align { pow2: 0} }, size: Size { raw: 0 } 
}

oli-obk · 2019-09-16T12:29:05Z

Heh, that's a bug in the miri engine. The reason we never encountered it before (and never will in const eval) is that we have very few tests around the various ways to do UB in const eval.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=eaade80ea70ed29998b57c9aa11e4050 shows the ICE on stable. I'll open an issue and we'll fix it there. Until then... a possible workaround may be to remove variables of uninhabited type from the list of variables to const prop

bors · 2019-09-28T09:15:21Z

⌛ Testing commit dd486dd with merge ab3bd45112a1ac58136d4f6aa2cbcbf672450299...

@oli-obk

…li-obk Deduplicate some code between miri and const prop r? @oli-obk

Centril · 2019-09-28T09:19:14Z

@bors retry rolled up... (don't ask why...)

bors · 2019-09-28T10:53:22Z

⌛ Testing commit dd486dd with merge a6f3ffb269ab0e067c2df49e118a13fc037be236...

wesleywiser · 2019-09-28T14:49:07Z

I fixed the mir-opt test that was failing on the noopt builder and ran the rest of the test suite locally with optimize-test = false and didn't see any other errors.

oli-obk · 2019-09-28T15:00:25Z

@bors r+

bors · 2019-09-28T15:00:26Z

📌 Commit ba2d6c4 has been approved by oli-obk

bjorn3 · 2019-09-28T15:03:08Z

src/test/ui/lint/lint-exceeding-bitshifts.rs

@@ -1,3 +1,5 @@
+// compile-flags: -O


Shouldn't this warn in debug mode too?

@oli-obk Just in case this has been overlooked: we should always run the constant-folding pass and only commit the results when optimizations are enabled (although maybe we shouldn't care about optimization level at all?).

That's what we do, the differences arise because the MIR differs and thus different lints are reported

bors · 2019-09-28T15:54:19Z

⌛ Testing commit ba2d6c4 with merge 488381c...

@oli-obk

Deduplicate some code between miri and const prop r? @oli-obk

bors · 2019-09-28T19:46:21Z

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 488381c to master...

RalfJung · 2019-10-09T14:46:39Z

src/librustc/mir/interpret/error.rs

@@ -389,6 +389,10 @@ pub enum UnsupportedOpInfo<'tcx> {
    /// Free-form case. Only for errors that are never caught!
    Unsupported(String),

+    /// FIXME(#64506) Error used to work around accessing projections of
+    /// uninhabited types.
+    UninhabitedValue,


Why is "unsupported" the right category for this?

To me, it seemed similar to some of the other "unsupported" cases like ReadForeignStatic, NoMirFor, DerefFunctionPointer, OutOfTls etc in that it represents allowed behavior in a valid program that simply isn't implemented (or can't be implemented) in miri. I dismissed InvalidProgramInfo because this doesn't indicate an invalid program, however reading the doc comment, it sounds like that may actually be a more appropriate classification. Thoughts?

Looks like @oli-obk is already working on removing this again so whatever.

RalfJung · 2019-10-09T14:47:31Z

src/librustc_mir/interpret/machine.rs

+        frame.locals[local].access()
+    }
+
+    /// Called before a `StaticKind::Static` value is accessed.


Please specify what "access" means here. Is this called when the address of a static is taken? Or just on reads/writes?

I will double check and improve the comment in the other open PR I have but I believe this is just on reads/writes.

RalfJung · 2019-10-09T14:48:59Z

src/librustc_mir/interpret/step.rs

@@ -132,7 +132,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
    ///
    /// There is no separate `eval_rvalue` function. Instead, the code for handling each rvalue
    /// type writes its results directly into the memory specified by the place.
-    fn eval_rvalue_into_place(
+    pub fn eval_rvalue_into_place(


Oh, why this? We're trying to keep a clean interface for the interpreter, that'll never work if everyone just adds pub whenever they feel like it. :(

Looking at the const_prop changes, it seems like what happened is that const_prop moved up a layer or two of the interpreter abstractions. Shouldn't that mean that while some new operations need to be public now, some of the previously used one can be made private? If you could scan the functions that const_prop no longer needs and make them private, that would make me much less concerned about this change. :)

Shouldn't that mean that while some new operations need to be public now, some of the previously used one can be made private?

Yes, I think you're correct. I'll do that in the other PR as well.

This comment has been minimized.

Sign in to view

oli-obk reviewed Sep 13, 2019

View reviewed changes

src/librustc_mir/interpret/eval_context.rs Outdated Show resolved Hide resolved

src/librustc_mir/interpret/place.rs Outdated Show resolved Hide resolved

src/librustc_mir/interpret/step.rs Outdated Show resolved Hide resolved

Centril reviewed Sep 13, 2019

View reviewed changes

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 13, 2019

wesleywiser force-pushed the const_prop_use_ecx branch 2 times, most recently from cfd0efd to 13bb10a Compare September 14, 2019 11:18

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 14, 2019

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 14, 2019

wesleywiser force-pushed the const_prop_use_ecx branch from 13bb10a to 0b333ef Compare September 14, 2019 13:42

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 14, 2019

Centril added a commit to Centril/rust that referenced this pull request Sep 14, 2019

Rollup merge of rust-lang#64419 - wesleywiser:const_prop_use_ecx, r=o…

8e00329

…li-obk Deduplicate some code between miri and const prop r? @oli-obk

Centril mentioned this pull request Sep 14, 2019

Rollup of 4 pull requests #64461

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 14, 2019

Centril added a commit to Centril/rust that referenced this pull request Sep 28, 2019

Rollup merge of rust-lang#64419 - wesleywiser:const_prop_use_ecx, r=o…

dccc8b4

…li-obk Deduplicate some code between miri and const prop r? @oli-obk

Centril mentioned this pull request Sep 28, 2019

Rollup of 5 pull requests #64870

Closed

wesleywiser added 2 commits September 28, 2019 07:51

Allow reading non-mutable statics in const prop

a99f255

Fix lint-exceeding-bitshifts ui tests

ba2d6c4

wesleywiser force-pushed the const_prop_use_ecx branch from dd486dd to ba2d6c4 Compare September 28, 2019 11:52

bjorn3 reviewed Sep 28, 2019

View reviewed changes

bors added a commit that referenced this pull request Sep 28, 2019

Auto merge of #64419 - wesleywiser:const_prop_use_ecx, r=oli-obk

488381c

Deduplicate some code between miri and const prop r? @oli-obk

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 28, 2019

bors merged commit ba2d6c4 into rust-lang:master Sep 28, 2019

This was referenced Sep 28, 2019

Create compilation target versions of ::alloc::Layout #64299

Closed

Do not ICE when combining unsized locals and async #64527

Closed

Introduce librustc_index crate #64778

Merged

ecstatic-morse mentioned this pull request Oct 2, 2019

Const eval incorrectly computes that mutable param is zero #64970

Closed

wesleywiser mentioned this pull request Oct 2, 2019

Inconsistent const_err on index access to array in polymorphic functions #65005

Closed

RalfJung reviewed Oct 9, 2019

View reviewed changes

bytwise mentioned this pull request Nov 13, 2019

passing reference to field of by-value struct causes ICE with mir-opt-level=3 #66345

Closed

wesleywiser mentioned this pull request Mar 24, 2020

Why does const-prop refuse to read from statics that contain pointers? #70356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate some code between miri and const prop #64419

Deduplicate some code between miri and const prop #64419

wesleywiser commented Sep 13, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Sep 14, 2019

bors commented Sep 14, 2019

This comment has been minimized.

oli-obk commented Sep 14, 2019

wesleywiser commented Sep 14, 2019

oli-obk commented Sep 14, 2019

bors commented Sep 14, 2019

Centril commented Sep 14, 2019

wesleywiser commented Sep 15, 2019

oli-obk commented Sep 16, 2019

bors commented Sep 28, 2019

Centril commented Sep 28, 2019

bors commented Sep 28, 2019

wesleywiser commented Sep 28, 2019

oli-obk commented Sep 28, 2019

bors commented Sep 28, 2019

bjorn3 Sep 28, 2019

eddyb Sep 28, 2019

oli-obk Sep 28, 2019

bors commented Sep 28, 2019

bors commented Sep 28, 2019

RalfJung Oct 9, 2019

wesleywiser Oct 9, 2019

RalfJung Oct 9, 2019

RalfJung Oct 9, 2019

wesleywiser Oct 9, 2019

RalfJung Oct 9, 2019 •

edited

Loading

RalfJung Oct 9, 2019

wesleywiser Oct 9, 2019

Deduplicate some code between miri and const prop #64419

Deduplicate some code between miri and const prop #64419

Conversation

wesleywiser commented Sep 13, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Sep 14, 2019

bors commented Sep 14, 2019

This comment has been minimized.

oli-obk commented Sep 14, 2019

wesleywiser commented Sep 14, 2019

oli-obk commented Sep 14, 2019

bors commented Sep 14, 2019

Centril commented Sep 14, 2019

wesleywiser commented Sep 15, 2019

oli-obk commented Sep 16, 2019

bors commented Sep 28, 2019

Centril commented Sep 28, 2019

bors commented Sep 28, 2019

wesleywiser commented Sep 28, 2019

oli-obk commented Sep 28, 2019

bors commented Sep 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 28, 2019

bors commented Sep 28, 2019

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 Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Oct 9, 2019 •

edited

Loading