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

ICE with unleash-miri: mutable allocation in constant #71316

Closed
RalfJung opened this issue Apr 19, 2020 · 10 comments · Fixed by #71665
Closed

ICE with unleash-miri: mutable allocation in constant #71316

RalfJung opened this issue Apr 19, 2020 · 10 comments · Fixed by #71665
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

I tried this code with -Zunleash-the-miri-inside-of-you:

// compile-flags: -Zunleash-the-miri-inside-of-you

#![feature(const_raw_ptr_deref)]
#![feature(const_mut_refs)]
#![allow(const_err)]

use std::cell::UnsafeCell;

// make sure we do not just intern this as mutable
const MUTABLE_BEHIND_RAW: *mut i32 = &UnsafeCell::new(42) as *const _ as *mut _;

fn main() {
}

This is a variant of the mutable_const test.

I am getting an ICE:

error: internal compiler error: mutable allocation in constant
  --> ice.rs:10:1
   |
10 | const MUTABLE_BEHIND_RAW: *mut i32 = &UnsafeCell::new(42) as *const _ as *mut _;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', src/librustc_errors/lib.rs:366:17

Cc @rust-lang/wg-const-eval

@RalfJung RalfJung added the C-bug Category: This is a bug. label Apr 19, 2020
@RalfJung
Copy link
Member Author

Hm okay in another case it looks like we are actually expecting an ICE with miri-unleash? That seems rather extreme though.

@RalfJung RalfJung added the A-const-eval Area: Constant evaluation (MIR interpretation) label Apr 19, 2020
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 19, 2020
@Mark-Simulacrum Mark-Simulacrum added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Apr 19, 2020
@rust-lang-glacier-bot rust-lang-glacier-bot added the glacier ICE tracked in rust-lang/glacier. label Apr 20, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Apr 22, 2020

I thought that was the intent 😆 The miri-engine doesn't need to gracefully error in situations that can only occur in unleash mode. There's no way a user will ever see it unless we have a bug in either our knowledge of the engine or the static checks.

@RalfJung
Copy link
Member Author

That doesn't match what we do elsewhere though; CTFE doesn't ICE when it hits a box on inline assembly or a non-const fn-call.

This error is slightly different though... it is entirely unclear (to me at least) what the intended behavior is here. On the one hand the user asked for interior mutability, on the other hand it's a const. Any of the following three behaviors seems reasonable:

  • Just make this a mutable allocation and let the const point to it (as if it was an "anonymous static" or so).
  • Just make this an immutable allocation, so if the user writes to it later that's UB. This is potentially surprising though since they did use UnsafeCell.
  • Give a nice error instead of ICEing.

@RalfJung
Copy link
Member Author

Curiously, mutable_references_ice ICEs in a different way. The issues seem related though. It would be nice if they could all go through the same error path.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 22, 2020

of these three options I'd choose the nice error. I'd rather bail out than introduce mutability to constants.

@RalfJung
Copy link
Member Author

I am getting the feeling that that check is overzealous... we also get an ICE for this constant, which arguably is fine:

const MUTABLE_BEHIND_RAW: *const Vec<i32> = &Vec::<i32>::new() as *const _;

@RalfJung
Copy link
Member Author

RalfJung commented Apr 28, 2020

In other words, the current const check in the leftover_allocations phase has nothing to do with mutability. These allocations will all be mutable, because all allocations start as mutable and then get frozen when initialization is done. We are right now just rejecting all leftover (untyped) allocations in consts because we cannot sanity-check them. That's not unreasonable, but it is not what the error message says.

@RalfJung
Copy link
Member Author

In fact, this ICEs on stable:

#![allow(const_err)]

const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _;

fn main() {}

@oli-obk
Copy link
Contributor

oli-obk commented May 2, 2020

This seems like one of the cases where we'd need to overwrite the mutability of the allocation. This is one of the promotion-without-promotable cases. The MIR is

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at src/main.rs:3:36: 3:59
        StorageLive(_2);                 // bb0[1]: scope 0 at src/main.rs:3:36: 3:47
        StorageLive(_3);                 // bb0[2]: scope 0 at src/main.rs:3:37: 3:47
        _3 = const std::vec::Vec::<i32>::new() -> bb1; // bb0[3]: scope 0 at src/main.rs:3:37: 3:47
                                         // ty::Const
                                         // + ty: fn() -> std::vec::Vec<i32> {std::vec::Vec::<i32>::new}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: src/main.rs:3:37: 3:45
                                         // + user_ty: UserType(0)
                                         // + literal: Const { ty: fn() -> std::vec::Vec<i32> {std::vec::Vec::<i32>::new}, val: Value(Scalar(<ZST>)) }
    }

    bb1: {
        _2 = &_3;                        // bb1[0]: scope 0 at src/main.rs:3:36: 3:47
        _1 = &raw const (*_2);           // bb1[1]: scope 0 at src/main.rs:3:36: 3:47
        _0 = _1;                         // bb1[2]: scope 0 at src/main.rs:3:36: 3:59
        StorageDead(_2);                 // bb1[3]: scope 0 at src/main.rs:3:58: 3:59
        StorageDead(_1);                 // bb1[4]: scope 0 at src/main.rs:3:58: 3:59
        return;                          // bb1[5]: scope 0 at src/main.rs:3:1: 3:60
    }

As you can see, there's no StorageDead for _3, making it live forever. But miri needs to make it a mutable allocation in order to write the return value of the Vec::new() call into it.

@RalfJung
Copy link
Member Author

RalfJung commented May 2, 2020

Yeah and interning already makes these immutable:

alloc.mutability = Mutability::Not;

It just also ICEs currently, but that is fixed by #71665 (which makes it a hard error instead).

RalfJung added a commit to RalfJung/rust that referenced this issue May 14, 2020
Miri interning: replace ICEs by proper errors

Fixes rust-lang#71316

I also did some refactoring, as I kept being confused by all the parameters to `intern_shallow`, some of which have invalid combinations (such as a mutable const). So instead `InternMode` now contains all the information that is needed and invalid combinations are ruled out by the type system.

Also I removed interpreter errors from interning. We already ignored almost all errors, and the `ValidationFailure` errors that we handled separately actually cannot ever happen here. The only interpreter failure that was actually reachable was the UB on dangling pointers -- and arguably, a dangling raw pointer is not UB, so the error was not even correct. It's just that the rest of the compiler does not like "dangling" `AllocId`.

It should be possible to review the 3 commits separately.

r? @oli-obk
Cc @rust-lang/wg-const-eval
RalfJung added a commit to RalfJung/rust that referenced this issue May 15, 2020
Miri interning: replace ICEs by proper errors

Fixes rust-lang#71316

I also did some refactoring, as I kept being confused by all the parameters to `intern_shallow`, some of which have invalid combinations (such as a mutable const). So instead `InternMode` now contains all the information that is needed and invalid combinations are ruled out by the type system.

Also I removed interpreter errors from interning. We already ignored almost all errors, and the `ValidationFailure` errors that we handled separately actually cannot ever happen here. The only interpreter failure that was actually reachable was the UB on dangling pointers -- and arguably, a dangling raw pointer is not UB, so the error was not even correct. It's just that the rest of the compiler does not like "dangling" `AllocId`.

It should be possible to review the 3 commits separately.

r? @oli-obk
Cc @rust-lang/wg-const-eval
@bors bors closed this as completed in 584252b May 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants