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

Suppress ICE when validators disagree on LiveDrops in presence of &mut #65485

Merged

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Oct 17, 2019

Resolves #65394.

This hack disables the validator mismatch ICE in cases where a MutBorrow error has been emitted by both validators, but they don't agree on the number of LiveDrop errors.

The new validator is more conservative about whether a value is moved from in the presence of mutable borrows. For example, the new validator will emit a LiveDrop error on the following code.

const _: Vec<i32> = {
    let mut x = Vec::new();
    let px = &mut x as *mut _;
    let y = x;
    unsafe { ptr::write(px, Vec::new()); }
    y
};

This code is not UB AFAIK (it passes MIRI at least). The current validator does not emit a LiveDrop error for x upon exit from the initializer. x is not actually dropped, so I think this is correct? A proper fix for this would require a new MaybeInitializedLocals dataflow analysis or maybe a relaxation of the existing IndirectlyMutableLocals one.

r? @RalfJung

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 17, 2019
@ecstatic-morse ecstatic-morse changed the title Suppress ICE when validators don't agree on LiveDrops in presence of &mut Suppress ICE when validators disagree on LiveDrops in presence of &mut Oct 17, 2019
@RalfJung
Copy link
Member

RalfJung commented Oct 17, 2019

I'm afraid I am mostly unfamiliar with const qualification after all the recent changes. I didn't follow the "new validator" in any detail (oh man the name is confusing, I thought you were talking about this) -- I just don't have the time to do that next to all the UCG and Miri things, sorry.

r? @eddyb @oli-obk

@rust-highfive rust-highfive assigned eddyb and unassigned RalfJung Oct 17, 2019
@RalfJung
Copy link
Member

This code is not UB AFAIK (it passes MIRI).

That's just an artifact of what Miri happens to check right now -- this is the kind of code where really we have no idea if we have to make it UB, want to make it UB, or whatever.

@eddyb
Copy link
Member

eddyb commented Oct 18, 2019

That's... a perplexing example. I'd expect x to be dropped, naively, but there is no way to detect the indirect assignment. I prefer the conservative behavior of the new validator.

@eddyb
Copy link
Member

eddyb commented Oct 18, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Oct 18, 2019

📌 Commit af691de has been approved by eddyb

@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 Oct 18, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
…match-ugliness, r=eddyb

Suppress ICE when validators disagree on `LiveDrop`s in presence of `&mut`

Resolves rust-lang#65394.

This hack disables the validator mismatch ICE in cases where a `MutBorrow` error has been emitted by both validators, but they don't agree on the number of `LiveDrop` errors.

The new validator is more conservative about whether a value is moved from in the presence of mutable borrows. For example, the new validator will emit a `LiveDrop` error on the following code.

```rust
const _: Vec<i32> = {
    let mut x = Vec::new();
    let px = &mut x as *mut _;
    let y = x;
    unsafe { ptr::write(px, Vec::new()); }
    y
};
```

This code is not UB AFAIK (it passes MIRI at least). The current validator does not emit a `LiveDrop` error for `x` upon exit from the initializer. `x` is not actually dropped, so I think this is correct? A proper fix for this would require a new `MaybeInitializedLocals` dataflow analysis or maybe a relaxation of the existing `IndirectlyMutableLocals` one.

r? @RalfJung
Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
…match-ugliness, r=eddyb

Suppress ICE when validators disagree on `LiveDrop`s in presence of `&mut`

Resolves rust-lang#65394.

This hack disables the validator mismatch ICE in cases where a `MutBorrow` error has been emitted by both validators, but they don't agree on the number of `LiveDrop` errors.

The new validator is more conservative about whether a value is moved from in the presence of mutable borrows. For example, the new validator will emit a `LiveDrop` error on the following code.

```rust
const _: Vec<i32> = {
    let mut x = Vec::new();
    let px = &mut x as *mut _;
    let y = x;
    unsafe { ptr::write(px, Vec::new()); }
    y
};
```

This code is not UB AFAIK (it passes MIRI at least). The current validator does not emit a `LiveDrop` error for `x` upon exit from the initializer. `x` is not actually dropped, so I think this is correct? A proper fix for this would require a new `MaybeInitializedLocals` dataflow analysis or maybe a relaxation of the existing `IndirectlyMutableLocals` one.

r? @RalfJung
Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
…match-ugliness, r=eddyb

Suppress ICE when validators disagree on `LiveDrop`s in presence of `&mut`

Resolves rust-lang#65394.

This hack disables the validator mismatch ICE in cases where a `MutBorrow` error has been emitted by both validators, but they don't agree on the number of `LiveDrop` errors.

The new validator is more conservative about whether a value is moved from in the presence of mutable borrows. For example, the new validator will emit a `LiveDrop` error on the following code.

```rust
const _: Vec<i32> = {
    let mut x = Vec::new();
    let px = &mut x as *mut _;
    let y = x;
    unsafe { ptr::write(px, Vec::new()); }
    y
};
```

This code is not UB AFAIK (it passes MIRI at least). The current validator does not emit a `LiveDrop` error for `x` upon exit from the initializer. `x` is not actually dropped, so I think this is correct? A proper fix for this would require a new `MaybeInitializedLocals` dataflow analysis or maybe a relaxation of the existing `IndirectlyMutableLocals` one.

r? @RalfJung
bors added a commit that referenced this pull request Oct 19, 2019
Rollup of 6 pull requests

Successful merges:

 - #65174 (Fix zero-size uninitialized boxes)
 - #65252 (expand: Simplify expansion of derives)
 - #65485 (Suppress ICE when validators disagree on `LiveDrop`s in presence of `&mut`)
 - #65542 (Refer to "associated functions" instead of "static methods")
 - #65545 (More symbol cleanups)
 - #65576 (Don't add `argc` and `argv` arguments to `main` on WASI.)

Failed merges:

r? @ghost
@bors bors merged commit af691de into rust-lang:master Oct 19, 2019
@ecstatic-morse ecstatic-morse deleted the const-validation-mismatch-ugliness branch October 19, 2019 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nightly ICE with non-const macro in const
5 participants