Properly tag inline bulk-copy loads/stores with alias regions (#13610)

The small-constant-length inline fast path for `{memory,table,array}.copy`
emitted region-less loads/stores, while every other access to the same bytes is
region-tagged. This led to stale values being forwarded in store-to-load
forwarding.

Author: fitzgen

crates/cranelift/src/func_environ.rs

@@ -433,6 +433,22 @@ impl<'module_environment> FuncEnvironment<'module_environment> {
         self.alias_region(func, key)
     }
 
+    /// Get the alias region for the storage of a bulk-copy entity.
+    fn bulk_copy_alias_region(
+        &mut self,
+        func: &mut Function,
+        entity: CheckedEntity,
+    ) -> Option<ir::AliasRegion> {
+        match entity {
+            CheckedEntity::Memory(index) => Some(self.memory_alias_region(func, index)),
+            CheckedEntity::Table { table, .. } => Some(self.table_alias_region(func, table)),
+            CheckedEntity::Array { .. } => Some(self.gc_heap_alias_region(func)),
+            CheckedEntity::Data { .. } | CheckedEntity::RuntimeData(_) | CheckedEntity::Elem(_) => {
+                None
+            }
+        }
+    }
+
     pub(crate) fn vmctx_alias_region(
         &mut self,
         func: &mut Function,
@@ -3687,14 +3703,18 @@ impl FuncEnvironment<'_> {
             dst,
             src,
             const_len: Some(bytes),
+            src_entity,
+            dst_entity,
             ..
         } = op
         {
             if bytes <= INLINE_COPY_MAX_BYTES {
                 if self.tunables.consume_fuel {
                     self.fuel_consumed += bytes as i64;
                 }
-                self.emit_inline_memcpy(builder, dst, src, bytes);
+                let src_region = self.bulk_copy_alias_region(builder.func, src_entity);
+                let dst_region = self.bulk_copy_alias_region(builder.func, dst_entity);
+                self.emit_inline_memcpy(builder, dst, src, bytes, src_region, dst_region);
                 return;
             }
         }
@@ -4347,6 +4367,8 @@ impl FuncEnvironment<'_> {
                         src: src_raw_addr,
                         len: len_ptr,
                         const_len: const_count,
+                        src_entity,
+                        dst_entity,
                     },
                 );
                 Ok(())
@@ -4704,6 +4726,8 @@ impl FuncEnvironment<'_> {
                     src: src_elem_addr,
                     len: dst_copy_byte_len,
                     const_len,
+                    src_entity,
+                    dst_entity,
                 },
             );
             return Ok(());
@@ -4823,14 +4847,27 @@ impl FuncEnvironment<'_> {
         dst_addr: ir::Value,
         src_addr: ir::Value,
         bytes: u64,
+        src_region: Option<ir::AliasRegion>,
+        dst_region: Option<ir::AliasRegion>,
     ) {
         // `trusted()` (notrap + aligned) is sound: the range is already
         // bounds-checked, and each load feeds only its paired store, so the
         // backend selects unaligned moves regardless of the `aligned` flag.
         // Endianness is pinned to `Little` because Pulley's `v128` load/store
         // only encode the little-endian variant, and matching load/store
         // endianness preserves the destination bytes either way.
-        let flags = ir::MemFlagsData::trusted().with_endianness(Endianness::Little);
+        //
+        // The loads/stores carry the source/destination entity's alias region
+        // so alias analysis keeps them in the same disjoint memory category as
+        // the entity's other (region-tagged) accesses; otherwise a region-less
+        // load here could be forwarded a stale value across an intervening
+        // region-tagged store to the same address.
+        let load_flags = ir::MemFlagsData::trusted()
+            .with_endianness(Endianness::Little)
+            .with_alias_region(src_region);
+        let store_flags = ir::MemFlagsData::trusted()
+            .with_endianness(Endianness::Little)
+            .with_alias_region(dst_region);
         const WIDTHS: &[(u64, ir::Type)] = &[
             (16, ir::types::I8X16),
             (8, ir::types::I64),
@@ -4853,10 +4890,10 @@ impl FuncEnvironment<'_> {
         }
         let vals: SmallVec<[ir::Value; 12]> = chunks
             .iter()
-            .map(|&(off, ty)| builder.ins().load(ty, flags, src_addr, off))
+            .map(|&(off, ty)| builder.ins().load(ty, load_flags, src_addr, off))
             .collect();
         for (&(off, _), val) in chunks.iter().zip(vals) {
-            builder.ins().store(flags, val, dst_addr, off);
+            builder.ins().store(store_flags, val, dst_addr, off);
         }
     }
 
