fix Miri discriminant handling by RalfJung · Pull Request #63448 · rust-lang/rust

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

+ // 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

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

+ 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

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

…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. Fixesrust-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 bc8cde5Compare 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 43b52abCompare 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 8c0f601Compare 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 b21622cCompare September 16, 2019 14:28

eddyb reviewed Sep 18, 2019
View reviewed changes

oli-obk reviewed Sep 18, 2019
View reviewed changes

Comment threadsrc/librustc_mir/interpret/operand.rs Outdated

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:masterSep 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

Uh oh!

Uh oh!

Conversation

RalfJung commented Aug 10, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Aug 10, 2019

Uh oh!

RalfJung commented Aug 17, 2019

Uh oh!

JohnCSimon commented Aug 24, 2019

Uh oh!

RalfJung commented Aug 24, 2019

Uh oh!

RalfJung commented Aug 30, 2019

Uh oh!

JohnCSimon commented Sep 7, 2019

Uh oh!

RalfJung commented Sep 16, 2019

Uh oh!

RalfJung commented Sep 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Sep 16, 2019

Uh oh!

oli-obk commented Sep 16, 2019

Uh oh!

RalfJung commented Sep 16, 2019

Uh oh!

eddybSep 18, 2019

Choose a reason for hiding this comment

Uh oh!

eddybSep 18, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJungSep 18, 2019

Choose a reason for hiding this comment

Uh oh!

eddybSep 18, 2019

Choose a reason for hiding this comment

Uh oh!

RalfJungSep 18, 2019

Choose a reason for hiding this comment

Uh oh!

eddybSep 18, 2019

Choose a reason for hiding this comment

Uh oh!

eddyb commented Sep 18, 2019

Uh oh!

Uh oh!

RalfJung commented Sep 19, 2019

Uh oh!

bors commented Sep 19, 2019

Uh oh!

bors commented Sep 19, 2019

Uh oh!

bors commented Sep 19, 2019

Uh oh!

mati865 commented Sep 19, 2019

Uh oh!

RalfJung commented Sep 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RalfJung commented Aug 10, 2019•
edited
Loading

RalfJung commented Sep 16, 2019•
edited
Loading

eddybSep 18, 2019•
edited
Loading