New version of execution_result

This re-implements the execution_result struct. Now it has three valid states: value, void and trap. The struct is simplified, close to POD type, std::vector is not used any more. Co-authored-by: Paweł Bylica <chfast@gmail.com>
wasmx · Jul 16, 2020 · c8e22a1 · c8e22a1
1 parent 154a76b
commit c8e22a1
Show file tree

Hide file tree

Showing 11 changed files with 64 additions and 68 deletions.
diff --git a/lib/fizzy/execute.cpp b/lib/fizzy/execute.cpp
@@ -281,11 +281,11 @@ bool invoke_function(
 
     const auto num_outputs = func_type.outputs.size();
     // NOTE: we can assume these two from validation
-    assert(ret.stack.size() == num_outputs);
     assert(num_outputs <= 1);
+    assert(ret.has_value == (num_outputs == 1));
     // Push back the result
     if (num_outputs != 0)
-        stack.push(ret.stack[0]);
+        stack.push(ret.value);
 
     return true;
 }
@@ -600,7 +600,7 @@ execution_result execute(Instance& instance, FuncIdx func_idx, span<const uint64
 {
     assert(depth >= 0);
     if (depth > CallStackLimit)
-        return {true, {}};
+        return Trap;
 
     if (func_idx < instance.imported_functions.size())
         return instance.imported_functions[func_idx].function(instance, args, depth);
@@ -1458,7 +1458,10 @@ execution_result execute(Instance& instance, FuncIdx func_idx, span<const uint64
 end:
     // WebAssembly 1.0 allows at most one return variable.
     assert(trap || (pc == &code.instructions[code.instructions.size()] && stack.size() <= 1));
-    return {trap, {stack.rbegin(), stack.rend()}};
+    if (trap)
+        return Trap;
+
+    return stack.size() != 0 ? stack.pop() : Void;
 }
 
 std::vector<ExternalFunction> resolve_imported_functions(

diff --git a/lib/fizzy/execute.hpp b/lib/fizzy/execute.hpp
@@ -17,13 +17,21 @@ namespace fizzy
 // The result of an execution.
 struct execution_result
 {
-    // true if execution resulted in a trap
-    bool trapped;
-    // the resulting stack (e.g. return values)
-    // NOTE: this can be either 0 or 1 items
-    std::vector<uint64_t> stack;
+    const bool trapped = false;
+    const bool has_value = false;
+    const uint64_t value = 0;
+
+    /// Constructs result with a value.
+    constexpr execution_result(uint64_t _value) noexcept : has_value{true}, value{_value} {}
+
+    /// Constructs result in "void" or "trap" state depending on the success flag.
+    /// Prefer using Void and Trap constants instead.
+    constexpr explicit execution_result(bool success) noexcept : trapped{!success} {}
 };
 
+constexpr execution_result Void{true};
+constexpr execution_result Trap{false};
+
 struct Instance;
 
 struct ExternalFunction

diff --git a/test/spectests/spectests.cpp b/test/spectests/spectests.cpp
@@ -210,20 +210,14 @@ class test_runner
                     const auto& expected = cmd.at("expected");
                     if (expected.empty())
                     {
-                        if (result->stack.empty())
+                        if (!result->has_value)
                             pass();
                         else
                             fail("Unexpected returned value.");
                         continue;
                     }
 
-                    if (result->stack.size() != 1)
-                    {
-                        fail("More than 1 value returned.");
-                        continue;
-                    }
-
-                    if (!check_result(result->stack[0], expected.at(0)))
+                    if (!check_result(result->value, expected.at(0)))
                         continue;
 
                     pass();

diff --git a/test/unittests/api_test.cpp b/test/unittests/api_test.cpp
@@ -16,14 +16,12 @@ namespace
 {
 auto function_returning_value(uint64_t value) noexcept
 {
-    return [value](Instance&, span<const uint64_t>, int) {
-        return execution_result{false, {value}};
-    };
+    return [value](Instance&, span<const uint64_t>, int) { return value; };
 }
 
 execution_result function_returning_void(Instance&, span<const uint64_t>, int) noexcept
 {
-    return {};
+    return Void;
 }
 }  // namespace
 
@@ -222,7 +220,7 @@ TEST(api, find_exported_function)
         "0061736d010000000105016000017f021001087370656374657374036261720000040401700000050401010102"
         "0606017f0041000b07170403666f6f000001670300037461620100036d656d0200");
 
-    auto bar = [](Instance&, span<const uint64_t>, int) { return execution_result{false, {42}}; };
+    auto bar = [](Instance&, span<const uint64_t>, int) { return 42; };
     const auto bar_type = FuncType{{}, {ValType::i32}};
 
     auto instance_reexported_function =

diff --git a/test/unittests/execute_call_test.cpp b/test/unittests/execute_call_test.cpp
@@ -147,11 +147,11 @@ TEST(execute_call, call_indirect_imported_table)
 
     const Module module = parse(bin);
 
-    auto f1 = [](Instance&, span<const uint64_t>, int) { return execution_result{false, {1}}; };
-    auto f2 = [](Instance&, span<const uint64_t>, int) { return execution_result{false, {2}}; };
-    auto f3 = [](Instance&, span<const uint64_t>, int) { return execution_result{false, {3}}; };
-    auto f4 = [](Instance&, span<const uint64_t>, int) { return execution_result{false, {4}}; };
-    auto f5 = [](Instance&, span<const uint64_t>, int) { return execution_result{true, {}}; };
+    auto f1 = [](Instance&, span<const uint64_t>, int) { return 1; };
+    auto f2 = [](Instance&, span<const uint64_t>, int) { return 2; };
+    auto f3 = [](Instance&, span<const uint64_t>, int) { return 3; };
+    auto f4 = [](Instance&, span<const uint64_t>, int) { return 4; };
+    auto f5 = [](Instance&, span<const uint64_t>, int) { return Trap; };
 
     auto out_i32 = FuncType{{}, {ValType::i32}};
     auto out_i64 = FuncType{{}, {ValType::i64}};
@@ -219,7 +219,7 @@ TEST(execute_call, imported_function_call)
     const auto module = parse(wasm);
 
     constexpr auto host_foo = [](Instance&, span<const uint64_t>, int) -> execution_result {
-        return {false, {42}};
+        return 42;
     };
     const auto host_foo_type = module.typesec[0];
 
@@ -246,7 +246,7 @@ TEST(execute_call, imported_function_call_with_arguments)
     const auto module = parse(wasm);
 
     auto host_foo = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] * 2}};
