Skip to content

Commit cebdcc7

Browse files
authored
Unrolled build for rust-lang#122537
Rollup merge of rust-lang#122537 - RalfJung:interpret-allocation, r=oli-obk interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit That new raw getter will be needed to let Miri pass pointers to natively executed FFI code ("extern-so" mode). While doing that I realized our get_bytes_mut are named less scary than get_bytes_unchecked so I rectified that. Also I realized `mem_copy_repeatedly` would break if we called it for multiple overlapping copies so I made sure this does not happen. And I realized that we are actually [violating Stacked Borrows in the interpreter](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/I.20think.20Miri.20violates.20Stacked.20Borrows.20.F0.9F.99.88).^^ That was introduced in rust-lang#87777. r? ```@oli-obk```
2 parents eff958c + 7be47b2 commit cebdcc7

File tree

2 files changed

+45
-14
lines changed

2 files changed

+45
-14
lines changed

compiler/rustc_const_eval/src/interpret/memory.rs

+14-3
Original file line numberDiff line numberDiff line change
@@ -1159,11 +1159,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
11591159
};
11601160

11611161
// Side-step AllocRef and directly access the underlying bytes more efficiently.
1162-
// (We are staying inside the bounds here so all is good.)
1162+
// (We are staying inside the bounds here and all bytes do get overwritten so all is good.)
11631163
let alloc_id = alloc_ref.alloc_id;
11641164
let bytes = alloc_ref
11651165
.alloc
1166-
.get_bytes_mut(&alloc_ref.tcx, alloc_ref.range)
1166+
.get_bytes_unchecked_for_overwrite(&alloc_ref.tcx, alloc_ref.range)
11671167
.map_err(move |e| e.to_interp_error(alloc_id))?;
11681168
// `zip` would stop when the first iterator ends; we want to definitely
11691169
// cover all of `bytes`.
@@ -1184,6 +1184,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
11841184
self.mem_copy_repeatedly(src, dest, size, 1, nonoverlapping)
11851185
}
11861186

1187+
/// Performs `num_copies` many copies of `size` many bytes from `src` to `dest + i*size` (where
1188+
/// `i` is the index of the copy).
1189+
///
1190+
/// Either `nonoverlapping` must be true or `num_copies` must be 1; doing repeated copies that
1191+
/// may overlap is not supported.
11871192
pub fn mem_copy_repeatedly(
11881193
&mut self,
11891194
src: Pointer<Option<M::Provenance>>,
@@ -1245,8 +1250,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
12451250
(dest_alloc_id, dest_prov),
12461251
dest_range,
12471252
)?;
1253+
// Yes we do overwrite all bytes in `dest_bytes`.
12481254
let dest_bytes = dest_alloc
1249-
.get_bytes_mut_ptr(&tcx, dest_range)
1255+
.get_bytes_unchecked_for_overwrite_ptr(&tcx, dest_range)
12501256
.map_err(|e| e.to_interp_error(dest_alloc_id))?
12511257
.as_mut_ptr();
12521258

@@ -1280,6 +1286,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
12801286
}
12811287
}
12821288
}
1289+
if num_copies > 1 {
1290+
assert!(nonoverlapping, "multi-copy only supported in non-overlapping mode");
1291+
}
12831292

12841293
let size_in_bytes = size.bytes_usize();
12851294
// For particularly large arrays (where this is perf-sensitive) it's common that
@@ -1292,6 +1301,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
12921301
} else if src_alloc_id == dest_alloc_id {
12931302
let mut dest_ptr = dest_bytes;
12941303
for _ in 0..num_copies {
1304+
// Here we rely on `src` and `dest` being non-overlapping if there is more than
1305+
// one copy.
12951306
ptr::copy(src_bytes, dest_ptr, size_in_bytes);
12961307
dest_ptr = dest_ptr.add(size_in_bytes);
12971308
}

compiler/rustc_middle/src/mir/interpret/allocation.rs

+31-11
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,16 @@ pub trait AllocBytes:
3737
/// Create a zeroed `AllocBytes` of the specified size and alignment.
3838
/// Returns `None` if we ran out of memory on the host.
3939
fn zeroed(size: Size, _align: Align) -> Option<Self>;
40+
41+
/// Gives direct access to the raw underlying storage.
42+
///
43+
/// Crucially this pointer is compatible with:
44+
/// - other pointers retunred by this method, and
45+
/// - references returned from `deref()`, as long as there was no write.
46+
fn as_mut_ptr(&mut self) -> *mut u8;
4047
}
4148

