Merge #1731 #1732

1731: For store instructions, always load first. r=nlewycky a=nlewycky

Sometimes the store may be partially-successful before trapping, for instance if it is partway in valid memory and partway into the guard page.

Use a load instruction before the store to ensure that all the memory is addressable. The loaded value is discarded.

NB. We don't apply this to atomics. It's not clear whether atomic stores can be half-committed.

Fixes align.wast and memory_trap.wast on aarch64.


1732: Make wasi tests use the engine we're testing instead of always engine-jit. r=nlewycky a=nlewycky



Co-authored-by: Nick Lewycky <[email protected]>

lib/compiler-llvm/src/translator/code.rs

@@ -6286,6 +6286,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     4,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let store = self.builder.build_store(effective_address, value);
                 self.annotate_user_memaccess(memory_index, memarg, 1, store)?;
             }
@@ -6300,6 +6307,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     8,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let store = self.builder.build_store(effective_address, value);
                 self.annotate_user_memaccess(memory_index, memarg, 1, store)?;
             }
@@ -6315,6 +6329,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     4,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let store = self.builder.build_store(effective_address, v);
                 self.annotate_user_memaccess(memory_index, memarg, 1, store)?;
             }
@@ -6330,6 +6351,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     8,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let store = self.builder.build_store(effective_address, v);
                 self.annotate_user_memaccess(memory_index, memarg, 1, store)?;
             }
@@ -6345,6 +6373,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     16,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let store = self.builder.build_store(effective_address, v);
                 self.annotate_user_memaccess(memory_index, memarg, 1, store)?;
             }
@@ -6603,6 +6638,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     1,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let narrow_value =
                     self.builder
                         .build_int_truncate(value, self.intrinsics.i8_ty, "");
@@ -6620,6 +6662,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     2,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let narrow_value =
                     self.builder
                         .build_int_truncate(value, self.intrinsics.i16_ty, "");
@@ -6637,6 +6686,13 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> {
                     offset,
                     4,
                 )?;
+                let dead_load = self.builder.build_load(effective_address, "");
+                self.annotate_user_memaccess(
+                    memory_index,
+                    memarg,
+                    1,
+                    dead_load.as_instruction_value().unwrap(),
+                )?;
                 let narrow_value =
                     self.builder
                         .build_int_truncate(value, self.intrinsics.i32_ty, "");

tests/compilers/imports.rs

