Bug fix: assertion fail when allocating for sizes that are not a multiple of MIN_ALIGNMENT

.github/scripts/ci-test.sh

@@ -2,4 +2,12 @@ set -xe
 
 cargo test --features nogc
 cargo test --features semispace
-python examples/build.py
+python examples/build.py
+
+# Test with DummyVM (each test in a separate run)
+cd vmbindings/dummyvm
+for p in $(find ../../src/plan -mindepth 1 -type d | xargs -L 1 basename); do
+    for t in $(ls src/tests/ -I mod.rs | sed -n 's/\.rs$//p'); do
+        cargo test --features $p -- $t;
+    done;
+done;

.github/workflows/pre-review-ci.yml

@@ -32,7 +32,7 @@ jobs:
         run: |
           sudo apt-get update
           sudo apt-get install libc6-dev-i386
-          export RUSTUP_TOOLCHAIN=
+          export RUSTUP_TOOLCHAIN=nightly-2020-07-08
           ./.github/scripts/ci-test.sh
 
       # Style checks

examples/build.py

@@ -8,7 +8,7 @@
 
 # check RUSTUP_TOOLCHAIN
 if "RUSTUP_TOOLCHAIN" in os.environ:
-    toolchain = os.environ["RUSTUP_TOOLCHAIN"]
+    toolchain = "+" + os.environ["RUSTUP_TOOLCHAIN"]
 else:
     toolchain = "+nightly"
 

src/plan/collector_context.rs