42-
// Default `bytes` for `Allocation` is a `Box<[u8]>`.
49+
/// Default `bytes` for `Allocation` is a `Box<u8>`.
4350
impl AllocBytes for Box<[u8]> {
4451
fn from_bytes<'a>(slice: impl Into<Cow<'a, [u8]>>, _align: Align) -> Self {
4552
Box::<[u8]>::from(slice.into())
@@ -51,6 +58,11 @@ impl AllocBytes for Box<[u8]> {
5158
let bytes = unsafe { bytes.assume_init() };
5259
Some(bytes)
5360
}
61+
62+
fn as_mut_ptr(&mut self) -> *mut u8 {
63+
// Carefully avoiding any intermediate references.
64+
ptr::addr_of_mut!(**self).cast()
65+
}
5466
}
5567

5668
/// This type represents an Allocation in the Miri/CTFE core engine.
@@ -399,10 +411,6 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes>
399411

400412
/// Byte accessors.
401413
impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes> {
402-
pub fn base_addr(&self) -> *const u8 {
403-
self.bytes.as_ptr()
404-
}
405-
406414
/// This is the entirely abstraction-violating way to just grab the raw bytes without
407415
/// caring about provenance or initialization.
408416
///
@@ -452,13 +460,14 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes>
452460
Ok(self.get_bytes_unchecked(range))
453461
}
454462

455-
/// Just calling this already marks everything as defined and removes provenance,
456-
/// so be sure to actually put data there!
463+
/// This is the entirely abstraction-violating way to just get mutable access to the raw bytes.
464+
/// Just calling this already marks everything as defined and removes provenance, so be sure to
465+
/// actually overwrite all the data there!
457466
///
458467
/// It is the caller's responsibility to check bounds and alignment beforehand.
459468
/// Most likely, you want to use the `PlaceTy` and `OperandTy`-based methods
460469
/// on `InterpCx` instead.
461-
pub fn get_bytes_mut(
470+
pub fn get_bytes_unchecked_for_overwrite(
462471
&mut self,
463472
cx: &impl HasDataLayout,
464473
range: AllocRange,
@@ -469,8 +478,9 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes>
469478
Ok(&mut self.bytes[range.start.bytes_usize()..range.end().bytes_usize()])
470479
}
471480

472-
/// A raw pointer variant of `get_bytes_mut` that avoids invalidating existing aliases into this memory.
473-
pub fn get_bytes_mut_ptr(
481+
/// A raw pointer variant of `get_bytes_unchecked_for_overwrite` that avoids invalidating existing immutable aliases
482+
/// into this memory.
483+
pub fn get_bytes_unchecked_for_overwrite_ptr(
474484
&mut self,
475485
cx: &impl HasDataLayout,
476486
range: AllocRange,
@@ -479,10 +489,19 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes>
479489
self.provenance.clear(range, cx)?;
480490

481491
assert!(range.end().bytes_usize() <= self.bytes.len()); // need to do our own bounds-check
492+
// Cruciall, we go via `AllocBytes::as_mut_ptr`, not `AllocBytes::deref_mut`.
482493
let begin_ptr = self.bytes.as_mut_ptr().wrapping_add(range.start.bytes_usize());
483494
let len = range.end().bytes_usize() - range.start.bytes_usize();
484495
Ok(ptr::slice_from_raw_parts_mut(begin_ptr, len))
485496
}
497+
498+
/// This gives direct mutable access to the entire buffer, just exposing their internal state
499+
/// without reseting anything. Directly exposes `AllocBytes::as_mut_ptr`. Only works if
500+
/// `OFFSET_IS_ADDR` is true.
501+
pub fn get_bytes_unchecked_raw_mut(&mut self) -> *mut u8 {
502+
assert!(Prov::OFFSET_IS_ADDR);
503+
self.bytes.as_mut_ptr()
504+
}
486505
}
487506

488507
/// Reading and writing.
@@ -589,7 +608,8 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes>
589608
};
590609

591610
let endian = cx.data_layout().endian;
592-
let dst = self.get_bytes_mut(cx, range)?;
611+
// Yes we do overwrite all the bytes in `dst`.
612+
let dst = self.get_bytes_unchecked_for_overwrite(cx, range)?;
593613
write_target_uint(endian, dst, bytes).unwrap();
594614

595615
// See if we have to also store some provenance.

0 commit comments

Comments
 (0)