Merge pull request #340 from wasmx/bench-exec

bench: Fix execution pre-validation

circle.yml

@@ -214,7 +214,6 @@ jobs:
     executor: linux-clang-latest
     environment:
       BUILD_TYPE: Coverage
-      TESTS_FILTER: unittests
       CMAKE_OPTIONS: -DCMAKE_CXX_CLANG_TIDY=clang-tidy
     steps:
       - checkout
@@ -224,27 +223,17 @@ jobs:
           name: "Collect coverage data"
           working_directory: ~/build
           command: |
+            binaries='-object bin/fizzy-unittests -object bin/fizzy-spectests -object bin/fizzy-bench'
+
             mkdir coverage
             find -name '*.profraw'
             llvm-profdata merge *.profraw -o fizzy.profdata
 
-            llvm-cov report -use-color -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt \
-              -object bin/fizzy-unittests \
-              -object bin/fizzy-spectests
-            llvm-cov report -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt > coverage/report.txt \
-              -object bin/fizzy-unittests \
-              -object bin/fizzy-spectests
-
-            llvm-cov show -format=html -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt -region-coverage-lt=100 > coverage/missing.html \
-              -object bin/fizzy-unittests \
-              -object bin/fizzy-spectests
-            llvm-cov show -format=html -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt > coverage/full.html \
-              -object bin/fizzy-unittests \
-              -object bin/fizzy-spectests
-
-            llvm-cov export -instr-profile fizzy.profdata -format=lcov > fizzy.lcov \
-              -object bin/fizzy-unittests \
-              -object bin/fizzy-spectests
+            llvm-cov report -use-color -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt $binaries
+            llvm-cov report -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt $binaries > coverage/report.txt
+            llvm-cov show -format=html -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt -region-coverage-lt=100 $binaries > coverage/missing.html
+            llvm-cov show -format=html -instr-profile fizzy.profdata -Xdemangler llvm-cxxfilt $binaries > coverage/full.html
+            llvm-cov export -instr-profile fizzy.profdata -format=lcov $binaries > fizzy.lcov
             genhtml fizzy.lcov -o coverage -t Fizzy
       - store_artifacts:
           path: ~/build/coverage

test/CMakeLists.txt

@@ -19,6 +19,7 @@ add_subdirectory(bench)
 add_subdirectory(bench_internal)
 add_subdirectory(spectests)
 add_subdirectory(unittests)
+add_subdirectory(smoketests)
 
 if(FIZZY_FUZZING)
     add_subdirectory(fuzzer)

test/bench/bench.cpp

