fix Miri discriminant handling #63448

RalfJung · 2019-08-10T18:53:08Z

This can be reviewed commit-by-commit fairly well.

The Miri side is at rust-lang/miri#903.

Fixes #62138

r? @eddyb @oli-obk

eddyb · 2019-08-10T19:50:38Z

I'd prefer to block this on #63450.

RalfJung · 2019-08-17T11:03:39Z

I factored out the parts that are not blocked into a separate PR: #63658.

@oli-obk

Refactor Miri ops (unary, binary) to have more types This is the part of #63448 that is just a refactoring. It helps that PR by making it easier to perform machine arithmetic. r? @oli-obk @eddyb

JohnCSimon · 2019-08-24T02:16:08Z

Ping from triage:
This PR has sat idle for over a week, can you please post your status? Thanks.
@RalfJung @eddyb

RalfJung · 2019-08-24T06:57:42Z

@eddyb is blocking this on #63450. That's fine for me as long as that does not take too long.

RalfJung · 2019-08-30T06:57:58Z

@eddyb #63450 does not seem to be moving anywhere. Could you explain why it is better to leave this Miri bug unfixed?

JohnCSimon · 2019-09-07T16:35:17Z

Ping from triage
@RalfJung @eddyb Still blocked on #63450 ...

RalfJung · 2019-09-16T13:05:52Z

@eddyb noticed that doing all arithmetic using machine operations at discriminant type is also not correct, so now this does the variant idx computations on the host (checked so they do not overflow).

RalfJung · 2019-09-16T13:07:59Z

@eddyb also agreed to no longer block this on #63450

RalfJung · 2019-09-16T14:03:09Z

I should also mention, tests will be added to Miri. My naive attempt at turning that into CTFE tests failed. @oli-obk is it possible to test this with current CTFE so that we have an in-tree test?

oli-obk · 2019-09-16T14:09:51Z

is it possible to test this with current CTFE so that we have an in-tree test?

Make it a -Zunleash-the-miri-inside-of-you test 😈

RalfJung · 2019-09-16T14:25:57Z

Make it a -Zunleash-the-miri-inside-of-you test

I think that actually worked :D

eddyb · 2019-09-18T11:35:05Z

src/librustc_mir/interpret/operand.rs

+                        // Then computing the absolute variant idx should not overflow any more.
+                        let variant_index = variants_start
+                            .checked_add(variant_index_relative)
+                            .expect("oveflow computing absolute variant idx");


I wonder if this can overflow when it's the dataful variant. Still, ICE > silently doing the wrong thing.

eddyb · 2019-09-18T11:36:28Z

src/librustc_mir/interpret/operand.rs

