Repository navigation
Boxes with custom allocators break several underlying assumptions in Miri (and MIR more broadly) #95453
Copy link
Copy link
Open
Labels
A-MIRArea: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.htmlArea: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.htmlA-allocatorsArea: Custom and system allocatorsArea: Custom and system allocatorsA-boxArea: Our favorite opsem complicationArea: Our favorite opsem complicationC-bugCategory: This is a bug.Category: This is a bug.I-ICEIssue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.
Description
This error got turned into a general design issue for Box as a language primitive vs Box having an 'allocator' field. See original bug report below. Also see a further testcase at rust-lang/miri#2072.
This is minimized from an error I ran into while implementing the Allocator API in my crate, bump-into, and trying to test the implementation with Miri:
Code
#![feature(allocator_api)]
extern crate alloc;
use alloc::alloc::{AllocError, Allocator};
use core::alloc::Layout;
use core::cell::Cell;
use core::mem::MaybeUninit;
use core::ptr::{self, NonNull};
struct OnceAlloc<'a> {
space: Cell<&'a mut [MaybeUninit<u8>]>,
}
unsafe impl<'shared, 'a: 'shared> Allocator for &'shared OnceAlloc<'a> {
fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
let space = self.space.replace(&mut []);
let (ptr, len) = (space.as_mut_ptr(), space.len());
if ptr.align_offset(layout.align()) != 0 || len < layout.size() {
return Err(AllocError);
}
let slice_ptr = ptr::slice_from_raw_parts_mut(ptr as *mut u8, len);
unsafe { Ok(NonNull::new_unchecked(slice_ptr)) }
}
unsafe fn deallocate(&self, _ptr: NonNull<u8>, _layout: Layout) {}
}
fn main() {
let mut space = vec![MaybeUninit::new(0); 1];
let once_alloc = OnceAlloc {
space: Cell::new(&mut space[..]),
};
let _boxed = Box::new_in([0u8; 1], &once_alloc);
}Meta
rustc --version --verbose:
rustc 1.61.0-nightly (1d9c262ee 2022-03-26)
binary: rustc
commit-hash: 1d9c262eea411ec5230f8a4c9ba50b3647064da4
commit-date: 2022-03-26
host: x86_64-unknown-linux-gnu
release: 1.61.0-nightly
LLVM version: 14.0.0
Error output
$ env RUST_BACKTRACE=full cargo miri run
Finished dev [unoptimized + debuginfo] target(s) in 0.00s
Running `/home/autumn/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri target/miri/x86_64-unknown-linux-gnu/debug/miri-ice`
error: internal compiler error: /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/compiler/rustc_const_eval/src/interpret/place.rs:777:26: write_immediate_to_mplace: invalid ScalarPair layout: TyAndLayout {
ty: *mut [u8; 1],
layout: Layout {
fields: Primitive,
variants: Single {
index: 0,
},
abi: Scalar(
Scalar {
value: Pointer,
valid_range: 0..=18446744073709551615,
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align {
pow2: 3,
},
pref: Align {
pow2: 3,
},
},
size: Size {
raw: 8,
},
},
}
--> /home/autumn/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:359:13
|
359 | boxed.as_mut_ptr().write(x);
| ^^^^^^^^^^^^^^^^^^
Backtrace
thread 'rustc' panicked at 'Box<dyn Any>', /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/compiler/rustc_errors/src/lib.rs:1222:9
stack backtrace:
0: 0x7f4c0669d55d - std::backtrace_rs::backtrace::libunwind::trace::hb8664f5c920dcd17
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
1: 0x7f4c0669d55d - std::backtrace_rs::backtrace::trace_unsynchronized::h4718639a025d9e7f
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
2: 0x7f4c0669d55d - std::sys_common::backtrace::_print_fmt::h147f7c369d4f1e8e
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/sys_common/backtrace.rs:66:5
3: 0x7f4c0669d55d - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hee801905e555ec30
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/sys_common/backtrace.rs:45:22
4: 0x7f4c066f755c - core::fmt::write::h4bb35aa347c570f4
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/core/src/fmt/mod.rs:1190:17
5: 0x7f4c0668eab1 - std::io::Write::write_fmt::h6ed268cd0958975b
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/io/mod.rs:1655:15
6: 0x7f4c066a0645 - std::sys_common::backtrace::_print::h3f974cf4bd76b112
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/sys_common/backtrace.rs:48:5
7: 0x7f4c066a0645 - std::sys_common::backtrace::print::h56cc71e242d02ce1
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/sys_common/backtrace.rs:35:9
8: 0x7f4c066a0645 - std::panicking::default_hook::{{closure}}::hca67c27c011873e3
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/panicking.rs:295:22
9: 0x7f4c066a02f9 - std::panicking::default_hook::ha53803c35aaf5526
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/panicking.rs:314:9
10: 0x7f4c06ece6c1 - rustc_driver[1cb8e757554a7e43]::DEFAULT_HOOK::{closure#0}::{closure#0}
11: 0x7f4c066a0d90 - std::panicking::rust_panic_with_hook::hc072423965447af2
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/panicking.rs:702:17
12: 0x5584173e5793 - std[907b070526960113]::panicking::begin_panic::<rustc_errors[f8e1677070e2f6d9]::ExplicitBug>::{closure#0}
13: 0x5584173e5356 - std[907b070526960113]::sys_common::backtrace::__rust_end_short_backtrace::<std[907b070526960113]::panicking::begin_panic<rustc_errors[f8e1677070e2f6d9]::ExplicitBug>::{closure#0}, !>
14: 0x55841725f996 - std[907b070526960113]::panicking::begin_panic::<rustc_errors[f8e1677070e2f6d9]::ExplicitBug>
15: 0x558417309876 - std[907b070526960113]::panic::panic_any::<rustc_errors[f8e1677070e2f6d9]::ExplicitBug>
16: 0x558417307f2a - <rustc_errors[f8e1677070e2f6d9]::HandlerInner>::span_bug::<rustc_span[c0544ac70c34a1b7]::span_encoding::Span>
17: 0x558417307d70 - <rustc_errors[f8e1677070e2f6d9]::Handler>::span_bug::<rustc_span[c0544ac70c34a1b7]::span_encoding::Span>
18: 0x558417312215 - rustc_middle[4fcaad924d73098a]::ty::context::tls::with_opt::<rustc_middle[4fcaad924d73098a]::util::bug::opt_span_bug_fmt<rustc_span[c0544ac70c34a1b7]::span_encoding::Span>::{closure#0}, ()>
19: 0x558417311f69 - rustc_middle[4fcaad924d73098a]::util::bug::opt_span_bug_fmt::<rustc_span[c0544ac70c34a1b7]::span_encoding::Span>
20: 0x55841725fa63 - rustc_middle[4fcaad924d73098a]::util::bug::span_bug_fmt::<rustc_span[c0544ac70c34a1b7]::span_encoding::Span>
21: 0x558417350c5d - <rustc_const_eval[3ebf8ee4e08cef50]::interpret::eval_context::InterpCx<miri[17c863d3c0e6d9d6]::machine::Evaluator>>::write_immediate_to_mplace_no_validate
22: 0x55841734ff04 - <rustc_const_eval[3ebf8ee4e08cef50]::interpret::eval_context::InterpCx<miri[17c863d3c0e6d9d6]::machine::Evaluator>>::write_immediate_no_validate
23: 0x55841734f9c4 - <rustc_const_eval[3ebf8ee4e08cef50]::interpret::eval_context::InterpCx<miri[17c863d3c0e6d9d6]::machine::Evaluator>>::copy_op_no_validate
24: 0x55841734d4ba - <rustc_const_eval[3ebf8ee4e08cef50]::interpret::eval_context::InterpCx<miri[17c863d3c0e6d9d6]::machine::Evaluator>>::copy_op_transmute
25: 0x558417355324 - <rustc_const_eval[3ebf8ee4e08cef50]::interpret::eval_context::InterpCx<miri[17c863d3c0e6d9d6]::machine::Evaluator>>::pop_stack_frame
26: 0x558417342ddf - <rustc_const_eval[3ebf8ee4e08cef50]::interpret::eval_context::InterpCx<miri[17c863d3c0e6d9d6]::machine::Evaluator>>::terminator
27: 0x5584172f42f0 - miri[17c863d3c0e6d9d6]::eval::eval_entry
28: 0x558417265aec - <rustc_interface[38cec0579f964c4b]::passes::QueryContext>::enter::<<miri[f35c181d28723cbf]::MiriCompilerCalls as rustc_driver[1cb8e757554a7e43]::Callbacks>::after_analysis::{closure#0}, ()>
29: 0x5584172637e5 - <miri[f35c181d28723cbf]::MiriCompilerCalls as rustc_driver[1cb8e757554a7e43]::Callbacks>::after_analysis
30: 0x7f4c08d3e4b5 - <rustc_interface[38cec0579f964c4b]::interface::Compiler>::enter::<rustc_driver[1cb8e757554a7e43]::run_compiler::{closure#1}::{closure#2}, core[69ec914610f0cf77]::result::Result<core[69ec914610f0cf77]::option::Option<rustc_interface[38cec0579f964c4b]::queries::Linker>, rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>>
31: 0x7f4c08d514bf - rustc_span[c0544ac70c34a1b7]::with_source_map::<core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>, rustc_interface[38cec0579f964c4b]::interface::create_compiler_and_run<core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>, rustc_driver[1cb8e757554a7e43]::run_compiler::{closure#1}>::{closure#1}>
32: 0x7f4c08d3f114 - rustc_interface[38cec0579f964c4b]::interface::create_compiler_and_run::<core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>, rustc_driver[1cb8e757554a7e43]::run_compiler::{closure#1}>
33: 0x7f4c08d3be92 - <scoped_tls[9467090f342ff13]::ScopedKey<rustc_span[c0544ac70c34a1b7]::SessionGlobals>>::set::<rustc_interface[38cec0579f964c4b]::interface::run_compiler<core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>, rustc_driver[1cb8e757554a7e43]::run_compiler::{closure#1}>::{closure#0}, core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>>
34: 0x7f4c08d3a1ff - std[907b070526960113]::sys_common::backtrace::__rust_begin_short_backtrace::<rustc_interface[38cec0579f964c4b]::util::run_in_thread_pool_with_globals<rustc_interface[38cec0579f964c4b]::interface::run_compiler<core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>, rustc_driver[1cb8e757554a7e43]::run_compiler::{closure#1}>::{closure#0}, core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>>::{closure#0}, core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>>
35: 0x7f4c08d52452 - <<std[907b070526960113]::thread::Builder>::spawn_unchecked_<rustc_interface[38cec0579f964c4b]::util::run_in_thread_pool_with_globals<rustc_interface[38cec0579f964c4b]::interface::run_compiler<core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>, rustc_driver[1cb8e757554a7e43]::run_compiler::{closure#1}>::{closure#0}, core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>>::{closure#0}, core[69ec914610f0cf77]::result::Result<(), rustc_errors[f8e1677070e2f6d9]::ErrorGuaranteed>>::{closure#1} as core[69ec914610f0cf77]::ops::function::FnOnce<()>>::call_once::{shim:vtable#0}
36: 0x7f4c066aaf83 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::hf0cca23978c89840
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/alloc/src/boxed.rs:1861:9
37: 0x7f4c066aaf83 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::h9da765b1ac504456
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/alloc/src/boxed.rs:1861:9
38: 0x7f4c066aaf83 - std::sys::unix::thread::Thread::new::thread_start::hb3629e14c6b629e3
at /rustc/1d9c262eea411ec5230f8a4c9ba50b3647064da4/library/std/src/sys/unix/thread.rs:108:17
39: 0x7f4c064835c2 - start_thread
40: 0x7f4c06508584 - __clone
41: 0x0 - <unknown>
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
A-MIRArea: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.htmlArea: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.htmlA-allocatorsArea: Custom and system allocatorsArea: Custom and system allocatorsA-boxArea: Our favorite opsem complicationArea: Our favorite opsem complicationC-bugCategory: This is a bug.Category: This is a bug.I-ICEIssue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.
Activity
RalfJung commentedon Apr 18, 2022
Thanks for the report!
Something went very wrong a bit before this error occurred... a ScalarPair value should never have Scalar layout.
matthiaskrgr commentedon Apr 18, 2022
smaller repro from glacier:
autumnontape commentedon Apr 18, 2022
@matthiaskrgr Indeed, I get a similar error when I run that code under Miri:
Details
This is with the latest nightly:
But that ICE is marked as fixed. Should this be closed as a duplicate and #78459 reopened?
RalfJung commentedon Apr 18, 2022
#78459 was fixed by doing something in the codegen backend, this here is about the interpreter. It think it is a separate, albeit related, problem.
autumnontape commentedon Apr 18, 2022
I see, that explains why it wasn't caught as a regression.
RalfJung commentedon Apr 19, 2022
I suspect this has something to do with dereferencing a
Box. It looks like evenBox<_, Alloc>are considered primitive pointers that can be dereferenced, which somehow has to implicitly throw away the allocation part? There is no indication in the MIR that anything like that is happening, so each backend has to do it separately.Honestly that sounds like a pretty bad way to encode this all. The way allocators were added to
Boxbroke a fundamental assumption that Miri (and probably plenty of other MIR consumers) made: if builtin_deref succeeds on a value of a given type (aka if we can have*ptrin MIR on a value of that type), then that value is a pointer (and just a pointer, not a pointer-allocator pair). Now instead all MIR consumers have to be wary each time they use a "pointer" (i.e., something where*ptrworks) because that "pointer" might be arbitrarily complex data where we first have to dig for the actual pointer part in it. We can keep putting ad-hoc fixes for this everywhere but I can almost guarantee this is going to create a constant stream of issues.Cc @rust-lang/wg-mir-opt (there is no WG-MIR I think?) @rust-lang/wg-allocators
RalfJung commentedon Apr 19, 2022
Here's a way to cause even more fun ICEs in Miri, by making
Box<T, G>so big that it cannot be represented as an 'immediate' any more. Interestingly this works in codegen, I guess it must be littered with special cases that treatBoxpointers different than any other pointer -- not sure if there is another way to do that.And indeed, we have
rust/compiler/rustc_codegen_ssa/src/mir/operand.rs
Line 131 in d32ce37
rust/compiler/rustc_codegen_ssa/src/mir/place.rs
Line 450 in c58af72
rust/compiler/rustc_codegen_ssa/src/mir/place.rs
Line 468 in c58af72
rust/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs
Line 454 in e169261
rust/compiler/rustc_codegen_llvm/src/type_of.rs
Line 343 in d32ce37
[-]"write_immediate_to_mplace: invalid ScalarPair layout: ..." in Miri[/-][+]Boxes with custom allocators break several underlying assumptions in Miri[/+]CAD97 commentedon Apr 19, 2022
"
Boxis a special type kind" strikes again...The proper fix-for-all-time is to stop making
Boxspecial, and for it to just be a regular aggregate andDerefMoveor whatever you want to call it specify the special behavior where the deref place is managed kinda like a local.How practical that is in the short/medium term, though, is a different question. However, it's probably required to let
Boxuse the theoretical storage API (as inline storage isn't even a pointer anymore), and might be simpler than compounding special cases even more...RalfJung commentedon Apr 19, 2022
I got things to work for the example, and for a nasty variant of the example that makes
Boxnot even aScalarPairtype any more:As you can see, various random places throughout the interpreter need to be aware of this -- the hallmark of non-compositional design. Stacked Borrows will ignore such Boxes (so if they are
noalias, this will miss UB) and validity checking will not complain when they fail to be dereferencable. BasicallyBoxwith non-ZST allocator is treated like a regular user type that somehow magically can have the*operator applied to it, but has none of the otherBoxmagic. (I don't know if we generate LLVM attributes likenoaliasfor such boxes; if we do, that'd be even worse since Miri would not be able to check that.)Also, this does not entirely fix the problem; this example still ICEs because the casting and unsizing code assumes that it will only be called with regular pointer types. I've run out of steam for fixing that; it would probably require special treatment around here. There are probably more places I missed, it is very hard to be sure since this is breaking some very fundamental assumptions.
RalfJung commentedon Apr 19, 2022
Yeah, I don't have a good alternative idea either. Borrow checking works on MIR and needs to treat these things as primitive pointers. We could have a MIR transformation run after borrowck to remove
*ptrfor non-primitiveBoxes and replace it by raw ptr accesses, but that would still only cover part of the problem.Having primitive types that are also generic and contain arbitrary user-defined data, however, is clearly also bad. Having
Boxas a special type is not so bad when it behaves operationally exactly like other special types (e.g.&mut T). But havingBoxbe a special type that's completely different than any other special pointer type is annoying, and doing this in an existing codebase where everything assumesBoxis like the other pointers is a recipe for disaster.11 remaining items
RalfJung commentedon Jun 25, 2022
That there are issues remaining is also shown by the fact that #98510 fails -- we still are deref'ing
Boxes in some places in the interpreter, even if it isn't explicit in the MIR sources.CAD97 commentedon Jun 26, 2022
I aggressively agree with the point here, but there's something important left unspecified here:
Is it the compiler-understood primitive
std::boxed::Box, or a separate type? (std::ptr::Unique?&own/&move?)Obviously
Boxis currently primitive/special in many ways, and removing those will take a significant amount of work, much of which is difficult to make incremental. But I think "making box unspecial" will lead to a significantly better language.RalfJung commentedon Jun 26, 2022
It cannot be
std::boxed::Boxbecause we want that type to support custom allocators.Maybe it should be
Unique, though that then touches on rust-lang/unsafe-code-guidelines#326 -- we cannot use the same type inVecunless we want to reduceBoxes guarantees (e.g. of being dereferenceable).So maybe it is a new type "between" Box and Unique? I am not sure.
beepster4096 commentedon Jun 26, 2022
While this is a great idea and would fix the unsizing issue, it wouldn't solve the failures in #98510. The ICE there comes from validity checking code which needs to be special cased for box anyway to give good diagnostics.
beepster4096 commentedon Jun 26, 2022
Actually I think we can easily solve the unsizing issue by implementing
CoerceUnsizedforUniqueand ripping out all of the code special casingBoxhere.RalfJung commentedon Jun 26, 2022
I think validity checking only needs to special-case the "inner"
Boxtype that does not have an allocator parameter.RalfJung commentedon Jul 4, 2022
With rust-lang/miri#2323, the relevant testcase should pass in Miri.
However, this still requires some gross hacks so we should keep an issue open to track finding a proper solution for this.
RalfJung commentedon Aug 31, 2024
Turns out yes we (used to) add
noaliasand this even caused a soundness bug -- see #122018.Rollup merge of rust-lang#129812 - RalfJung:box-custom-alloc, r=compi…
Unrolled build for rust-lang#129812
Rollup merge of #129812 - RalfJung:box-custom-alloc, r=compiler-errors
Rollup merge of #129812 - RalfJung:box-custom-alloc, r=compiler-errors
safinaskar commentedon Dec 26, 2024
@RalfJung . Some notes.
Boxfrom compiler, thenBoxwill stop being ABI-compatible with pointer, because it is not marked as#[repr(transparent)]! I. e. we will not be able to passBoxto functions, which inspect pointer! As well as I understand, currently code in the wild assume thatBoxis ABI-compatible with pointer. And we cannot fix it, because this requires#[repr(transparent)], and we cannot put it because of allocator field. So, if we really want to stopBoxbeing magic, we should do one of two things:repr(transparent)depending on generic paramsptr::write) allocator to allocation. And similar thing when droppingBoxRcas well, because it will be nice idea to makeRcABI-compatible with pointer, too (see MakeRc<T>::derefandArc<T>::derefzero-cost #132553 )let x = Box::new(0i32); *x = *x. If we replace*x = *xwith*x = Box::into_inner(x), it will not compile any more. I think there is a lot of code like this in the wild. So if we ever want to makeBoxnot-a-magic anymore, then we should account for this case in all futureDerefMoveproposalsbjorn3 commentedon Dec 26, 2024
We won't remove all special casing for Box anytime soon. That doesn't mean we can't remove some of the more surprising special cases like the validity invariant not allowing aliasing.