Repository: bytecodealliance/wasmtime
Pull request #8176 (2024-03-18 22:55:57)

PCC: support imported memories as well.
Exposed by a fuzzbug (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=67429); rather than exclude from fuzzing, it seemed easier to just implement. We need  to define a new memory type to describe the memory definition struct pointed to by vmctx, and set up points-to facts appropriately.

alexcrichton: I don't know enough here to really review what's going on, but could the pcc-related pieces get factored into their own functions/methods? Right now they're suffering quite a bit from a huge indentation due to being so nested so breaking them out might help with readability
cfallin: Perhaps; the original intent was to put the PCC logic alongside the CLIF generation because they "mirror" each other (both have largely the same case breakdown, so we'd mirror the control flow elsewhere, which seems more brittle). Happy to consider refactors, unfortunately don't have much time to do a big refactor now. @fitzgen and I cowrote the original; @fitzgen would you be willing to review the fuzzbug fix here?
cfallin: @alexcrichton On second look, I did a small refactor on some of the logic so hopefully it's at least a little easier to digest now -- let me know what you think. Thanks!
alexcrichton: Seems reasonable to me! Alas though I fear I know so little about how this is all supposed to work that I still can't really make heads or tails of it without diving much deeper into pcc stuff, so I'll leave @fitzgen to review

Patch:
diff --git a/.github/workflows/cargo-audit.yml b/.github/workflows/cargo-audit.yml
index 0f4b3bcb8..4b7486336 100644
--- a/.github/workflows/cargo-audit.yml
+++ b/.github/workflows/cargo-audit.yml
@@ -10,6 +10,6 @@ jobs:
       - uses: actions/checkout@v4
         with:
           submodules: true
-      - uses: actions-rs/audit-check@v1
+      - uses: rustsec/audit-check@v1.4.1
         with:
           token: ${{ secrets.GITHUB_TOKEN }}
diff --git a/RELEASES.md b/RELEASES.md
index 9a554aed0..52b80b5b5 100644
--- a/RELEASES.md
+++ b/RELEASES.md
@@ -16,7 +16,7 @@ Unreleased.
 
 ## 19.0.0
 
-Unreleased.
+Released 2024-03-20
 
 ### Added
 
diff --git a/cranelift/codegen/src/nan_canonicalization.rs b/cranelift/codegen/src/nan_canonicalization.rs
index bb791d60b..c1bc16f77 100644
--- a/cranelift/codegen/src/nan_canonicalization.rs
+++ b/cranelift/codegen/src/nan_canonicalization.rs
@@ -38,6 +38,10 @@ fn is_fp_arith(pos: &mut FuncCursor, inst: Inst) -> bool {
                 || opcode == Opcode::Nearest
                 || opcode == Opcode::Sqrt
                 || opcode == Opcode::Trunc
+                || opcode == Opcode::Fdemote
+                || opcode == Opcode::Fpromote
+                || opcode == Opcode::FvpromoteLow
+                || opcode == Opcode::Fvdemote
         }
         InstructionData::Binary { opcode, .. } => {
             opcode == Opcode::Fadd
@@ -48,24 +52,6 @@ fn is_fp_arith(pos: &mut FuncCursor, inst: Inst) -> bool {
                 || opcode == Opcode::Fsub
         }
         InstructionData::Ternary { opcode, .. } => opcode == Opcode::Fma,
-
-        // bitcasts coming from arbitrary integers need canonicalization as the
-        // integer isn't known to necessarily be a canonical NaN
-        InstructionData::LoadNoOffset {
-            opcode: Opcode::Bitcast,
-            arg,
-            flags: _,
-        } => {
-            let ret = pos.func.dfg.first_result(inst);
-            let ret_type = pos.func.dfg.value_type(ret);
-            let arg_type = pos.func.dfg.value_type(arg);
-            match (arg_type, ret_type) {
-                (types::I32, types::F32) => true,
-                (types::I64, types::F64) => true,
-                _ => false,
-            }
-        }
-
         _ => false,
     }
 }
diff --git a/crates/cranelift/src/func_environ.rs b/crates/cranelift/src/func_environ.rs
index 6104a9411..cf2b96f15 100644
--- a/crates/cranelift/src/func_environ.rs
+++ b/crates/cranelift/src/func_environ.rs
@@ -1037,6 +1037,87 @@ impl<'module_environment> FuncEnvironment<'module_environment> {
             .get(&func_index)
             .map(|s| *s)
     }