@@ -43,7 +43,7 @@ fn get_module(store: &Store) -> Result<Module> {
 
 #[test]
 fn dynamic_function() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let module = get_module(&store)?;
     static HITS: AtomicUsize = AtomicUsize::new(0);
     Instance::new(
@@ -83,7 +83,7 @@ fn dynamic_function() -> Result<()> {
 
 #[test]
 fn dynamic_function_with_env() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let module = get_module(&store)?;
 
     let env: Arc<AtomicUsize> = Arc::new(AtomicUsize::new(0));
@@ -124,7 +124,7 @@ fn dynamic_function_with_env() -> Result<()> {
 
 #[test]
 fn static_function() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let module = get_module(&store)?;
 
     static HITS: AtomicUsize = AtomicUsize::new(0);
@@ -162,7 +162,7 @@ fn static_function() -> Result<()> {
 
 #[test]
 fn static_function_with_results() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let module = get_module(&store)?;
 
     static HITS: AtomicUsize = AtomicUsize::new(0);
@@ -200,7 +200,7 @@ fn static_function_with_results() -> Result<()> {
 
 #[test]
 fn static_function_with_env() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let module = get_module(&store)?;
 
     let env: Arc<AtomicUsize> = Arc::new(AtomicUsize::new(0));
@@ -238,7 +238,7 @@ fn static_function_with_env() -> Result<()> {
 
 #[test]
 fn static_function_that_fails() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (import "host" "0" (func))
 

tests/compilers/multi_value_imports.rs

@@ -39,7 +39,7 @@ macro_rules! mvr_test {
             #[test]
             #[cfg_attr(any(feature = "test-cranelift", feature="test-singlepass"), ignore)]
             fn native() -> anyhow::Result<()> {
-                let store = get_store();
+                let store = get_store(false);
                 let module = get_module(&store)?;
                 let instance = wasmer::Instance::new(
                     &module,
@@ -65,7 +65,7 @@ macro_rules! mvr_test {
             #[test]
             #[cfg_attr(feature="test-singlepass", ignore)]
             fn dynamic() -> anyhow::Result<()> {
-                let store = get_store();
+                let store = get_store(false);
                 let module = get_module(&store)?;
                 let callback_fn = wasmer::Function::new(&store, &wasmer::FunctionType::new(vec![wasmer::ValType::I32], vec![ $( <$result_type>::expected_valtype() ),* ]), dynamic_callback_fn);
                 let instance = wasmer::Instance::new(

tests/compilers/native_functions.rs

@@ -8,7 +8,7 @@ use wasmer::*;
 
 #[test]
 fn native_function_works_for_wasm() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"(module
         (func $multiply (import "env" "multiply") (param i32 i32) (result i32))
         (func (export "add") (param i32 i32) (result i32)
@@ -52,7 +52,7 @@ fn native_function_works_for_wasm() -> Result<()> {
 
 #[test]
 fn static_host_function_without_env() -> anyhow::Result<()> {
-    let store = get_store();
+    let store = get_store(false);
 
     fn f(a: i32, b: i64, c: f32, d: f64) -> (f64, f32, i64, i32) {
         (d * 4.0, c * 3.0, b * 2, a * 1)
@@ -83,7 +83,7 @@ fn static_host_function_without_env() -> anyhow::Result<()> {
 
 #[test]
 fn static_host_function_with_env() -> anyhow::Result<()> {
-    let store = get_store();
+    let store = get_store(false);
 
     fn f(env: &mut Env, a: i32, b: i64, c: f32, d: f64) -> (f64, f32, i64, i32) {
         assert_eq!(*env.0.borrow(), 100);
@@ -143,7 +143,7 @@ fn static_host_function_with_env() -> anyhow::Result<()> {
 
 #[test]
 fn dynamic_host_function_without_env() -> anyhow::Result<()> {
-    let store = get_store();
+    let store = get_store(false);
 
     let f = Function::new(
         &store,
@@ -170,7 +170,7 @@ fn dynamic_host_function_without_env() -> anyhow::Result<()> {
 
 #[test]
 fn dynamic_host_function_with_env() -> anyhow::Result<()> {
-    let store = get_store();
+    let store = get_store(false);
 
     #[derive(Clone)]
     struct Env(Rc<RefCell<i32>>);

tests/compilers/serialize.rs

@@ -4,7 +4,7 @@ use wasmer::*;
 
 #[test]
 fn test_serialize() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module
         (func $hello (import "" "hello"))
@@ -20,7 +20,7 @@ fn test_serialize() -> Result<()> {
 
 #[test]
 fn test_deserialize() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module $name
             (import "host" "sum_part" (func (param i32 i64 i32 f32 f64) (result i64)))

tests/compilers/traps.rs

@@ -5,7 +5,7 @@ use wasmer::*;
 
 #[test]
 fn test_trap_return() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module
         (func $hello (import "" "hello"))
@@ -47,7 +47,7 @@ fn test_trap_return() -> Result<()> {
     ignore
 )]
 fn test_trap_trace() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module $hello_mod
             (func (export "run") (call $hello))
@@ -83,7 +83,7 @@ fn test_trap_trace() -> Result<()> {
 
 #[test]
 fn test_trap_trace_cb() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module $hello_mod
             (import "" "throw" (func $throw))
@@ -134,7 +134,7 @@ fn test_trap_trace_cb() -> Result<()> {
     ignore
 )]
 fn test_trap_stack_overflow() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module $rec_mod
             (func $run (export "run") (call $run))
@@ -173,7 +173,7 @@ fn test_trap_stack_overflow() -> Result<()> {
     ignore
 )]
 fn trap_display_pretty() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module $m
             (func $die unreachable)
@@ -214,7 +214,7 @@ RuntimeError: unreachable
     ignore
 )]
 fn trap_display_multi_module() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module $a
             (func $die unreachable)
@@ -266,7 +266,7 @@ RuntimeError: unreachable
 
 #[test]
 fn trap_start_function_import() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let binary = r#"
         (module $a
             (import "" "" (func $foo))
@@ -299,7 +299,7 @@ fn trap_start_function_import() -> Result<()> {
 
 #[test]
 fn rust_panic_import() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let binary = r#"
         (module $a
             (import "" "foo" (func $foo))
@@ -344,7 +344,7 @@ fn rust_panic_import() -> Result<()> {
 
 #[test]
 fn rust_panic_start_function() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let binary = r#"
         (module $a
             (import "" "" (func $foo))
@@ -389,7 +389,7 @@ fn rust_panic_start_function() -> Result<()> {
 
 #[test]
 fn mismatched_arguments() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let binary = r#"
         (module $a
             (func (export "foo") (param i32))
@@ -426,7 +426,7 @@ fn mismatched_arguments() -> Result<()> {
     ignore
 )]
 fn call_signature_mismatch() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let binary = r#"
         (module $a
             (func $foo
@@ -465,7 +465,7 @@ RuntimeError: indirect call type mismatch
     ignore
 )]
 fn start_trap_pretty() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let wat = r#"
         (module $m
             (func $die unreachable)
@@ -497,7 +497,7 @@ RuntimeError: unreachable
 #[test]
 #[cfg_attr(feature = "test-native", ignore)]
 fn present_after_module_drop() -> Result<()> {
-    let store = get_store();
+    let store = get_store(false);
     let module = Module::new(&store, r#"(func (export "foo") unreachable)"#)?;
     let instance = Instance::new(&module, &imports! {})?;
     let func: Function = instance.exports.get_function("foo")?.clone();

tests/compilers/utils.rs

@@ -36,18 +36,18 @@ pub fn get_compiler(canonicalize_nans: bool) -> impl CompilerConfig {
 }
 
 #[cfg(feature = "test-jit")]
-pub fn get_engine() -> impl Engine {
-    let compiler_config = get_compiler(false);
+pub fn get_engine(canonicalize_nans: bool) -> impl Engine {
+    let compiler_config = get_compiler(canonicalize_nans);
     JIT::new(&compiler_config).engine()
 }
 #[cfg(feature = "test-native")]
-pub fn get_engine() -> impl Engine {
-    let mut compiler_config = get_compiler(false);
+pub fn get_engine(canonicalize_nans: bool) -> impl Engine {
+    let mut compiler_config = get_compiler(canonicalize_nans);
     Native::new(&mut compiler_config).engine()
 }
 
-pub fn get_store() -> Store {
-    Store::new(&get_engine())
+pub fn get_store(canonicalize_nans: bool) -> Store {
+    Store::new(&get_engine(canonicalize_nans))
 }
 
 pub fn get_store_with_middlewares<I: Iterator<Item = Arc<dyn FunctionMiddlewareGenerator>>>(
@@ -67,7 +67,6 @@ pub fn get_store_with_middlewares<I: Iterator<Item = Arc<dyn FunctionMiddlewareG
 #[cfg(feature = "test-jit")]
 pub fn get_headless_store() -> Store {
     Store::new(&JIT::headless().engine())
-    // Store::new(&Native::headless().engine())
 }
 
 #[cfg(feature = "test-native")]