+        return args[0] * 2;
     };
     const auto host_foo_type = module.typesec[0];
 
@@ -290,10 +290,10 @@ TEST(execute_call, imported_functions_call_indirect)
     ASSERT_EQ(module.codesec.size(), 2);
 
     constexpr auto sqr = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] * args[0]}};
+        return args[0] * args[0];
     };
     constexpr auto isqrt = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {(11 + args[0] / 11) / 2}};
+        return (11 + args[0] / 11) / 2;
     };
 
     auto instance = instantiate(module, {{sqr, module.typesec[0]}, {isqrt, module.typesec[0]}});

diff --git a/test/unittests/execute_control_test.cpp b/test/unittests/execute_control_test.cpp
@@ -665,7 +665,7 @@ TEST(execute_control, br_1_out_of_function_and_imported_function)
         "0a0d010b00034041010c010b41000b");
 
     constexpr auto fake_imported_function = [](Instance&, span<const uint64_t>,
-                                                int) noexcept -> execution_result { return {}; };
+                                                int) noexcept -> execution_result { return Void; };
 
     const auto module = parse(bin);
     auto instance = instantiate(module, {{fake_imported_function, module.typesec[0]}});

diff --git a/test/unittests/execute_test.cpp b/test/unittests/execute_test.cpp
@@ -602,7 +602,7 @@ TEST(execute, imported_function)
     ASSERT_EQ(module.typesec.size(), 1);
 
     constexpr auto host_foo = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] + args[1]}};
