Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[const-prop] Fix ICE calculating enum discriminant #66960

Merged
merged 1 commit into from
Dec 3, 2019
Merged

[const-prop] Fix ICE calculating enum discriminant #66960

merged 1 commit into from
Dec 3, 2019

Conversation

wesleywiser
Copy link
Member

Fixes #66787

Different approach than #66857

r? @oli-obk
cc @RalfJung @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2019
@wesleywiser wesleywiser mentioned this pull request Dec 2, 2019
Closed
@wesleywiser
Copy link
Member Author

I've pushed changes which I believe addresses all of the feedback so far. This is ready for review.

@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2019

📌 Commit 03460b3d7ecd3ab7ff0d52bf38d6663f5b77f3a6 has been approved by oli-obk

@bors 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 Dec 3, 2019
@RalfJung
Copy link
Member

RalfJung commented Dec 3, 2019

Let's fix those typos before landing.

@bors r-

@bors 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 Dec 3, 2019
RalfJung
RalfJung reviewed Dec 3, 2019
View reviewed changes
src/librustc_mir/interpret/place.rs Outdated
Comment on lines 1042 to 1043
// Layout computation excludes uninhabited variants from consideration
// so this checks that the `variant_index` is valid.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key point is that there's no alternative: there's no way to represent those variants in the given layout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there's been a lot of muddying the waters on "variant_index validity".

It's always valid, it's always in range, it's impossible for it to not be a variant (despite what has been said wrt the comments lower down).
Also, for Multiple, the variants vec always includes all variants declared in the enum.

What needs to be handled here is "the variant is uninhabited" i.e. unrepresented.
This means two things, not just one (another source of confusion):

  • there is no tag/niche allocated for it, so the best you could do is write undef over the tag/niche
  • fully initializing (which is what SetDiscriminant means) an uninhabited variant should never be reachable, as another uninhabited Operand must've been evaluated for at least one of the fields, so this should be some sort of error (I think @RalfJung is right that this is technically an "invalid program" situation)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(of course the PR was updated while I was trying to post that comment, heh)

I think the new form of the comment seems fine, but I'd wait until @RalfJung also agrees, to proceed.

@RalfJung
Copy link
Member

RalfJung commented Dec 3, 2019

@bors r=oli-obk,RalfJung

@bors
Copy link
Contributor

bors commented Dec 3, 2019

📌 Commit 0be80f2 has been approved by oli-obk,RalfJung

@bors 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 Dec 3, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
@Centril
Rollup merge of rust-lang#66960 - wesleywiser:fix_66787_take2, r=oli-…
2c671d5 
…obk,RalfJung

[const-prop] Fix ICE calculating enum discriminant

Fixes rust-lang#66787

Different approach than rust-lang#66857

r? @oli-obk
cc @RalfJung @eddyb
@Centril Centril mentioned this pull request Dec 3, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
@Centril
Rollup merge of rust-lang#66960 - wesleywiser:fix_66787_take2, r=oli-…
ded9885 
…obk,RalfJung

[const-prop] Fix ICE calculating enum discriminant

Fixes rust-lang#66787

Different approach than rust-lang#66857

r? @oli-obk
cc @RalfJung @eddyb
@Centril Centril mentioned this pull request Dec 3, 2019
Merged
bors added a commit that referenced this pull request Dec 3, 2019
@bors
Auto merge of #66997 - Centril:rollup-uraqpgu, r=Centril
7afe6d9 
Rollup of 7 pull requests

Successful merges:

 - #66750 (Update the `wasi` crate for `wasm32-wasi`)
 - #66878 (Move Sessions into (new) librustc_session)
 - #66903 (parse_enum_item -> parse_enum_variant)
 - #66951 (miri: add throw_machine_stop macro)
 - #66957 (Change Linker for x86_64-fortanix-unknown-sgx target to rust-lld)
 - #66960 ([const-prop] Fix ICE calculating enum discriminant)
 - #66973 (Update the minimum external LLVM to 7)

Failed merges:

r? @ghost
@bors bors merged commit 0be80f2 into rust-lang:master Dec 3, 2019
@RalfJung RalfJung mentioned this pull request Dec 5, 2019
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 6, 2019
@JohnTitor
Rollup merge of rust-lang#67054 - RalfJung:set-discriminant-unreachab…
c85284e 
…le, r=oli-obk

codegen "unreachable" for invalid SetDiscriminant

Follow-up from rust-lang#66960. I also realized I don't understand our policy for using `abort` vs `unreachable`. AFAIK `abort` is safe to call and just aborts the process, while `unreachable` is UB. But sometimes we use both, like here

https://github.com/rust-lang/rust/blob/d825e35ee8325146e6c175a4c61bcb645b347d5e/src/librustc_codegen_ssa/mir/block.rs#L827-L828

and here

https://github.com/rust-lang/rust/blob/d825e35ee8325146e6c175a4c61bcb645b347d5e/src/librustc_codegen_ssa/mir/block.rs#L264-L265

The second case is even more confusing because that looks like an unreachable `return` to me, so why would we codegen a safe abort there?

r? @eddyb Cc @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@eddyb eddyb eddyb left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compiler panic while building crate with tokio, rocket, and snafu
6 participants
@wesleywiser @rust-highfive @oli-obk @bors @RalfJung @eddyb