+                            .checked_add(variant_index_relative)
+                            .expect("oveflow computing absolute variant idx");
+                        // Check if this is in the range that indicates an actual discriminant.
+                        if variants_start <= variant_index && variant_index <= variants_end {


Oh wait you can just compare variant_index_relative against variants_end - variants_start, and then you can move the addition inside the "niche" side of the if.

So the if should change to variant_index_relative <= variants_end - variants_start you say?

What is the "niche side of the if"?

eddyb · 2019-09-18T11:41:59Z

src/librustc_mir/interpret/place.rs

+                    let variants_start = niche_variants.start().as_u32();
+                    let variant_index_relative = variant_index.as_u32()
+                        .checked_sub(variants_start)
+                        .expect("overflow computing relative variant idx");


I think this is fine because this is a "niche" variant.

Sure but is there any reason not to ICE if this overflows anyway?

eddyb · 2019-09-18T11:44:43Z

src/librustc_mir/interpret/place.rs

+                    let discr_val = self.binary_op(
+                        mir::BinOp::Add,
+                        variant_index_relative_val,
+                        niche_start_val,
                    )?;


I wonder how much this matters - you just need to truncate the result, I think? Either way, the symmetry with reading is probably fine.

eddyb · 2019-09-18T11:52:15Z

r=me with #63448 (comment) fixed.

src/librustc_mir/interpret/operand.rs

…ute variant

RalfJung · 2019-09-19T07:26:18Z

r=me with #63448 (comment) fixed.

I think I did this -- and the tests are all passing.
@bors r=eddyb

bors · 2019-09-19T07:26:20Z

📌 Commit 1de533a has been approved by eddyb

bors · 2019-09-19T10:45:07Z

⌛ Testing commit 1de533a with merge b480f068c8efc171da04e5e132310df0778ff4b1...

bors · 2019-09-19T15:00:10Z

💥 Test timed out

mati865 · 2019-09-19T15:10:44Z

cc @pietroalbini ^^ many jobs were stared very late and some still are waiting for the agent

RalfJung · 2019-09-19T16:13:01Z

@bors retry

@eddyb

fix Miri discriminant handling This can be reviewed commit-by-commit fairly well. The Miri side is at rust-lang/miri#903. Fixes rust-lang#62138 r? @eddyb @oli-obk

@ghost

Rollup of 6 pull requests Successful merges: - #63448 (fix Miri discriminant handling) - #64592 (Point at original span when emitting unreachable lint) - #64601 (Fix backticks in documentation) - #64606 (Remove unnecessary `mut` in doc example) - #64611 (rustbuild: Don't package libstd twice) - #64613 (rustbuild: Copy crate doc files fewer times) Failed merges: r? @ghost

@ghost

Rollup of 6 pull requests Successful merges: - #63448 (fix Miri discriminant handling) - #64592 (Point at original span when emitting unreachable lint) - #64601 (Fix backticks in documentation) - #64606 (Remove unnecessary `mut` in doc example) - #64611 (rustbuild: Don't package libstd twice) - #64613 (rustbuild: Copy crate doc files fewer times) Failed merges: r? @ghost

fix discriminant handling The Miri side of rust-lang/rust#63448

rust-highfive assigned eddyb Aug 10, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2019

RalfJung mentioned this pull request Aug 10, 2019

fix discriminant handling rust-lang/miri#903

Merged

RalfJung force-pushed the miri-discriminant branch 2 times, most recently from 90d692b to bc8cde5 Compare August 17, 2019 10:58

RalfJung mentioned this pull request Aug 17, 2019

Refactor Miri ops (unary, binary) to have more types #63658

Merged

RalfJung force-pushed the miri-discriminant branch from bc8cde5 to 43b52ab Compare August 18, 2019 09:06

JohnCSimon added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 7, 2019

fix Miri discriminant load/store when overflows are involved

8c0f601

RalfJung force-pushed the miri-discriminant branch from 43b52ab to 8c0f601 Compare September 16, 2019 10:48

RalfJung added 3 commits September 16, 2019 13:08

better and more consistent variable names

b73f1a5

factor getting the discriminant layout to a new method

75fe84f

do the variant idx computations on the host (non-overflowing)

61a28d5

RalfJung removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 16, 2019

add test

b21622c

RalfJung force-pushed the miri-discriminant branch from 5530722 to b21622c Compare September 16, 2019 14:28

eddyb reviewed Sep 18, 2019

View reviewed changes

oli-obk reviewed Sep 18, 2019

View reviewed changes

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

first determine if the variant is a niche-variant, then compute absol…

1de533a

…ute variant

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2019

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 19, 2019

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2019

Centril mentioned this pull request Sep 19, 2019

Rollup of 6 pull requests #64616

Merged

bors merged commit 1de533a into rust-lang:master Sep 20, 2019

bors added a commit to rust-lang/miri that referenced this pull request Sep 20, 2019

Auto merge of #903 - RalfJung:discriminant, r=RalfJung

0c77a10

fix discriminant handling The Miri side of rust-lang/rust#63448

RalfJung deleted the miri-discriminant branch September 28, 2019 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Miri discriminant handling #63448

fix Miri discriminant handling #63448

RalfJung commented Aug 10, 2019 •

edited

Loading

eddyb commented Aug 10, 2019

RalfJung commented Aug 17, 2019

JohnCSimon commented Aug 24, 2019

RalfJung commented Aug 24, 2019

RalfJung commented Aug 30, 2019

JohnCSimon commented Sep 7, 2019

RalfJung commented Sep 16, 2019

RalfJung commented Sep 16, 2019 •

edited

Loading

RalfJung commented Sep 16, 2019

oli-obk commented Sep 16, 2019

RalfJung commented Sep 16, 2019

eddyb Sep 18, 2019

eddyb Sep 18, 2019 •

edited

Loading

RalfJung Sep 18, 2019

eddyb Sep 18, 2019

RalfJung Sep 18, 2019

eddyb Sep 18, 2019

eddyb commented Sep 18, 2019

RalfJung commented Sep 19, 2019

bors commented Sep 19, 2019

bors commented Sep 19, 2019

bors commented Sep 19, 2019

mati865 commented Sep 19, 2019

RalfJung commented Sep 19, 2019

fix Miri discriminant handling #63448

fix Miri discriminant handling #63448

Conversation

RalfJung commented Aug 10, 2019 • edited Loading

eddyb commented Aug 10, 2019

RalfJung commented Aug 17, 2019

JohnCSimon commented Aug 24, 2019

RalfJung commented Aug 24, 2019

RalfJung commented Aug 30, 2019

JohnCSimon commented Sep 7, 2019

RalfJung commented Sep 16, 2019

RalfJung commented Sep 16, 2019 • edited Loading

RalfJung commented Sep 16, 2019

oli-obk commented Sep 16, 2019

RalfJung commented Sep 16, 2019

eddyb Sep 18, 2019

Choose a reason for hiding this comment

eddyb Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

RalfJung Sep 18, 2019

Choose a reason for hiding this comment

eddyb Sep 18, 2019

Choose a reason for hiding this comment

RalfJung Sep 18, 2019

Choose a reason for hiding this comment

eddyb Sep 18, 2019

Choose a reason for hiding this comment

eddyb commented Sep 18, 2019

RalfJung commented Sep 19, 2019

bors commented Sep 19, 2019

bors commented Sep 19, 2019

bors commented Sep 19, 2019

mati865 commented Sep 19, 2019

RalfJung commented Sep 19, 2019

RalfJung commented Aug 10, 2019 •

edited

Loading

RalfJung commented Sep 16, 2019 •

edited

Loading

eddyb Sep 18, 2019 •

edited

Loading