@@ -32,10 +32,10 @@ pub trait CollectorContext<VM: VMBinding> {
         align: usize,
         allocator: Allocator,
     ) -> Allocator {
-        let large = crate::util::alloc::allocator::get_maximum_aligned_size(
+        let large = crate::util::alloc::allocator::get_maximum_aligned_size::<VM>(
             bytes,
             align,
-            crate::util::alloc::allocator::MIN_ALIGNMENT,
+            VM::MIN_ALIGNMENT,
         ) > MAX_NON_LOS_COPY_BYTES;
         if large {
             Allocator::Los

src/util/alloc/allocator.rs

@@ -10,45 +10,38 @@ use crate::util::OpaquePointer;
 use crate::vm::VMBinding;
 use crate::vm::{ActivePlan, Collection};
 
-// FIXME: Put this somewhere more appropriate
-pub const ALIGNMENT_VALUE: usize = 0xdead_beef;
-pub const LOG_MIN_ALIGNMENT: usize = LOG_BYTES_IN_INT as usize;
-pub const MIN_ALIGNMENT: usize = 1 << LOG_MIN_ALIGNMENT;
-#[cfg(target_arch = "x86")]
-pub const MAX_ALIGNMENT_SHIFT: usize = 1 + LOG_BYTES_IN_LONG as usize - LOG_BYTES_IN_INT as usize;
-#[cfg(target_arch = "x86_64")]
-pub const MAX_ALIGNMENT_SHIFT: usize = LOG_BYTES_IN_LONG as usize - LOG_BYTES_IN_INT as usize;
-
-pub const MAX_ALIGNMENT: usize = MIN_ALIGNMENT << MAX_ALIGNMENT_SHIFT;
-
 #[inline(always)]
-pub fn align_allocation_no_fill(region: Address, alignment: usize, offset: isize) -> Address {
-    align_allocation(region, alignment, offset, MIN_ALIGNMENT, false)
+pub fn align_allocation_no_fill<VM: VMBinding>(
+    region: Address,
+    alignment: usize,
+    offset: isize,
+) -> Address {
+    align_allocation::<VM>(region, alignment, offset, VM::MIN_ALIGNMENT, false)
 }
 
 #[inline(always)]
-pub fn align_allocation(
+pub fn align_allocation<VM: VMBinding>(
     region: Address,
     alignment: usize,
     offset: isize,
     known_alignment: usize,
     fillalignmentgap: bool,
 ) -> Address {
-    debug_assert!(known_alignment >= MIN_ALIGNMENT);
+    debug_assert!(known_alignment >= VM::MIN_ALIGNMENT);
     // Make sure MIN_ALIGNMENT is reasonable.
     #[allow(clippy::assertions_on_constants)]
     {
-        debug_assert!(MIN_ALIGNMENT >= BYTES_IN_INT);
+        debug_assert!(VM::MIN_ALIGNMENT >= BYTES_IN_INT);
     }
     debug_assert!(!(fillalignmentgap && region.is_zero()));
-    debug_assert!(alignment <= MAX_ALIGNMENT);
+    debug_assert!(alignment <= VM::MAX_ALIGNMENT);
     debug_assert!(offset >= 0);
-    debug_assert!(region.is_aligned_to(MIN_ALIGNMENT));
-    debug_assert!((alignment & (MIN_ALIGNMENT - 1)) == 0);
-    debug_assert!((offset & (MIN_ALIGNMENT - 1) as isize) == 0);
+    debug_assert!(region.is_aligned_to(VM::ALLOC_END_ALIGNMENT));
+    debug_assert!((alignment & (VM::MIN_ALIGNMENT - 1)) == 0);
+    debug_assert!((offset & (VM::MIN_ALIGNMENT - 1) as isize) == 0);
 
     // No alignment ever required.
-    if alignment <= known_alignment || MAX_ALIGNMENT <= MIN_ALIGNMENT {
+    if alignment <= known_alignment || VM::MAX_ALIGNMENT <= VM::MIN_ALIGNMENT {
         return region;
     }
 
@@ -59,47 +52,51 @@ pub fn align_allocation(
     let neg_off = -offset; // fromIntSignExtend
     let delta = (neg_off - region_isize) & mask;
 
-    if fillalignmentgap && (ALIGNMENT_VALUE != 0) {
-        fill_alignment_gap(region, region + delta);
+    if fillalignmentgap && (VM::ALIGNMENT_VALUE != 0) {
+        fill_alignment_gap::<VM>(region, region + delta);
     }
 
     region + delta
 }
 
 #[inline(always)]
-pub fn fill_alignment_gap(immut_start: Address, end: Address) {
+pub fn fill_alignment_gap<VM: VMBinding>(immut_start: Address, end: Address) {
     let mut start = immut_start;
 
-    if MAX_ALIGNMENT - MIN_ALIGNMENT == BYTES_IN_INT {
+    if VM::MAX_ALIGNMENT - VM::MIN_ALIGNMENT == BYTES_IN_INT {
         // At most a single hole
         if end - start != 0 {
             unsafe {
-                start.store(ALIGNMENT_VALUE);
+                start.store(VM::ALIGNMENT_VALUE);
             }
         }
     } else {
         while start < end {
             unsafe {
-                start.store(ALIGNMENT_VALUE);
+                start.store(VM::ALIGNMENT_VALUE);
             }
             start += BYTES_IN_INT;
         }
     }
 }
 
 #[inline(always)]
-pub fn get_maximum_aligned_size(size: usize, alignment: usize, known_alignment: usize) -> usize {
+pub fn get_maximum_aligned_size<VM: VMBinding>(
+    size: usize,
+    alignment: usize,
+    known_alignment: usize,
+) -> usize {
     trace!(
         "size={}, alignment={}, known_alignment={}, MIN_ALIGNMENT={}",
         size,
         alignment,
         known_alignment,
-        MIN_ALIGNMENT
+        VM::MIN_ALIGNMENT
     );
     debug_assert!(size == size & !(known_alignment - 1));
-    debug_assert!(known_alignment >= MIN_ALIGNMENT);
+    debug_assert!(known_alignment >= VM::MIN_ALIGNMENT);
 
-    if MAX_ALIGNMENT <= MIN_ALIGNMENT || alignment <= known_alignment {
+    if VM::MAX_ALIGNMENT <= VM::MIN_ALIGNMENT || alignment <= known_alignment {
         size
     } else {
         size + alignment - known_alignment

src/util/alloc/bumpallocator.rs

@@ -94,14 +94,14 @@ impl<VM: VMBinding> Allocator<VM> for BumpAllocator<VM> {
 
     fn alloc(&mut self, size: usize, align: usize, offset: isize) -> Address {
         trace!("alloc");
-        let result = align_allocation_no_fill(self.cursor, align, offset);
+        let result = align_allocation_no_fill::<VM>(self.cursor, align, offset);
         let new_cursor = result + size;
 
         if new_cursor > self.limit {
             trace!("Thread local buffer used up, go to alloc slow path");
             self.alloc_slow(size, align, offset)
         } else {
-            fill_alignment_gap(self.cursor, result);
+            fill_alignment_gap::<VM>(self.cursor, result);
             self.cursor = new_cursor;
             trace!(
                 "Bump allocation size: {}, result: {}, new_cursor: {}, limit: {}",

src/util/alloc/large_object_allocator.rs

@@ -28,7 +28,7 @@ impl<VM: VMBinding> Allocator<VM> for LargeObjectAllocator<VM> {
 
     fn alloc(&mut self, size: usize, align: usize, offset: isize) -> Address {
         let cell: Address = self.alloc_slow(size, align, offset);
-        allocator::align_allocation(cell, align, offset, allocator::MIN_ALIGNMENT, true)
+        allocator::align_allocation::<VM>(cell, align, offset, VM::MIN_ALIGNMENT, true)
     }
 
     fn alloc_slow(&mut self, size: usize, align: usize, offset: isize) -> Address {
@@ -38,7 +38,7 @@ impl<VM: VMBinding> Allocator<VM> for LargeObjectAllocator<VM> {
     fn alloc_slow_once(&mut self, size: usize, align: usize, _offset: isize) -> Address {
         let header = 0; // HashSet is used instead of DoublyLinkedList
         let maxbytes =
-            allocator::get_maximum_aligned_size(size + header, align, allocator::MIN_ALIGNMENT);
+            allocator::get_maximum_aligned_size::<VM>(size + header, align, VM::MIN_ALIGNMENT);
         let pages = crate::util::conversions::bytes_to_pages_up(maxbytes);
         let sp = self.space.unwrap().allocate_pages(self.tls, pages);
         if sp.is_zero() {

src/vm/mod.rs

@@ -1,3 +1,5 @@
+use crate::util::constants::*;
+
 mod active_plan;
 mod collection;
 mod object_model;
@@ -19,4 +21,20 @@ where
     type VMCollection: Collection<Self>;
     type VMActivePlan: ActivePlan<Self>;
     type VMReferenceGlue: ReferenceGlue<Self>;
+
+    const ALIGNMENT_VALUE: usize = 0xdead_beef;
+    const LOG_MIN_ALIGNMENT: usize = LOG_BYTES_IN_INT as usize;
+    const MIN_ALIGNMENT: usize = 1 << Self::LOG_MIN_ALIGNMENT;
+    #[cfg(target_arch = "x86")]
+    const MAX_ALIGNMENT_SHIFT: usize = 1 + LOG_BYTES_IN_LONG as usize - LOG_BYTES_IN_INT as usize;
+    #[cfg(target_arch = "x86_64")]
+    const MAX_ALIGNMENT_SHIFT: usize = LOG_BYTES_IN_LONG as usize - LOG_BYTES_IN_INT as usize;
+
+    const MAX_ALIGNMENT: usize = Self::MIN_ALIGNMENT << Self::MAX_ALIGNMENT_SHIFT;
+
+    // This value is used to assert if the cursor is reasonable after last allocation.
+    // At the end of an allocation, the allocation cursor should be aligned to this value.
+    // Note that MMTk does not attempt to do anything to align the cursor to this value, but
+    // it merely asserts with this constant.
+    const ALLOC_END_ALIGNMENT: usize = 1;
 }

vmbindings/dummyvm/src/lib.rs

@@ -13,6 +13,9 @@ pub mod active_plan;
 pub mod reference_glue;
 pub mod api;
 
+#[cfg(test)]
+mod tests;
+
 pub struct DummyVM;
 
 impl VMBinding for DummyVM {

vmbindings/dummyvm/src/tests/issue139.rs

@@ -0,0 +1,17 @@
+use crate::api::*;
+use mmtk::util::OpaquePointer;
+use mmtk::Allocator;
+
+#[test]
+pub fn issue139_alloc_non_multiple_of_min_alignment() {
+    gc_init(200*1024*1024);
+    let handle = bind_mutator(OpaquePointer::UNINITIALIZED);
+
+    // Allocate 6 bytes with 8 bytes ailgnment required
+    let addr = alloc(handle, 6, 8, 0, Allocator::Default);
+    assert!(addr.is_aligned_to(8));
+    // After the allocation, the cursor is not MIN_ALIGNMENT aligned. If we have the assertion in the next allocation to check if the cursor is aligned to MIN_ALIGNMENT, it fails.
+    // We have to remove that assertion.
+    let addr2 = alloc(handle, 6, 8, 0, Allocator::Default);
+    assert!(addr2.is_aligned_to(8));
+}

vmbindings/dummyvm/src/tests/mod.rs

@@ -0,0 +1,4 @@
+// Each module should only contain one #[test] function.
+// We should run each module in a separate test process, as we do not have proper
+// setup/teardown procedure for MMTk instances.
+mod issue139;