+        return args[0] + args[1];
     };
 
     auto instance = instantiate(module, {{host_foo, module.typesec[0]}});
@@ -622,10 +622,10 @@ TEST(execute, imported_two_functions)
     ASSERT_EQ(module.typesec.size(), 1);
 
     constexpr auto host_foo1 = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] + args[1]}};
+        return args[0] + args[1];
     };
     constexpr auto host_foo2 = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] * args[1]}};
+        return args[0] * args[1];
     };
 
     auto instance =
@@ -649,10 +649,10 @@ TEST(execute, imported_functions_and_regular_one)
         "000a0901070041aa80a8010b");
 
     constexpr auto host_foo1 = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] + args[1]}};
+        return args[0] + args[1];
     };
     constexpr auto host_foo2 = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] * args[0]}};
+        return args[0] * args[0];
     };
 
     const auto module = parse(wasm);
@@ -664,7 +664,7 @@ TEST(execute, imported_functions_and_regular_one)
 
     // check correct number of arguments is passed to host
     constexpr auto count_args = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args.size()}};
+        return args.size();
     };
 
     auto instance_counter =
@@ -689,10 +689,10 @@ TEST(execute, imported_two_functions_different_type)
         "0001030201010a0901070042aa80a8010b");
 
     constexpr auto host_foo1 = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] + args[1]}};
+        return args[0] + args[1];
     };
     constexpr auto host_foo2 = [](Instance&, span<const uint64_t> args, int) -> execution_result {
-        return {false, {args[0] * args[0]}};
+        return args[0] * args[0];
     };
 
     const auto module = parse(wasm);
@@ -713,7 +713,7 @@ TEST(execute, imported_function_traps)
     const auto wasm = from_hex("0061736d0100000001070160027f7f017f020b01036d6f6403666f6f0000");
 
     constexpr auto host_foo = [](Instance&, span<const uint64_t>, int) -> execution_result {
-        return {true, {}};
+        return Trap;
     };
 
     const auto module = parse(wasm);

diff --git a/test/unittests/instantiate_test.cpp b/test/unittests/instantiate_test.cpp
@@ -18,7 +18,8 @@ uint64_t call_table_func(Instance& instance, size_t idx)
 {
     const auto& elem = (*instance.table)[idx];
     const auto res = elem->function(instance, {}, 0);
-    return res.stack.front();
+    EXPECT_TRUE(res.has_value);
+    return res.value;
 }
 }  // namespace
 
@@ -30,9 +31,7 @@ TEST(instantiate, imported_functions)
     const auto bin = from_hex("0061736d0100000001060160017f017f020b01036d6f6403666f6f0000");
     const auto module = parse(bin);
 
-    auto host_foo = [](Instance&, span<const uint64_t>, int) -> execution_result {
-        return {true, {}};
-    };
+    auto host_foo = [](Instance&, span<const uint64_t>, int) -> execution_result { return Trap; };
     auto instance = instantiate(module, {{host_foo, module.typesec[0]}});
 
     ASSERT_EQ(instance->imported_functions.size(), 1);
@@ -53,12 +52,8 @@ TEST(instantiate, imported_functions_multiple)
         "0061736d0100000001090260017f017f600000021702036d6f6404666f6f310000036d6f6404666f6f320001");
     const auto module = parse(bin);
 
-    auto host_foo1 = [](Instance&, span<const uint64_t>, int) -> execution_result {
-        return {true, {0}};
-    };
-    auto host_foo2 = [](Instance&, span<const uint64_t>, int) -> execution_result {
-        return {true, {}};
-    };
+    auto host_foo1 = [](Instance&, span<const uint64_t>, int) -> execution_result { return Trap; };
+    auto host_foo2 = [](Instance&, span<const uint64_t>, int) -> execution_result { return Trap; };
     auto instance =
         instantiate(module, {{host_foo1, module.typesec[0]}, {host_foo2, module.typesec[1]}});
 
@@ -93,9 +88,7 @@ TEST(instantiate, imported_function_wrong_type)
     const auto bin = from_hex("0061736d0100000001060160017f017f020b01036d6f6403666f6f0000");
     const auto module = parse(bin);
 
