Wasm GC: Unify supported alignment and minimum block size in FreeList

Additionally, support alignment 16 for `v128`s inside GC objects.

We previously allowed the alignment and minimum block size to be different
values, *and* did not round allocation sizes up to the minimum block size. This
could lead to bugs like the following sequence of events:

* free list initially has a block 0x10..MAX
* alloc(size=16) -> 0x10
  * free list now has one block: 0x20..MAX
* alloc(size=16) -> 0x20
  * free list now has one block: 0x30..MAX
* dealloc(0x10, size=16)
  * this merges 0x10..0x20 into 0x30..MAX because the gap between 0x20 and 0x30
    is smaller than the minimum block size
* dealloc(0x20, size=16)
  * triggers overlapping blocks assertion failure!

If we ensure that the minimum allocation size is a multiple of our supported
alignment and we clamp requested allocations' sizes to at least the minimum
block size, then we could avoid this.

It is simpler, however, to unify our supported alignment and our minimum block
size into the same value. This PR does that. No more need for multiple concepts
which interact in subtle ways.

Author: fitzgen

crates/wasmtime/src/runtime/vm/gc/enabled/free_list.rs

@@ -13,13 +13,11 @@ pub(crate) struct FreeList {
 }
 
 /// Our minimum and maximum supported alignment. Every allocation is aligned to
-/// this.
-const ALIGN_U32: u32 = 8;
+/// this. Additionally, this is the minimum allocation size, and every
+/// allocation is rounded up to this size.
+const ALIGN_U32: u32 = 16;
 const ALIGN_USIZE: usize = ALIGN_U32 as usize;
 