+
+    /// Proof-carrying code: create a memtype describing an empty
+    /// runtime struct (to be updated later).
+    fn create_empty_struct_memtype(&self, func: &mut ir::Function) -> ir::MemoryType {
+        func.create_memory_type(ir::MemoryTypeData::Struct {
+            size: 0,
+            fields: vec![],
+        })
+    }
+
+    /// Proof-carrying code: add a new field to a memtype used to
+    /// describe a runtime struct. A memory region of type `memtype`
+    /// will have a pointer at `offset` pointing to another memory
+    /// region of type `pointee`. `readonly` indicates whether the
+    /// PCC-checked code is expected to update this field or not.
+    fn add_field_to_memtype(
+        &self,
+        func: &mut ir::Function,
+        memtype: ir::MemoryType,
+        offset: u32,
+        pointee: ir::MemoryType,
+        readonly: bool,
+    ) {
+        let ptr_size = self.pointer_type().bytes();
+        match &mut func.memory_types[memtype] {
+            ir::MemoryTypeData::Struct { size, fields } => {
+                *size = std::cmp::max(*size, offset.checked_add(ptr_size).unwrap().into());
+                fields.push(ir::MemoryTypeField {
+                    ty: self.pointer_type(),
+                    offset: offset.into(),
+                    readonly,
+                    fact: Some(ir::Fact::Mem {
+                        ty: pointee,
+                        min_offset: 0,
+                        max_offset: 0,
+                        nullable: false,
+                    }),
+                });
+
+                // Sort fields by offset -- we need to do this now
+                // because we may create an arbitrary number of
+                // memtypes for imported memories and we don't
+                // otherwise track them.
+                fields.sort_by_key(|f| f.offset);
+            }
+            _ => panic!("Cannot add field to non-struct memtype"),
+        }
+    }
+
+    /// Add one level of indirection to a pointer-and-memtype pair:
+    /// generate a load in the code at the specified offset, and if
+    /// memtypes are in use, add a field to the original struct and
+    /// generate a new memtype for the pointee.
+    fn load_pointer_with_memtypes(
+        &self,
+        func: &mut ir::Function,
+        value: ir::GlobalValue,
+        offset: u32,
+        readonly: bool,
+        memtype: Option<ir::MemoryType>,
+    ) -> (ir::GlobalValue, Option<ir::MemoryType>) {
+        let pointee = func.create_global_value(ir::GlobalValueData::Load {
+            base: value,
+            offset: Offset32::new(i32::try_from(offset).unwrap()),
+            global_type: self.pointer_type(),
+            flags: MemFlags::trusted().with_readonly(),
+        });
+
+        let mt = memtype.map(|mt| {
+            let pointee_mt = self.create_empty_struct_memtype(func);
+            self.add_field_to_memtype(func, mt, offset, pointee_mt, readonly);
+            func.global_value_facts[pointee] = Some(Fact::Mem {
+                ty: pointee_mt,
+                min_offset: 0,
+                max_offset: 0,
+                nullable: false,
+            });
+            pointee_mt
+        });
+        (pointee, mt)
+    }
 }
 
 struct Call<'a, 'func, 'module_env> {
@@ -2037,16 +2118,17 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
                     // VMMemoryDefinition` to it and dereference that when
                     // atomically growing it.
                     let from_offset = self.offsets.vmctx_vmmemory_pointer(def_index);
-                    let memory = func.create_global_value(ir::GlobalValueData::Load {
-                        base: vmctx,
-                        offset: Offset32::new(i32::try_from(from_offset).unwrap()),
-                        global_type: pointer_type,
-                        flags: MemFlags::trusted().with_readonly(),
-                    });
+                    let (memory, def_mt) = self.load_pointer_with_memtypes(
+                        func,
+                        vmctx,
+                        from_offset,
+                        true,
+                        self.pcc_vmctx_memtype,
+                    );
                     let base_offset = i32::from(self.offsets.ptr.vmmemory_definition_base());
                     let current_length_offset =
                         i32::from(self.offsets.ptr.vmmemory_definition_current_length());
-                    (memory, base_offset, current_length_offset, None)
+                    (memory, base_offset, current_length_offset, def_mt)
                 } else {
                     let owned_index = self.module.owned_memory_index(def_index);
                     let owned_base_offset =
@@ -2065,16 +2147,17 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
                 }
             } else {
                 let from_offset = self.offsets.vmctx_vmmemory_import_from(index);
-                let memory = func.create_global_value(ir::GlobalValueData::Load {
-                    base: vmctx,
-                    offset: Offset32::new(i32::try_from(from_offset).unwrap()),
-                    global_type: pointer_type,
-                    flags: MemFlags::trusted().with_readonly(),
-                });
+                let (memory, def_mt) = self.load_pointer_with_memtypes(
+                    func,
+                    vmctx,
+                    from_offset,
+                    true,
+                    self.pcc_vmctx_memtype,
+                );
                 let base_offset = i32::from(self.offsets.ptr.vmmemory_definition_base());
                 let current_length_offset =
                     i32::from(self.offsets.ptr.vmmemory_definition_current_length());
-                (memory, base_offset, current_length_offset, None)
+                (memory, base_offset, current_length_offset, def_mt)
             }
         };
 