-    auto host_foo = [](Instance&, span<const uint64_t>, int) -> execution_result {
-        return {true, {}};
-    };
+    auto host_foo = [](Instance&, span<const uint64_t>, int) -> execution_result { return Trap; };
     const auto host_foo_type = FuncType{{}, {}};
 
     EXPECT_THROW_MESSAGE(instantiate(module, {{host_foo, host_foo_type}}), instantiate_error,
@@ -574,7 +567,7 @@ TEST(instantiate, element_section_fills_imported_table)
         "0061736d010000000105016000017f020b01016d037461620170000403050400000000090f020041010b020001"
         "0041020b0202030a1504040041010b040041020b040041030b040041040b");
 
-    auto f0 = [](Instance&, span<const uint64_t>, int) { return execution_result{false, {0}}; };
+    auto f0 = [](Instance&, span<const uint64_t>, int) { return 0; };
 
     table_elements table(4);
     table[0] = ExternalFunction{f0, FuncType{{}, {ValType::i32}}};
@@ -602,7 +595,7 @@ TEST(instantiate, element_section_out_of_bounds_doesnt_change_imported_table)
         "0b0200000a0601040041010b");
     Module module = parse(bin);
 
-    auto f0 = [](Instance&, span<const uint64_t>, int) { return execution_result{false, {0}}; };
+    auto f0 = [](Instance&, span<const uint64_t>, int) { return 0; };
 
     table_elements table(3);
     table[0] = ExternalFunction{f0, FuncType{{}, {ValType::i32}}};

diff --git a/test/utils/asserts.cpp b/test/utils/asserts.cpp
@@ -13,12 +13,8 @@ std::ostream& operator<<(std::ostream& os, execution_result result)
         return os << "trapped";
 
     os << "result(";
-    std::string_view separator;
-    for (const auto& x : result.stack)
-    {
-        os << separator << x;
-        separator = ", ";
-    }
+    if (result.has_value)
+        os << result.value;
     os << ")";
     return os;
 }

diff --git a/test/utils/asserts.hpp b/test/utils/asserts.hpp
@@ -15,12 +15,12 @@ MATCHER(Traps, "")  // NOLINT(readability-redundant-string-init)
 
 MATCHER(Result, "empty result")
 {
-    return !arg.trapped && arg.stack.size() == 0;
+    return !arg.trapped && !arg.has_value;
 }
 
 MATCHER_P(Result, value, "")  // NOLINT(readability-redundant-string-init)
 {
-    return !arg.trapped && arg.stack.size() == 1 && arg.stack[0] == uint64_t(value);
+    return !arg.trapped && arg.has_value && arg.value == uint64_t(value);
 }
 
 #define EXPECT_THROW_MESSAGE(stmt, ex_type, expected)                                        \

diff --git a/test/utils/fizzy_engine.cpp b/test/utils/fizzy_engine.cpp
@@ -58,7 +58,7 @@ fizzy::execution_result env_adler32(
 {
     assert(instance.memory != nullptr);
     const auto ret = fizzy::test::adler32(bytes_view{*instance.memory}.substr(args[0], args[1]));
-    return {false, {ret}};
+    return ret;
 }
 }  // namespace
 
@@ -134,8 +134,12 @@ std::optional<WasmEngine::FuncRef> FizzyEngine::find_function(
 WasmEngine::Result FizzyEngine::execute(
     WasmEngine::FuncRef func_ref, const std::vector<uint64_t>& args)
 {
-    const auto [trapped, result_stack] =
-        fizzy::execute(*m_instance, static_cast<uint32_t>(func_ref), args);
-    return {trapped, !result_stack.empty() ? result_stack.back() : std::optional<uint64_t>{}};
+    const auto status = fizzy::execute(*m_instance, static_cast<uint32_t>(func_ref), args);
+    if (status.trapped)
+        return {true, std::nullopt};
+    else if (status.has_value)
+        return {false, status.value};
+    else
+        return {false, std::nullopt};
 }
 }  // namespace fizzy::test