-/// Our minimum allocation size.
-const MIN_BLOCK_SIZE: u32 = 24;
-
 impl FreeList {
     /// Create a new `Layout` from the given `size` with an alignment that is
     /// compatible with this free list.
@@ -30,6 +28,7 @@ impl FreeList {
     /// Create a new `FreeList` for a contiguous region of memory of the given
     /// size.
     pub fn new(capacity: usize) -> Self {
+        log::trace!("FreeList::new({capacity})");
         let mut free_list = FreeList {
             capacity,
             free_block_index_to_len: BTreeMap::new(),
@@ -99,7 +98,7 @@ impl FreeList {
         debug_assert_eq!(block_index % ALIGN_U32, 0);
         debug_assert_eq!(block_len % ALIGN_U32, 0);
 
-        if block_len - alloc_size < MIN_BLOCK_SIZE {
+        if block_len - alloc_size < ALIGN_U32 {
             // The block is not large enough to split.
             return block_len;
         }
@@ -130,6 +129,7 @@ impl FreeList {
     ///
     /// * `Err(_)`:
     pub fn alloc(&mut self, layout: Layout) -> Result<Option<NonZeroU32>> {
+        log::trace!("FreeList::alloc({layout:?})");
         let alloc_size = self.check_layout(layout)?;
         debug_assert_eq!(alloc_size % ALIGN_U32, 0);
 
@@ -150,11 +150,14 @@ impl FreeList {
         #[cfg(debug_assertions)]
         self.check_integrity();
 
+        log::trace!("FreeList::alloc({layout:?}) -> {block_index:#x}");
         Ok(Some(unsafe { NonZeroU32::new_unchecked(block_index) }))
     }
 
     /// Deallocate an object with the given layout.
     pub fn dealloc(&mut self, index: NonZeroU32, layout: Layout) {
+        log::trace!("FreeList::dealloc({index:#x}, {layout:?})");
+
         let index = index.get();
         debug_assert_eq!(index % ALIGN_U32, 0);
 
@@ -182,6 +185,12 @@ impl FreeList {
                 if blocks_are_contiguous(prev_index, prev_len, index)
                     && blocks_are_contiguous(index, alloc_size, next_index) =>
             {
+                log::trace!(
+                    "merging blocks {prev_index:#x}..{prev_len:#x}, {index:#x}..{index_end:#x}, {next_index:#x}..{next_end:#x}",
+                    prev_len = prev_index + prev_len,
+                    index_end = index + u32::try_from(layout.size()).unwrap(),
+                    next_end = next_index + next_len,
+                );
                 self.free_block_index_to_len.remove(&next_index);
                 let merged_block_len = next_index + next_len - prev_index;
                 debug_assert_eq!(merged_block_len % ALIGN_U32, 0);
@@ -192,6 +201,11 @@ impl FreeList {
             (Some((prev_index, prev_len)), _)
                 if blocks_are_contiguous(prev_index, prev_len, index) =>
             {
+                log::trace!(
+                    "merging blocks {prev_index:#x}..{prev_len:#x}, {index:#x}..{index_end:#x}",
+                    prev_len = prev_index + prev_len,
+                    index_end = index + u32::try_from(layout.size()).unwrap(),
+                );
                 let merged_block_len = index + alloc_size - prev_index;
                 debug_assert_eq!(merged_block_len % ALIGN_U32, 0);
                 *self.free_block_index_to_len.get_mut(&prev_index).unwrap() = merged_block_len;
@@ -201,6 +215,11 @@ impl FreeList {
             (_, Some((next_index, next_len)))
                 if blocks_are_contiguous(index, alloc_size, next_index) =>
             {
+                log::trace!(
+                    "merging blocks {index:#x}..{index_end:#x}, {next_index:#x}..{next_end:#x}",
+                    index_end = index + u32::try_from(layout.size()).unwrap(),
+                    next_end = next_index + next_len,
+                );
                 self.free_block_index_to_len.remove(&next_index);
                 let merged_block_len = next_index + next_len - index;
                 debug_assert_eq!(merged_block_len % ALIGN_U32, 0);
@@ -210,6 +229,7 @@ impl FreeList {
             // None of the blocks are contiguous: insert this block into the
             // free list.
             (_, _) => {
+                log::trace!("cannot merge blocks");
                 self.free_block_index_to_len.insert(index, alloc_size);
             }
         }
@@ -271,7 +291,7 @@ impl FreeList {
 
         let len = round_u32_down_to_pow2(end.saturating_sub(start), ALIGN_U32);
 
-        let entire_range = if len >= MIN_BLOCK_SIZE {
+        let entire_range = if len >= ALIGN_U32 {
             Some((start, len))
         } else {
             None
@@ -290,9 +310,25 @@ fn blocks_are_contiguous(prev_index: u32, prev_len: u32, next_index: u32) -> boo
     // the size of the `Layout` given to us upon deallocation (aka `prev_len`)
     // is smaller than the actual size of the block we allocated.
     let end_of_prev = prev_index + prev_len;
-    debug_assert!(next_index >= end_of_prev);
+    log::trace!(
+        "checking for overlapping blocks: \n\
+         \t prev_index = {prev_index:#x}\n\
+         \t   prev_len = {prev_len:#x}\n\
+         \tend_of_prev = {end_of_prev:#x}\n\
+         \t next_index = {next_index:#x}\n\
+         `next_index` should be >= `end_of_prev`"
+    );
+    debug_assert!(
+        next_index >= end_of_prev,
+        "overlapping blocks: \n\
+         \t prev_index = {prev_index:#x}\n\
+         \t   prev_len = {prev_len:#x}\n\
+         \tend_of_prev = {end_of_prev:#x}\n\
+         \t next_index = {next_index:#x}\n\
+         `next_index` should be >= `end_of_prev`"
+    );
     let delta_to_next = next_index - end_of_prev;
-    delta_to_next < MIN_BLOCK_SIZE
+    delta_to_next < ALIGN_U32
 }
 
 #[inline]
@@ -479,21 +515,20 @@ mod tests {
     #[test]
     fn allocate_no_split() {
         // Create a free list with the capacity to allocate two blocks of size
-        // `MIN_BLOCK_SIZE`.
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 2);
+        // `ALIGN_U32`.
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 2);
 
         assert_eq!(free_list.free_block_index_to_len.len(), 1);
         assert_eq!(
             free_list.max_size(),
-            usize::try_from(MIN_BLOCK_SIZE).unwrap() * 2
+            usize::try_from(ALIGN_U32).unwrap() * 2
         );
 
         // Allocate a block such that the remainder is not worth splitting.
         free_list
             .alloc(
                 Layout::from_size_align(
-                    usize::try_from(MIN_BLOCK_SIZE).unwrap() + ALIGN_USIZE,
+                    usize::try_from(ALIGN_U32).unwrap() + ALIGN_USIZE,
                     ALIGN_USIZE,
                 )
                 .unwrap(),
@@ -508,21 +543,20 @@ mod tests {
     #[test]
     fn allocate_and_split() {
         // Create a free list with the capacity to allocate three blocks of size
-        // `MIN_BLOCK_SIZE`.
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 3);
+        // `ALIGN_U32`.
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 3);
 
         assert_eq!(free_list.free_block_index_to_len.len(), 1);
         assert_eq!(
             free_list.max_size(),
-            usize::try_from(MIN_BLOCK_SIZE).unwrap() * 3
+            usize::try_from(ALIGN_U32).unwrap() * 3
         );
 
         // Allocate a block such that the remainder is not worth splitting.
         free_list
             .alloc(
                 Layout::from_size_align(
-                    usize::try_from(MIN_BLOCK_SIZE).unwrap() + ALIGN_USIZE,
+                    usize::try_from(ALIGN_U32).unwrap() + ALIGN_USIZE,
                     ALIGN_USIZE,
                 )
                 .unwrap(),
@@ -537,10 +571,9 @@ mod tests {
     #[test]
     fn dealloc_merge_prev_and_next() {
         let layout =
-            Layout::from_size_align(usize::try_from(MIN_BLOCK_SIZE).unwrap(), ALIGN_USIZE).unwrap();
+            Layout::from_size_align(usize::try_from(ALIGN_U32).unwrap(), ALIGN_USIZE).unwrap();
 
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 100);
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 100);
         assert_eq!(
             free_list.free_block_index_to_len.len(),
             1,
@@ -586,10 +619,9 @@ mod tests {
     #[test]
     fn dealloc_merge_with_prev_and_not_next() {
         let layout =
-            Layout::from_size_align(usize::try_from(MIN_BLOCK_SIZE).unwrap(), ALIGN_USIZE).unwrap();
+            Layout::from_size_align(usize::try_from(ALIGN_U32).unwrap(), ALIGN_USIZE).unwrap();
 
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 100);
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 100);
         assert_eq!(
             free_list.free_block_index_to_len.len(),
             1,
@@ -635,10 +667,9 @@ mod tests {
     #[test]
     fn dealloc_merge_with_next_and_not_prev() {
         let layout =
-            Layout::from_size_align(usize::try_from(MIN_BLOCK_SIZE).unwrap(), ALIGN_USIZE).unwrap();
+            Layout::from_size_align(usize::try_from(ALIGN_U32).unwrap(), ALIGN_USIZE).unwrap();
 
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 100);
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 100);
         assert_eq!(
             free_list.free_block_index_to_len.len(),
             1,
@@ -684,10 +715,9 @@ mod tests {
     #[test]
     fn dealloc_no_merge() {
         let layout =
-            Layout::from_size_align(usize::try_from(MIN_BLOCK_SIZE).unwrap(), ALIGN_USIZE).unwrap();
+            Layout::from_size_align(usize::try_from(ALIGN_U32).unwrap(), ALIGN_USIZE).unwrap();
 
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 100);
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 100);
         assert_eq!(
             free_list.free_block_index_to_len.len(),
             1,
@@ -737,18 +767,17 @@ mod tests {
     #[test]
     fn alloc_size_too_large() {
         // Free list with room for 10 min-sized blocks.
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 10);
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 10);
         assert_eq!(
             free_list.max_size(),
-            usize::try_from(MIN_BLOCK_SIZE).unwrap() * 10
+            usize::try_from(ALIGN_U32).unwrap() * 10
         );
 
         // Attempt to allocate something that is 20 times the size of our
         // min-sized block.
         assert!(free_list
             .alloc(
-                Layout::from_size_align(usize::try_from(MIN_BLOCK_SIZE).unwrap() * 20, ALIGN_USIZE)
+                Layout::from_size_align(usize::try_from(ALIGN_U32).unwrap() * 20, ALIGN_USIZE)
                     .unwrap(),
             )
             .is_err());
@@ -757,20 +786,49 @@ mod tests {
     #[test]
     fn alloc_align_too_large() {
         // Free list with room for 10 min-sized blocks.
-        let mut free_list =
-            FreeList::new(ALIGN_USIZE + usize::try_from(MIN_BLOCK_SIZE).unwrap() * 10);
+        let mut free_list = FreeList::new(ALIGN_USIZE + usize::try_from(ALIGN_U32).unwrap() * 10);
         assert_eq!(
             free_list.max_size(),
-            usize::try_from(MIN_BLOCK_SIZE).unwrap() * 10
+            usize::try_from(ALIGN_U32).unwrap() * 10
         );
 
         // Attempt to allocate something that requires larger alignment than
         // `FreeList` supports.
         assert!(free_list
             .alloc(
-                Layout::from_size_align(usize::try_from(MIN_BLOCK_SIZE).unwrap(), ALIGN_USIZE * 2)
+                Layout::from_size_align(usize::try_from(ALIGN_U32).unwrap(), ALIGN_USIZE * 2)
                     .unwrap(),
             )
             .is_err());
     }
+
+    #[test]
+    fn all_pairwise_alloc_dealloc_orderings() {
+        let tests: &[fn(&mut FreeList, Layout)] = &[
+            |f, l| {
+                let a = f.alloc(l).unwrap().unwrap();
+                let b = f.alloc(l).unwrap().unwrap();
+                f.dealloc(a, l);
+                f.dealloc(b, l);
+            },
+            |f, l| {
+                let a = f.alloc(l).unwrap().unwrap();
+                let b = f.alloc(l).unwrap().unwrap();
+                f.dealloc(b, l);
+                f.dealloc(a, l);
+            },
+            |f, l| {
+                let a = f.alloc(l).unwrap().unwrap();
+                f.dealloc(a, l);
+                let b = f.alloc(l).unwrap().unwrap();
+                f.dealloc(b, l);
+            },
+        ];
+
+        let l = Layout::from_size_align(16, 8).unwrap();
+        for test in tests {
+            let mut f = FreeList::new(0x100);
+            test(&mut f, l);
+        }
+    }
 }

tests/misc_testsuite/gc/alloc-v128-struct.wast

@@ -0,0 +1,12 @@
+;;! gc = true
+;;! simd = true
+
+(module
+  (type $s (struct (field v128)))
+  (func (export "alloc")
+    struct.new_default $s
+    drop
+  )
+)
+
+(assert_return (invoke "alloc"))