Auto merge of #817 - RalfJung:small-alloc, r=RalfJung

align small malloc-allocations even less, and test that we do

Needs #62295 to land.

Fixes rust-lang/miri#812.

Author: bors

rust-version

@@ -1 +1 @@
-088b987307b91612ab164026e1dcdd0129fdb62b
+24a9bcbb7cb0d8bdc11b8252a9c13f7562c7e4ca

src/eval.rs

@@ -8,7 +8,7 @@ use rustc::hir::def_id::DefId;
 use rustc::mir;
 
 use crate::{
-    InterpResult, InterpError, InterpretCx, StackPopCleanup, struct_error,
+    InterpResult, InterpError, InterpCx, StackPopCleanup, struct_error,
     Scalar, Tag, Pointer,
     MemoryExtra, MiriMemoryKind, Evaluator, TlsEvalContextExt,
 };
@@ -28,8 +28,8 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
     tcx: TyCtxt<'tcx>,
     main_id: DefId,
     config: MiriConfig,
-) -> InterpResult<'tcx, InterpretCx<'mir, 'tcx, Evaluator<'tcx>>> {
-    let mut ecx = InterpretCx::new(
+) -> InterpResult<'tcx, InterpCx<'mir, 'tcx, Evaluator<'tcx>>> {
+    let mut ecx = InterpCx::new(
         tcx.at(syntax::source_map::DUMMY_SP),
         ty::ParamEnv::reveal_all(),
         Evaluator::new(),
@@ -43,7 +43,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
         config.validate
     };
 
-    // FIXME: InterpretCx::new should take an initial MemoryExtra
+    // FIXME: InterpCx::new should take an initial MemoryExtra
     ecx.memory_mut().extra = MemoryExtra::new(config.seed.map(StdRng::seed_from_u64), validate);
 
     let main_instance = ty::Instance::mono(ecx.tcx.tcx, main_id);

src/machine.rs

@@ -25,6 +25,8 @@ pub enum MiriMemoryKind {
     Rust,
     /// `malloc` memory.
     C,
+    /// Windows `HeapAlloc` memory.
+    WinHeap,
     /// Part of env var emulation.
     Env,
     /// Statics.
@@ -103,8 +105,8 @@ impl<'tcx> Evaluator<'tcx> {
     }
 }
 
-/// A rustc InterpretCx for Miri.
-pub type MiriEvalContext<'mir, 'tcx> = InterpretCx<'mir, 'tcx, Evaluator<'tcx>>;
+/// A rustc InterpCx for Miri.
+pub type MiriEvalContext<'mir, 'tcx> = InterpCx<'mir, 'tcx, Evaluator<'tcx>>;
 
 /// A little trait that's useful to be inherited by extension traits.
 pub trait MiriEvalContextExt<'mir, 'tcx> {
@@ -136,14 +138,14 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
     const STATIC_KIND: Option<MiriMemoryKind> = Some(MiriMemoryKind::Static);
 
     #[inline(always)]
-    fn enforce_validity(ecx: &InterpretCx<'mir, 'tcx, Self>) -> bool {
+    fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
         ecx.memory().extra.validate
     }
 
     /// Returns `Ok()` when the function was handled; fail otherwise.
     #[inline(always)]
     fn find_fn(
-        ecx: &mut InterpretCx<'mir, 'tcx, Self>,
+        ecx: &mut InterpCx<'mir, 'tcx, Self>,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, Tag>],
         dest: Option<PlaceTy<'tcx, Tag>>,
@@ -154,7 +156,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
 
     #[inline(always)]
     fn call_intrinsic(
-        ecx: &mut rustc_mir::interpret::InterpretCx<'mir, 'tcx, Self>,
+        ecx: &mut rustc_mir::interpret::InterpCx<'mir, 'tcx, Self>,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, Tag>],
         dest: PlaceTy<'tcx, Tag>,
@@ -164,7 +166,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
 
     #[inline(always)]
     fn ptr_op(
-        ecx: &rustc_mir::interpret::InterpretCx<'mir, 'tcx, Self>,
+        ecx: &rustc_mir::interpret::InterpCx<'mir, 'tcx, Self>,
         bin_op: mir::BinOp,
         left: ImmTy<'tcx, Tag>,
         right: ImmTy<'tcx, Tag>,
@@ -173,7 +175,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
     }
 
     fn box_alloc(
-        ecx: &mut InterpretCx<'mir, 'tcx, Self>,
+        ecx: &mut InterpCx<'mir, 'tcx, Self>,
         dest: PlaceTy<'tcx, Tag>,
     ) -> InterpResult<'tcx> {
         trace!("box_alloc for {:?}", dest.layout.ty);
@@ -239,7 +241,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
     }
 
     #[inline(always)]
-    fn before_terminator(_ecx: &mut InterpretCx<'mir, 'tcx, Self>) -> InterpResult<'tcx>
+    fn before_terminator(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx>
     {
         // We are not interested in detecting loops.
         Ok(())
@@ -309,7 +311,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
 
     #[inline(always)]
     fn retag(
-        ecx: &mut InterpretCx<'mir, 'tcx, Self>,
+        ecx: &mut InterpCx<'mir, 'tcx, Self>,
         kind: mir::RetagKind,
         place: PlaceTy<'tcx, Tag>,
     ) -> InterpResult<'tcx> {
@@ -323,14 +325,14 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
 
     #[inline(always)]
     fn stack_push(
-        ecx: &mut InterpretCx<'mir, 'tcx, Self>,
+        ecx: &mut InterpCx<'mir, 'tcx, Self>,
     ) -> InterpResult<'tcx, stacked_borrows::CallId> {
         Ok(ecx.memory().extra.stacked_borrows.borrow_mut().new_call())
     }
 
     #[inline(always)]
     fn stack_pop(
-        ecx: &mut InterpretCx<'mir, 'tcx, Self>,
+        ecx: &mut InterpCx<'mir, 'tcx, Self>,
         extra: stacked_borrows::CallId,
     ) -> InterpResult<'tcx> {
         Ok(ecx.memory().extra.stacked_borrows.borrow_mut().end_call(extra))
@@ -407,7 +409,7 @@ impl MayLeak for MiriMemoryKind {
     fn may_leak(self) -> bool {
         use self::MiriMemoryKind::*;
         match self {
-            Rust | C => false,
+            Rust | C | WinHeap => false,
             Env | Static => true,
         }
     }

src/shims/foreign_items.rs

@@ -10,30 +10,48 @@ use crate::*;
 
 impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
-    /// Returns the minimum alignment for the target architecture.
-    fn min_align(&self) -> Align {
+    /// Returns the minimum alignment for the target architecture for allocations of the given size.
+    fn min_align(&self, size: u64, kind: MiriMemoryKind) -> Align {
         let this = self.eval_context_ref();
         // List taken from `libstd/sys_common/alloc.rs`.
         let min_align = match this.tcx.tcx.sess.target.target.arch.as_str() {
             "x86" | "arm" | "mips" | "powerpc" | "powerpc64" | "asmjs" | "wasm32" => 8,
             "x86_64" | "aarch64" | "mips64" | "s390x" | "sparc64" => 16,
             arch => bug!("Unsupported target architecture: {}", arch),
         };
-        Align::from_bytes(min_align).unwrap()
+        // Windows always aligns, even small allocations.
+        // Source: <https://support.microsoft.com/en-us/help/286470/how-to-use-pageheap-exe-in-windows-xp-windows-2000-and-windows-server>
+        // But jemalloc does not, so for the C heap we only align if the allocation is sufficiently big.
+        if kind == MiriMemoryKind::WinHeap || size >= min_align {
+            return Align::from_bytes(min_align).unwrap();
+        }
+        // We have `size < min_align`. Round `size` *down* to the next power of two and use that.
+        fn prev_power_of_two(x: u64) -> u64 {
+            let next_pow2 = x.next_power_of_two();
+            if next_pow2 == x {
+                // x *is* a power of two, just use that.
+                x
+            } else {
+                // x is between two powers, so next = 2*prev.
+                next_pow2 / 2
+            }
+        }
+        Align::from_bytes(prev_power_of_two(size)).unwrap()
     }
 
     fn malloc(
         &mut self,
         size: u64,
         zero_init: bool,
+        kind: MiriMemoryKind,
     ) -> Scalar<Tag> {
         let this = self.eval_context_mut();
         let tcx = &{this.tcx.tcx};
         if size == 0 {
             Scalar::from_int(0, this.pointer_size())
         } else {
-            let align = this.min_align();
-            let ptr = this.memory_mut().allocate(Size::from_bytes(size), align, MiriMemoryKind::C.into());
+            let align = this.min_align(size, kind);
+            let ptr = this.memory_mut().allocate(Size::from_bytes(size), align, kind.into());
             if zero_init {
                 // We just allocated this, the access cannot fail
                 this.memory_mut()
@@ -47,14 +65,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     fn free(
         &mut self,
         ptr: Scalar<Tag>,
+        kind: MiriMemoryKind,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         if !this.is_null(ptr)? {
             let ptr = this.force_ptr(ptr)?;
             this.memory_mut().deallocate(
                 ptr,
                 None,
-                MiriMemoryKind::C.into(),
+                kind.into(),
             )?;
         }
         Ok(())
@@ -64,39 +83,38 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         &mut self,
         old_ptr: Scalar<Tag>,
         new_size: u64,
+        kind: MiriMemoryKind,
     ) -> InterpResult<'tcx, Scalar<Tag>> {
         let this = self.eval_context_mut();
-        let align = this.min_align();
+        let new_align = this.min_align(new_size, kind);
         if this.is_null(old_ptr)? {
             if new_size == 0 {
                 Ok(Scalar::from_int(0, this.pointer_size()))
             } else {
                 let new_ptr = this.memory_mut().allocate(
                     Size::from_bytes(new_size),
-                    align,
-                    MiriMemoryKind::C.into()
+                    new_align,
+                    kind.into()
                 );
                 Ok(Scalar::Ptr(new_ptr))
             }
         } else {
             let old_ptr = this.force_ptr(old_ptr)?;
             let memory = this.memory_mut();
-            let old_size = Size::from_bytes(memory.get(old_ptr.alloc_id)?.bytes.len() as u64);
             if new_size == 0 {
                 memory.deallocate(
                     old_ptr,
-                    Some((old_size, align)),
-                    MiriMemoryKind::C.into(),
+                    None,
+                    kind.into(),
                 )?;
                 Ok(Scalar::from_int(0, this.pointer_size()))
             } else {
                 let new_ptr = memory.reallocate(
                     old_ptr,
-                    old_size,
-                    align,
+                    None,
                     Size::from_bytes(new_size),
-                    align,
-                    MiriMemoryKind::C.into(),
+                    new_align,
+                    kind.into(),
                 )?;
                 Ok(Scalar::Ptr(new_ptr))
             }
@@ -145,14 +163,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         match link_name {
             "malloc" => {
                 let size = this.read_scalar(args[0])?.to_usize(this)?;
-                let res = this.malloc(size, /*zero_init:*/ false);
+                let res = this.malloc(size, /*zero_init:*/ false, MiriMemoryKind::C);
                 this.write_scalar(res, dest)?;
             }
             "calloc" => {
                 let items = this.read_scalar(args[0])?.to_usize(this)?;
                 let len = this.read_scalar(args[1])?.to_usize(this)?;
                 let size = items.checked_mul(len).ok_or_else(|| InterpError::Overflow(mir::BinOp::Mul))?;
-                let res = this.malloc(size, /*zero_init:*/ true);
+                let res = this.malloc(size, /*zero_init:*/ true, MiriMemoryKind::C);
                 this.write_scalar(res, dest)?;
             }
             "posix_memalign" => {
@@ -187,12 +205,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             }
             "free" => {
                 let ptr = this.read_scalar(args[0])?.not_undef()?;
-                this.free(ptr)?;
+                this.free(ptr, MiriMemoryKind::C)?;
             }
             "realloc" => {
                 let old_ptr = this.read_scalar(args[0])?.not_undef()?;
                 let new_size = this.read_scalar(args[1])?.to_usize(this)?;
-                let res = this.realloc(old_ptr, new_size)?;
+                let res = this.realloc(old_ptr, new_size, MiriMemoryKind::C)?;
                 this.write_scalar(res, dest)?;
             }
 
@@ -262,12 +280,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 if !align.is_power_of_two() {
                     return err!(HeapAllocNonPowerOfTwoAlignment(align));
                 }
+                let align = Align::from_bytes(align).unwrap();
                 let new_ptr = this.memory_mut().reallocate(
                     ptr,
-                    Size::from_bytes(old_size),
-                    Align::from_bytes(align).unwrap(),
+                    Some((Size::from_bytes(old_size), align)),
                     Size::from_bytes(new_size),
-                    Align::from_bytes(align).unwrap(),
+                    align,
                     MiriMemoryKind::Rust.into(),
                 )?;
                 this.write_scalar(Scalar::Ptr(new_ptr), dest)?;
@@ -327,7 +345,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 trace!("__rust_maybe_catch_panic: {:?}", f_instance);
 
                 // Now we make a function call.
-                // TODO: consider making this reusable? `InterpretCx::step` does something similar
+                // TODO: consider making this reusable? `InterpCx::step` does something similar
                 // for the TLS destructors, and of course `eval_main`.
                 let mir = this.load_mir(f_instance.def)?;
                 let ret_place = MPlaceTy::dangling(this.layout_of(this.tcx.mk_unit())?, this).into();
@@ -765,22 +783,22 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 let flags = this.read_scalar(args[1])?.to_u32()?;
                 let size = this.read_scalar(args[2])?.to_usize(this)?;
                 let zero_init = (flags & 0x00000008) != 0; // HEAP_ZERO_MEMORY
-                let res = this.malloc(size, zero_init);
+                let res = this.malloc(size, zero_init, MiriMemoryKind::WinHeap);
                 this.write_scalar(res, dest)?;
             }
             "HeapFree" => {
                 let _handle = this.read_scalar(args[0])?.to_isize(this)?;
                 let _flags = this.read_scalar(args[1])?.to_u32()?;
                 let ptr = this.read_scalar(args[2])?.not_undef()?;
-                this.free(ptr)?;
+                this.free(ptr, MiriMemoryKind::WinHeap)?;
                 this.write_scalar(Scalar::from_int(1, Size::from_bytes(4)), dest)?;
             }
             "HeapReAlloc" => {
                 let _handle = this.read_scalar(args[0])?.to_isize(this)?;
                 let _flags = this.read_scalar(args[1])?.to_u32()?;
                 let ptr = this.read_scalar(args[2])?.not_undef()?;
                 let size = this.read_scalar(args[3])?.to_usize(this)?;
-                let res = this.realloc(ptr, size)?;
+                let res = this.realloc(ptr, size, MiriMemoryKind::WinHeap)?;
                 this.write_scalar(res, dest)?;
             }
 

tests/run-pass/realloc.rs renamed to tests/run-pass-noseed/malloc.rs

@@ -1,4 +1,5 @@
 //ignore-windows: Uses POSIX APIs
+//compile-flags: -Zmiri-seed=
 
 #![feature(rustc_private)]
 
@@ -7,6 +8,14 @@ use core::{slice, ptr};
 extern crate libc;
 
 fn main() {
+    // Test that small allocations sometimes *are* not very aligned.
+    let saw_unaligned = (0..64).any(|_| unsafe {
+        let p = libc::malloc(3);
+        libc::free(p);
+        (p as usize) % 4 != 0 // find any that this is *not* 4-aligned
+    });
+    assert!(saw_unaligned);
+
     unsafe {
         // Use calloc for initialized memory
         let p1 = libc::calloc(20, 1);