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.

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

Make RValue::Discriminant a normal Shallow read #56790

Merged
merged 1 commit into from
Dec 16, 2018

Conversation

matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented Dec 13, 2018

Enum layout optimizations mean that the discriminant of an enum may not be stored in a tag disjoint from the rest of the fields of the enum. Stop borrow checking as though they are.

Run with MIRI to see why this is needed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4ea0e31adbb3fe1893be528277060248.

This issue exists with the lexical borrow checker as well (see #45045) so migrate mode should prevent this from being immediately breaking.

r? @nikomatsakis

Fixes #56797

Enum layout optimizations mean that the discriminant of an enum may not
be stored in a tag disjoint from the rest of the fields of the enum.
Stop borrow checking as though they are.
@matthewjasper matthewjasper added beta-nominated Nominated for backporting to the compiler in the beta channel. NLL-sound Working towards the "invalid code does not compile" goal labels Dec 13, 2018
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2018
@nikomatsakis
Copy link
Contributor

Egads! Good catch. Fascinating.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 13, 2018

📌 Commit cdd5373 has been approved by nikomatsakis

@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 13, 2018
@nikomatsakis
Copy link
Contributor

@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 13, 2018
@nikomatsakis
Copy link
Contributor

so I think we should merge this but I'd like to hold off just a second until we get everything setup. e.g., @matthewjasper, perhaps we should file an issue?

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 14, 2018

📌 Commit cdd5373 has been approved by nikomatsakis

@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 14, 2018
Centril added a commit to Centril/rust that referenced this pull request Dec 16, 2018
…nts, r=nikomatsakis

Make RValue::Discriminant a normal Shallow read

Enum layout optimizations mean that the discriminant of an enum may not be stored in a tag disjoint from the rest of the fields of the enum. Stop borrow checking as though they are.

Run with MIRI to see why this is needed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=09a3236685a06b6096e2e2e3968b852c.

This issue exists with the lexical borrow checker as well (see rust-lang#45045) so migrate mode should prevent this from being immediately breaking.

r? @nikomatsakis

Fixes rust-lang#56797
bors added a commit that referenced this pull request Dec 16, 2018
Rollup of 20 pull requests

Successful merges:

 - #53506 (Documentation for impl From for AtomicBool and other Atomic types)
 - #56343 (Remove not used mod)
 - #56439 (Clearer error message for dead assign)
 - #56640 (Add FreeBSD unsigned char platforms to std::os::raw)
 - #56648 (Fix BTreeMap UB)
 - #56672 (Document time of back operations of a Linked List)
 - #56706 (Make `const unsafe fn` bodies `unsafe`)
 - #56742 (infer: remove Box from a returned Iterator)
 - #56761 (Suggest using `.display()` when trying to print a `Path`)
 - #56781 (Update LLVM submodule)
 - #56789 (rustc: Add an unstable `simd_select_bitmask` intrinsic)
 - #56790 (Make RValue::Discriminant a normal Shallow read)
 - #56793 (rustdoc: look for comments when scraping attributes/crates from doctests)
 - #56826 (rustc: Add the `cmpxchg16b` target feature on x86/x86_64)
 - #56832 (std: Use `rustc_demangle` from crates.io)
 - #56844 (Improve CSS rule)
 - #56850 (Fixed issue with using `Self` ctor in typedefs)
 - #56855 (Remove u8 cttz hack)
 - #56857 (Fix a small mistake regarding NaNs in a deprecation message)
 - #56858 (Fix doc of `std::fs::canonicalize`)

Failed merges:

 - #56741 (treat ref-to-raw cast like a reborrow: do a special kind of retag)

r? @ghost
@bors bors merged commit cdd5373 into master Dec 16, 2018
@Mark-Simulacrum Mark-Simulacrum deleted the borrowck-niche-discriminants branch December 16, 2018 23:30
@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 16, 2018
@pnkfelix
Copy link
Member

discussed at T-compiler meeting. Not approved for beta backport, mainly since, as @nikomatsakis put it:

  • this is a long-standing (soundness bug)
  • once this PR does land (via the train), we will start to issue warnings in 201 edition, not errors (since it will be part of NLL migration).
  • so no particular reason to backport, since we're not moving to a hard error (unless you use nightly with #![feature(nll)])

@pnkfelix pnkfelix removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NLL-sound Working towards the "invalid code does not compile" goal S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants