Merge #1009

1009: Fix LLVM crash on `return` with a float on the stack and a crash with gas metering. r=nlewycky a=nlewycky # Description This PR fixes some matters of testing. The return+float bug is caught by the LLVM verifier, which was accidentally unconditionally disabled even when running "make test check". Add a new cargo feature named "test" which is enabled by all the test crates. When the "test" feature is passed to the llvm-backend, run the LLVM verifier. Add a new crate `llvm-backend-test` which has one test so far, to ensure that this bug with the return+float does not crash LLVM. Include this new crate in runs of `make llvm`. Fix LLVM crash on `return` when there is a float with pending NaN canonicalization on the stack. Now that we run the verifier in testing, we discover another bug where `internal_field()` incorrectly labels a GEP with a TBAA label instead of the intended load. Fix this by labeling the correct instruction. No new test is introduced since this is already caught with `make bench-llvm`. # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Nick Lewycky <[email protected]>
wasmerio · Nov 23, 2019 · 2f394c9 · 2f394c9
2 parents 8f50dab + 91671d5
commit 2f394c9
Show file tree

Hide file tree

Showing 15 changed files with 72 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## **[Unreleased]**
 
+- [#1009](https://github.com/wasmerio/wasmer/pull/1009) Enable LLVM verifier for all tests, add new llvm-backend-tests crate.
+
 ## 0.11.0 - 2019-11-22
 
 - [#713](https://github.com/wasmerio/wasmer/pull/713) Add AArch64 support for singlepass.

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -50,6 +50,7 @@ members = [
     "lib/win-exception-handler",
     "lib/runtime-c-api",
     "lib/llvm-backend",
+    "lib/llvm-backend-tests",
     "lib/wasi",
     "lib/middleware-common",
     "lib/kernel-loader",

diff --git a/Makefile b/Makefile
@@ -97,6 +97,7 @@ cranelift: spectests-cranelift emtests-cranelift middleware-cranelift wasitests-
 
 llvm: spectests-llvm emtests-llvm wasitests-llvm
 	cargo test -p wasmer-llvm-backend --release
+	cargo test -p wasmer-llvm-backend-tests --release
 	cargo test -p wasmer-runtime-core-tests --release --no-default-features --features backend-llvm
 
 

diff --git a/lib/emscripten-tests/Cargo.toml b/lib/emscripten-tests/Cargo.toml
@@ -12,7 +12,7 @@ build = "build/mod.rs"
 wasmer-emscripten = { path = "../emscripten", version = "0.11.0" }
 wasmer-runtime = { path = "../runtime", version = "0.11.0", default-features = false }
 wasmer-clif-backend = { path = "../clif-backend", version = "0.11.0", optional = true}
-wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", optional = true }
+wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", optional = true, features = ["test"] }
 wasmer-singlepass-backend = { path = "../singlepass-backend", version = "0.11.0", optional = true }
 
 [dev-dependencies]

diff --git a/lib/llvm-backend-tests/Cargo.toml b/lib/llvm-backend-tests/Cargo.toml
@@ -0,0 +1,15 @@
+[package]
+name = "wasmer-llvm-backend-tests"
+version = "0.10.2"
+authors = ["Nick Lewycky <[email protected]>"]
+edition = "2018"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+wabt = "0.9.1"
+wasmer-runtime-core = { path = "../runtime-core", version = "0.11.0" }
+wasmer-runtime = { path = "../runtime", version = "0.11.0" }
+wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", features = ["test"] }
+
+[features]
diff --git a/lib/llvm-backend-tests/src/lib.rs b/lib/llvm-backend-tests/src/lib.rs
@@ -0,0 +1,7 @@
+pub use wabt::wat2wasm;
+use wasmer_llvm_backend::LLVMCompiler;
+use wasmer_runtime_core::backend::Compiler;
+
+pub fn get_compiler() -> impl Compiler {
+    LLVMCompiler::new()
+}
diff --git a/lib/llvm-backend-tests/tests/compile.rs b/lib/llvm-backend-tests/tests/compile.rs
@@ -0,0 +1,22 @@
+use wasmer_llvm_backend_tests::{get_compiler, wat2wasm};
+use wasmer_runtime::imports;
+use wasmer_runtime_core::compile_with;
+
+#[test]
+fn crash_return_with_float_on_stack() {
+    const MODULE: &str = r#"
+(module
+  (type (;0;) (func))
+  (type (;1;) (func (param f64) (result f64)))
+  (func $_start (type 0))
+  (func $fmod (type 1) (param f64) (result f64)
+    local.get 0
+    f64.const 0x0p+0 (;=0;)
+    f64.mul
+    return)
+)
+"#;
+    let wasm_binary = wat2wasm(MODULE.as_bytes()).expect("WAST not valid or malformed");
+    let module = compile_with(&wasm_binary, &get_compiler()).unwrap();
+    let instance = module.instantiate(&imports! {}).unwrap();
+}
diff --git a/lib/llvm-backend/Cargo.toml b/lib/llvm-backend/Cargo.toml
@@ -41,3 +41,4 @@ wabt = "0.9.1"
 
 [features]
 debug = ["wasmer-runtime-core/debug"]
+test = []
diff --git a/lib/llvm-backend/src/code.rs b/lib/llvm-backend/src/code.rs
@@ -1412,21 +1412,21 @@ impl FunctionCodeGenerator<CodegenError> for LLVMFunctionCodeGenerator {
                 }
             }
             Operator::Return => {
-                let frame = state.outermost_frame()?;
                 let current_block = builder.get_insert_block().ok_or(BinaryReaderError {
                     message: "not currently in a block",
                     offset: -1isize as usize,
                 })?;
 
-                builder.build_unconditional_branch(frame.br_dest());
-
                 let frame = state.outermost_frame()?;
                 for phi in frame.phis().to_vec().iter() {
                     let (arg, info) = state.pop1_extra()?;
                     let arg = apply_pending_canonicalization(builder, intrinsics, arg, info);
                     phi.add_incoming(&[(&arg, &current_block)]);
                 }
 
+                let frame = state.outermost_frame()?;
+                builder.build_unconditional_branch(frame.br_dest());
+
                 state.reachable = false;
             }
 
@@ -8286,9 +8286,10 @@ impl ModuleCodeGenerator<LLVMFunctionCodeGenerator, LLVMBackend, CodegenError>
         }
 
         let pass_manager = PassManager::create(());
-        if cfg!(test) {
-            pass_manager.add_verifier_pass();
-        }
+
+        #[cfg(feature = "test")]
+        pass_manager.add_verifier_pass();
+
         pass_manager.add_type_based_alias_analysis_pass();
         pass_manager.add_lower_expect_intrinsic_pass();
         pass_manager.add_scalar_repl_aggregates_pass();

diff --git a/lib/llvm-backend/src/intrinsics.rs b/lib/llvm-backend/src/intrinsics.rs
@@ -1132,7 +1132,7 @@ impl<'a> CtxType<'a> {
             module.clone(),
             intrinsics,
             "context_field_ptr_to_internals",
-            local_internals_ptr_ptr.as_instruction_value().unwrap(),
+            local_internals_ptr.as_instruction_value().unwrap(),
             None,
         );
         unsafe {

diff --git a/lib/middleware-common-tests/Cargo.toml b/lib/middleware-common-tests/Cargo.toml
@@ -11,7 +11,7 @@ publish = false
 wasmer-runtime-core = { path = "../runtime-core", version = "0.11.0" }
 wasmer-middleware-common = { path = "../middleware-common", version = "0.11.0" }
 wasmer-clif-backend = { path = "../clif-backend", version = "0.11.0" }
-wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", optional = true }
+wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", features = ["test"], optional = true }
 wasmer-singlepass-backend = { path = "../singlepass-backend", version = "0.11.0", optional = true }
 
 [features]

diff --git a/lib/runtime-core-tests/Cargo.toml b/lib/runtime-core-tests/Cargo.toml
@@ -12,7 +12,7 @@ wabt = "0.9.1"
 wasmer-runtime-core = { path = "../runtime-core", version = "0.11.0" }
 wasmer-clif-backend = { path = "../clif-backend", version = "0.11.0", optional = true }
 wasmer-singlepass-backend = { path = "../singlepass-backend", version = "0.11.0", optional = true }
-wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", optional = true }
+wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", features = ["test"], optional = true }
 
 [features]
 default = ["backend-cranelift"]

diff --git a/lib/spectests/Cargo.toml b/lib/spectests/Cargo.toml
@@ -11,7 +11,7 @@ edition = "2018"
 glob = "0.3"
 wasmer-runtime = { path = "../runtime", version = "0.11.0", default-features = false}
 wasmer-clif-backend = { path = "../clif-backend", version = "0.11.0", optional = true}
-wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", optional = true }
+wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", features = ["test"], optional = true }
 wasmer-singlepass-backend = { path = "../singlepass-backend", version = "0.11.0", optional = true }
 
 [build-dependencies]

diff --git a/lib/wasi-tests/Cargo.toml b/lib/wasi-tests/Cargo.toml
@@ -15,7 +15,7 @@ wasmer-wasi = { path = "../wasi", version = "0.11.0" }
 # hack to get tests to work
 wasmer-clif-backend = { path = "../clif-backend", version = "0.11.0", optional = true}
 wasmer-singlepass-backend = { path = "../singlepass-backend", version = "0.11.0", optional = true }
-wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", optional = true }
+wasmer-llvm-backend = { path = "../llvm-backend", version = "0.11.0", features = ["test"], optional = true }
 
 [build-dependencies]
 glob = "0.3"
Original file line number	Diff line number	Diff line change
Expand Up		@@ -41,3 +41,4 @@ wabt = "0.9.1"

		[features]
		debug = ["wasmer-runtime-core/debug"]
		test = []