@@ -6319,11 +6356,20 @@ enum BulkOp {
     /// must have type `env.pointer_type()`. `const_len`, when set, is the
     /// statically-known byte length (from a constant wasm length); the inline
     /// fast path in `raw_bulk_memory_operation` uses it to expand small copies.
+    ///
+    /// `src_entity`/`dst_entity` are the source and destination entities,
+    /// carried so that the inline fast path can tag its loads/stores with each
+    /// entity's alias region (the per-memory or GC-heap region); otherwise a
+    /// region-less inline load could be forwarded a stale value across an
+    /// intervening region-tagged store to the same address. They are unused on
+    /// the libcall path.
     MemoryCopy {
         dst: ir::Value,
         src: ir::Value,
         len: ir::Value,
         const_len: Option<u64>,
+        src_entity: CheckedEntity,
+        dst_entity: CheckedEntity,
     },
 
     /// A `memory.fill` operation, setting all bytes of `dst` to `val`.

tests/disas/array-copy-inline.wat

@@ -69,12 +69,12 @@
 ;; @002a                               v70 = uadd_overflow_trap v52, v92, user2  ; v92 = 28
 ;; @002a                               v71 = icmp ugt v70, v62
 ;; @002a                               trapnz v71, user2
-;; @002a                               v72 = load.i8x16 notrap aligned little v52
-;; @002a                               v73 = load.i64 notrap aligned little v52+16
-;; @002a                               v74 = load.i32 notrap aligned little v52+24
-;; @002a                               store notrap aligned little v72, v29
-;; @002a                               store notrap aligned little v73, v29+16
-;; @002a                               store notrap aligned little v74, v29+24
+;; @002a                               v72 = load.i8x16 notrap aligned little region1 v52
+;; @002a                               v73 = load.i64 notrap aligned little region1 v52+16
+;; @002a                               v74 = load.i32 notrap aligned little region1 v52+24
+;; @002a                               store notrap aligned little region1 v72, v29
+;; @002a                               store notrap aligned little region1 v73, v29+16
+;; @002a                               store notrap aligned little region1 v74, v29+24
 ;; @002e                               jump block1
 ;;
 ;;                                 block1:

tests/disas/memory-copy-inline.wat

@@ -13,6 +13,7 @@
 )
 ;; function u0:0(i64 vmctx, i64, i32, i32) tail {
 ;;     region0 = 8 "VMContext+0x8"
+;;     region1 = 805306368 "DefinedMemory(StaticModuleIndex(0), DefinedMemoryIndex(0))"
 ;;     gv0 = vmctx
 ;;     gv1 = load.i64 notrap aligned readonly region0 gv0+8
 ;;     gv2 = load.i64 notrap aligned gv1+24
@@ -34,9 +35,9 @@
 ;; @0024                               trapnz v25, heap_oob
 ;; @0024                               v13 = load.i64 notrap aligned readonly can_move v0+56
 ;; @0024                               v30 = iadd v13, v20
-;; @0024                               v32 = load.i8x16 notrap aligned little v30
+;; @0024                               v32 = load.i8x16 notrap aligned little region1 v30
 ;; @0024                               v17 = iadd v13, v7
-;; @0024                               store notrap aligned little v32, v17
+;; @0024                               store notrap aligned little region1 v32, v17
 ;; @0028                               jump block1
 ;;
 ;;                                 block1:

tests/misc_testsuite/alias-region-memory-copy.wast

@@ -0,0 +1,18 @@
+;;! bulk_memory = true
+
+;; Regression test for an alias-region miscompile in the inline `memory.copy`
+;; fast path (`emit_inline_memcpy`). A small constant-length copy expands to
+;; inline loads/stores that must carry the memory's alias region; otherwise the
+;; region-less load can be store-to-load forwarded a stale value across the
+;; intervening region-tagged `i32.store`, dropping that store.
+(module
+  (memory 1)
+  (func (export "test") (result i32)
+    (i32.store (i32.const 16) (i32.const 0x1111))            ;; addr16 = 0x1111
+    (memory.copy (i32.const 0) (i32.const 16) (i32.const 4)) ;; addr0  = 0x1111  (inline copy)
+    (i32.store (i32.const 0) (i32.const 0xAAAA))             ;; addr0  = 0xAAAA  (region-tagged)
+    (memory.copy (i32.const 32) (i32.const 0) (i32.const 4)) ;; addr32 = *addr0  (inline copy)
+    (i32.load (i32.const 32))                                ;; must be 0xAAAA
+  )
+)
+(assert_return (invoke "test") (i32.const 0xAAAA))

tests/misc_testsuite/gc/alias-region-array-copy.wast

@@ -0,0 +1,25 @@
+;;! gc = true
+
+;; Regression test for an alias-region miscompile in the inline `array.copy`
+;; fast path (`emit_inline_memcpy`). A small constant-length copy of an `i32`
+;; array must tag its inline loads/stores with the GC-heap alias region;
+;; otherwise the region-less load can be store-to-load forwarded a stale value
+;; across the intervening region-tagged `array.set`.
+;;
+;; The array is passed as a function parameter so that the two `array.copy`
+;; element addresses share a single SSA value (a fresh `array.new` local or a
+;; `global.get` reloads the reference and hides the forwarding).
+(module
+  (type $a (array (mut i32)))
+  (func $op (param $arr (ref $a)) (result i32)
+    (array.set $a (local.get $arr) (i32.const 4) (i32.const 0x1111))
+    (array.copy $a $a (local.get $arr) (i32.const 0) (local.get $arr) (i32.const 4) (i32.const 1))
+    (array.set $a (local.get $arr) (i32.const 0) (i32.const 0x2222))
+    (array.copy $a $a (local.get $arr) (i32.const 2) (local.get $arr) (i32.const 0) (i32.const 1))
+    (array.get $a (local.get $arr) (i32.const 2)) ;; must be 0x2222
+  )
+  (func (export "test") (result i32)
+    (call $op (array.new $a (i32.const 0) (i32.const 8)))
+  )
+)
+(assert_return (invoke "test") (i32.const 0x2222))

tests/misc_testsuite/gc/alias-region-table-copy.wast

@@ -0,0 +1,20 @@
+;;! gc = true
+;;! bulk_memory = true
+
+;; Regression test for an alias-region miscompile in the inline `table.copy`
+;; fast path (`emit_inline_memcpy`). A small constant-length copy of an `i31ref`
+;; table must tag its inline loads/stores with the table's alias region;
+;; otherwise the region-less load can be store-to-load forwarded a stale value
+;; across the intervening region-tagged `table.set`. (`i31ref` gives a clean
+;; region-tagged element with no lazy-init libcall to mask the bug.)
+(module
+  (table $t 8 8 i31ref)
+  (func (export "test") (result i32)
+    (table.set $t (i32.const 4) (ref.i31 (i32.const 0x1111)))
+    (table.copy $t $t (i32.const 0) (i32.const 4) (i32.const 1)) ;; t[0] = 0x1111  (inline copy)
+    (table.set $t (i32.const 0) (ref.i31 (i32.const 0x2222)))    ;; t[0] = 0x2222  (region-tagged)
+    (table.copy $t $t (i32.const 2) (i32.const 0) (i32.const 1)) ;; t[2] = *t[0]   (inline copy)
+    (i31.get_u (table.get $t (i32.const 2)))                     ;; must be 0x2222
+  )
+)
+(assert_return (invoke "test") (i32.const 0x2222))