@@ -49,7 +49,7 @@ void benchmark_parse(
 
     // Pre-run for validation
     if (!engine->parse(wasm_binary))
-        return state.SkipWithError("Parsing failed");
+        state.SkipWithError("Parsing failed");
 
     const auto input_size = wasm_binary.size();
     auto num_bytes_parsed = uint64_t{0};
@@ -88,73 +88,95 @@ struct ExecutionBenchmarkCase
 
 /// Performs test run of the benchmark case and checks results produces by the engine
 /// against the expected values.
-void validate_benchmark_case(benchmark::State& state, fizzy::test::WasmEngine& engine,
+bool validate_benchmark_case(benchmark::State& state, fizzy::test::WasmEngine& engine,
     const ExecutionBenchmarkCase& benchmark_case)
 {
     if (!engine.instantiate(*benchmark_case.wasm_binary))
-        return state.SkipWithError("Instantiation failed");
+    {
+        state.SkipWithError("Instantiation failed");
+        return false;
+    }
 
     const auto func_ref = engine.find_function(benchmark_case.func_name, benchmark_case.func_sig);
     if (!func_ref)
     {
-        return state.SkipWithError(
-            ("Function \"" + benchmark_case.func_name + "\" not found").c_str());
+        state.SkipWithError(("Function \"" + benchmark_case.func_name + "\" not found").c_str());
+        return false;
     }
 
-    if (!benchmark_case.memory.empty())
+    if (!benchmark_case.memory.empty() && !engine.init_memory(benchmark_case.memory))
     {
-        if (!engine.init_memory(benchmark_case.memory))
-            return state.SkipWithError("Memory initialization failed");
+        state.SkipWithError("Memory initialization failed");
+        return false;
     }
 
     // Execute once and check results against expectations.
     const auto result = engine.execute(*func_ref, benchmark_case.func_args);
     if (result.trapped)
-        return state.SkipWithError("Trapped");
+    {
+        state.SkipWithError("Trapped");
+        return false;
+    }
 
     if (benchmark_case.expected_result)
     {
         if (!result.value)
         {
             const auto error_msg = "Missing result value, expected: " +
                                    std::to_string(*benchmark_case.expected_result);
-            return state.SkipWithError(error_msg.c_str());
+            state.SkipWithError(error_msg.c_str());
+            return false;
         }
         else if (*result.value != *benchmark_case.expected_result)
         {
             const auto error_msg = "Incorrect result value, expected: " +
                                    std::to_string(*benchmark_case.expected_result) +
                                    ", got: " + std::to_string(*result.value);
-            return state.SkipWithError(error_msg.c_str());
+            state.SkipWithError(error_msg.c_str());
+            return false;
         }
     }
     else if (result.value)
     {
-        return state.SkipWithError(
-            ("Unexpected result value: " + std::to_string(*result.value)).c_str());
+        state.SkipWithError(("Unexpected result value: " + std::to_string(*result.value)).c_str());
+        return false;
     }
     const auto memory = engine.get_memory();
     if (memory.size() < benchmark_case.expected_memory.size())
-        return state.SkipWithError("Result memory is shorter than expected");
+    {
+        state.SkipWithError("Result memory is shorter than expected");
+        return false;
+    }
 
     // Compare _beginning_ segment of the memory with expected.
     // Specifying expected full memory pages is impractical.
     if (!std::equal(std::begin(benchmark_case.expected_memory),
             std::end(benchmark_case.expected_memory), std::begin(memory)))
-        return state.SkipWithError("Incorrect result memory");
+    {
+        state.SkipWithError("Incorrect result memory");
+        return false;
+    }
+
+    return true;
 }
 
 void benchmark_execute(
     benchmark::State& state, EngineCreateFn create_fn, const ExecutionBenchmarkCase& benchmark_case)
 {
-    const auto engine = create_fn();
+    const auto has_memory = !benchmark_case.memory.empty();
 
-    validate_benchmark_case(state, *engine, benchmark_case);
+    const auto engine = create_fn();
+    const bool ok = validate_benchmark_case(state, *engine, benchmark_case);
 
-    const auto has_memory = !benchmark_case.memory.empty();
-    engine->instantiate(*benchmark_case.wasm_binary);
-    const auto func_ref = engine->find_function(benchmark_case.func_name, benchmark_case.func_sig);
+    std::optional<fizzy::test::WasmEngine::FuncRef> func_ref;
+    if (ok)
+    {
+        engine->instantiate(*benchmark_case.wasm_binary);
+        func_ref = engine->find_function(benchmark_case.func_name, benchmark_case.func_sig);
+    }
 
+    // The loop body will not be executed if validation error reported with state.SkipWithError().
+    // However, due to a bug in libbenchmark, the loop must be always reached.
     for ([[maybe_unused]] auto _ : state)
     {
         state.PauseTiming();

test/smoketests/CMakeLists.txt

@@ -0,0 +1,5 @@
+# Fizzy: A fast WebAssembly interpreter
+# Copyright 2020 The Fizzy Authors.
+# SPDX-License-Identifier: Apache-2.0
+
+add_subdirectory(benchmarks)

test/smoketests/benchmarks/CMakeLists.txt

@@ -0,0 +1,38 @@
+# Fizzy: A fast WebAssembly interpreter
+# Copyright 2020 The Fizzy Authors.
+# SPDX-License-Identifier: Apache-2.0
+
+add_test(
+    NAME fizzy/smoketests/bench/benchmarks
+    COMMAND fizzy-bench ${CMAKE_CURRENT_LIST_DIR} --benchmark_min_time=0.01
+)
+
+add_test(
+    NAME fizzy/smoketests/bench/cli-missing-dir-arg
+    COMMAND fizzy-bench
+)
+set_tests_properties(
+    fizzy/smoketests/bench/cli-missing-dir-arg
+    PROPERTIES
+    PASS_REGULAR_EXPRESSION "Missing DIR argument"
+)
+
+add_test(
+    NAME fizzy/smoketests/bench/cli-invalid-arg
+    COMMAND fizzy-bench ${CMAKE_CURRENT_LIST_DIR} --nonexisting_argument
+)
+set_tests_properties(
+    fizzy/smoketests/bench/cli-invalid-arg
+    PROPERTIES
+    PASS_REGULAR_EXPRESSION "error: unrecognized command-line flag: --nonexisting_argument"
+)
+
+
+# Dump coverage data to distinct files (otherwise file will be overwritten).
+set_tests_properties(
+    fizzy/smoketests/bench/benchmarks
+    fizzy/smoketests/bench/cli-missing-dir-arg
+    fizzy/smoketests/bench/cli-invalid-arg
+    PROPERTIES
+    ENVIRONMENT LLVM_PROFILE_FILE=${CMAKE_BINARY_DIR}/bench-%p.profraw
+)

test/smoketests/benchmarks/arith.inputs

@@ -0,0 +1,50 @@
+addition
+add
+ii:i
+1 2
+
+3
+
+division_by_zero
+div
+ii:i
+1 0
+
+0
+
+memory_initialization_failure
+add
+ii:i
+0 0
+fe
+0
+
+unexcepted_result
+div
+ii:i
+1 1
+
+
+
+expected_memory_shorter
+add
+ii:i
+0 0
+
+0
+00
+
+missing_function
+sub
+ii:i
+0 0
+
+0
+
+incorrect_result_value
+add
+ii:i
+2 2
+
+5
+

test/smoketests/benchmarks/arith.wat

@@ -0,0 +1,12 @@
+(module
+  (func (export "add") (param i32 i32) (result i32)
+    local.get 0
+    local.get 1
+    i32.add
+  )
+  (func (export "div") (param i32 i32) (result i32)
+    local.get 0
+    local.get 1
+    i32.div_u
+  )
+)

test/smoketests/benchmarks/invalid.inputs

@@ -0,0 +1,7 @@
+add
+add
+ii:i
+1 2
+
+
+

test/smoketests/benchmarks/invalid.wat

@@ -0,0 +1,6 @@
+(module
+  (func (export "add") (param i32 i32) (result i32)
+    local.get 0
+    i32.add
+  )
+)

test/smoketests/benchmarks/malformed/malformed.inputs

@@ -0,0 +1,7 @@
+fake
+fake_func
+:
+
+
+
+

test/smoketests/benchmarks/memory.inputs

@@ -0,0 +1,24 @@
+increase_value
+inc
+i:
+0
+ff000000
+
+00010000
+
+
+wrong_expected_memory
+inc
+i:
+0
+ff000000
+
+ffffffff
+
+missing_result_value
+inc
+i:
+0
+
+0
+

test/smoketests/benchmarks/memory.wat

@@ -0,0 +1,11 @@
+(module
+  (memory 1 1)
+  (func (export "inc") (param i32)
+    local.get 0
+    local.get 0
+    i32.load
+    i32.const 1
+    i32.add
+    i32.store
+  )
+)

test/smoketests/benchmarks/missing_imported_function.inputs

@@ -0,0 +1,7 @@
+main
+main
+i:i
+0
+
+0
+

test/smoketests/benchmarks/missing_imported_function.wat

@@ -0,0 +1,7 @@
+(module
+  (func $imported (import "env" "imported") (param i32) (result i32))
+  (func (export "main") (param i32) (result i32)
+    local.get 0
+    call $imported
+  )
+)

test/utils/fizzy_engine.cpp

@@ -135,7 +135,6 @@ WasmEngine::Result FizzyEngine::execute(
 {
     const auto [trapped, result_stack] =
         fizzy::execute(*m_instance, static_cast<uint32_t>(func_ref), args);
-    assert(result_stack.size() <= 1);
     return {trapped, !result_stack.empty() ? result_stack.back() : std::optional<uint64_t>{}};
 }
 }  // namespace fizzy::test