@@ -2140,7 +2223,9 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
                                 );
                                 *size = std::cmp::max(*size, fields_end);
                             }
-                            _ => {}
+                            _ => {
+                                panic!("Bad memtype");
+                            }
                         }
                         // Apply a fact to the base pointer.
                         (Some(base_fact), Some(data_mt))
@@ -2199,7 +2284,9 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
                                     offset + u64::from(self.isa.pointer_type().bytes()),
                                 );
                             }
-                            _ => {}
+                            _ => {
+                                panic!("Bad memtype");
+                            }
                         }
                         // Apply a fact to the base pointer.
                         (Some(base_fact), Some(data_mt))
@@ -2829,16 +2916,6 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m
         if self.tunables.consume_fuel && state.reachable() {
             self.fuel_function_exit(builder);
         }
-        if let Some(pcc_vmctx_memtype) = self.pcc_vmctx_memtype {
-            // Sort the fields by offset in the struct definition for
-            // vmctx, now that we've completed it.
-            match &mut builder.func.memory_types[pcc_vmctx_memtype] {
-                ir::MemoryTypeData::Struct { fields, .. } => {
-                    fields.sort_by_key(|f| f.offset);
-                }
-                _ => {}
-            }
-        }
         Ok(())
     }
 
diff --git a/crates/runtime/src/traphandlers.rs b/crates/runtime/src/traphandlers.rs
index 06855d307..92003e8b3 100644
--- a/crates/runtime/src/traphandlers.rs
+++ b/crates/runtime/src/traphandlers.rs
@@ -521,15 +521,6 @@ impl CallThreadState {
     }
 }
 
-struct ResetCell<'a, T: Copy>(&'a Cell<T>, T);
-
-impl<T: Copy> Drop for ResetCell<'_, T> {
-    #[inline]
-    fn drop(&mut self) {
-        self.0.set(self.1);
-    }
-}
-
 // A private inner module for managing the TLS state that we require across
 // calls in wasm. The WebAssembly code is called from C++ and then a trap may
 // happen which requires us to read some contextual state to figure out what to
diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs
index bef3e7061..271c9bc25 100644
--- a/crates/wasmtime/src/config.rs
+++ b/crates/wasmtime/src/config.rs
@@ -1009,9 +1009,14 @@ impl Config {
     /// Configures whether Cranelift should perform a NaN-canonicalization pass.
     ///
     /// When Cranelift is used as a code generation backend this will configure
-    /// it to replace NaNs with a single canonical value. This is useful for users
-    /// requiring entirely deterministic WebAssembly computation.
-    /// This is not required by the WebAssembly spec, so it is not enabled by default.
+    /// it to replace NaNs with a single canonical value. This is useful for
+    /// users requiring entirely deterministic WebAssembly computation.  This is
+    /// not required by the WebAssembly spec, so it is not enabled by default.
+    ///
+    /// Note that this option affects not only WebAssembly's `f32` and `f64`
+    /// types but additionally the `v128` type. This option will cause
+    /// operations using any of these types to have extra checks placed after
+    /// them to normalize NaN values as needed.
     ///
     /// The default value for this is `false`
     #[cfg(any(feature = "cranelift", feature = "winch"))]


Test patch:
diff --git a/tests/disas/pcc-imported-memory.wat b/tests/disas/pcc-imported-memory.wat
new file mode 100644
index 000000000..5bd75ad01
--- /dev/null
+++ b/tests/disas/pcc-imported-memory.wat
@@ -0,0 +1,78 @@
+;;! target = "x86_64"
+;;! test = "compile"
+;;! flags = [ "-Oopt-level=2", "-Cpcc=y" ]
+
+(module
+  (type (;0;) (func))
+  (import "" "" (memory (;0;) 1))
+  (func (;0;) (type 0)
+    (local i32 i32)
+    memory.size
+    local.set 0
+    block  ;; label = @1
+      block  ;; label = @2
+        memory.size
+        i32.const 65536
+        i32.mul
+        i32.const 4
+        local.get 0
+        i32.add
+        i32.le_u
+        br_if 0 (;@2;)
+        local.get 0
+        i32.const 0
+        i32.le_s
+        br_if 0 (;@2;)
+        local.get 0
+        i32.load align=1
+        local.set 1
+        br 1 (;@1;)
+      end
+      i32.const 0
+      local.set 1
+    end
+    local.get 1
+    drop))
+
+;; function u0:0:
+;;   pushq   %rbp
+;;   unwind PushFrameRegs { offset_upward_to_caller_sp: 16 }
+;;   movq    %rsp, %rbp
+;;   movq    8(%rdi), %r10
+;;   movq    0(%r10), %r10
+;;   cmpq    %rsp, %r10
+;;   jnbe #trap=stk_ovf
+;;   unwind DefineNewFrame { offset_upward_to_caller_sp: 16, offset_downward_to_clobbers: 0 }
+;; block0:
+;;   movq    72(%rdi), %r10
+;;   movq    8(%r10), %rax
+;;   movl    $65536, %r10d
+;;   xorq    %rdx, %rdx, %rdx
+;;   div     %rax, %rdx, %r10, %rax, %rdx ; trap=int_divz
+;;   movq    %rax, %r9
+;;   shll    $16, %r9d, %r9d
+;;   lea     4(%rax), %r10d
+;;   cmpl    %r10d, %r9d
+;;   jbe     label1; j label2
+;; block1:
+;;   jmp     label5
+;; block2:
+;;   testl   %eax, %eax
+;;   jle     label3; j label4
+;; block3:
+;;   jmp     label5
+;; block4:
+;;   movq    72(%rdi), %rcx
+;;   movq    0(%rcx), %rcx
+;;   movl    %eax, %edx
+;;   movl    0(%rcx,%rdx,1), %edi
+;;   jmp     label6
+;; block5:
+;;   xorl    %edi, %edi, %edi
+;;   jmp     label6
+;; block6:
+;;   jmp     label7
+;; block7:
+;;   movq    %rbp, %rsp
+;;   popq    %rbp
+;;   ret
diff --git a/tests/misc_testsuite/simd/canonicalize-nan.wast b/tests/misc_testsuite/simd/canonicalize-nan.wast
index b35a941f0..8a0ae71d9 100644
--- a/tests/misc_testsuite/simd/canonicalize-nan.wast
+++ b/tests/misc_testsuite/simd/canonicalize-nan.wast
@@ -39,6 +39,32 @@
     f64.reinterpret_i64
     f32.demote_f64
     i32.reinterpret_f32)
+
+  (func (export "reinterpret-and-promote") (param i32) (result i64)
+    local.get 0
+    f32.reinterpret_i32
+    f64.promote_f32
+    i64.reinterpret_f64)
+
+  (func (export "copysign-and-demote") (param f64) (result f32)
+    local.get 0
+    f64.const -0x1
+    f64.copysign
+    f32.demote_f64)
+
+  (func (export "copysign-and-promote") (param f32) (result f64)
+    local.get 0
+    f32.const -0x1
+    f32.copysign
+    f64.promote_f32)
+
+  (func (export "f32x4.demote_f64x2_zero") (param v128) (result v128)
+    local.get 0
+    f32x4.demote_f64x2_zero)
+
+  (func (export "f64x2.promote_low_f32x4") (param v128) (result v128)
+    local.get 0
+    f64x2.promote_low_f32x4)
 )
 
 (assert_return (invoke "f32x4.floor" (v128.const f32x4 1 -2.2 3.4 nan))
@@ -65,3 +91,17 @@
 
 (assert_return (invoke "reinterpret-and-demote" (i64.const 0xfffefdfccccdcecf))
                (i32.const 0x7fc00000))
+(assert_return (invoke "reinterpret-and-promote" (i32.const 0xfffefdfc))
+               (i64.const 0x7ff8000000000000))
+(assert_return (invoke "copysign-and-demote" (f64.const nan))
+               (f32.const nan:0x7fc00000))
+(assert_return (invoke "copysign-and-promote" (f32.const nan))
+               (f64.const nan:0x7ff8000000000000))
+
+(assert_return (invoke "f32x4.demote_f64x2_zero"
+               (v128.const i64x2 0xfffefdfccccdcecf 0xfffefdfccccdcecf))
+               (v128.const f32x4 nan:0x7fc00000 nan:0x7fc00000 0 0))
+
+(assert_return (invoke "f64x2.promote_low_f32x4"
+               (v128.const i32x4 0xfffefdfc 0xfffefdfc 0 0))
+               (v128.const f64x2 nan:0x7ff8000000000000 nan:0x7ff8000000000000))