From c02ed94974bb5f9797e69f37c54567555fd8c9bc Mon Sep 17 00:00:00 2001 From: Andrei Maiboroda Date: Wed, 21 Oct 2020 13:00:55 +0200 Subject: [PATCH 1/2] refactor: represent table elements as instance pointer + function index --- lib/fizzy/execute.cpp | 21 ++++------ lib/fizzy/instantiate.cpp | 14 ++----- lib/fizzy/instantiate.hpp | 13 ++++++- test/unittests/api_test.cpp | 8 +++- test/unittests/execute_call_test.cpp | 33 +++++++++------- test/unittests/instantiate_test.cpp | 57 +++++++++++++++++++--------- 6 files changed, 88 insertions(+), 58 deletions(-) diff --git a/lib/fizzy/execute.cpp b/lib/fizzy/execute.cpp index ff5422956..25794d3a4 100644 --- a/lib/fizzy/execute.cpp +++ b/lib/fizzy/execute.cpp @@ -477,15 +477,14 @@ void branch(const Code& code, OperandStack& stack, const uint8_t*& pc, const uin stack.drop(stack_drop); } -template -inline bool invoke_function( - const FuncType& func_type, const F& func, Instance& instance, OperandStack& stack, int depth) +inline bool invoke_function(const FuncType& func_type, uint32_t func_idx, Instance& instance, + OperandStack& stack, int depth) { const auto num_args = func_type.inputs.size(); assert(stack.size() >= num_args); const auto call_args = stack.rend() - num_args; - const auto ret = func(instance, call_args, depth + 1); + const auto ret = execute(instance, func_idx, call_args, depth + 1); // Bubble up traps if (ret.trapped) return false; @@ -503,14 +502,6 @@ inline bool invoke_function( return true; } -inline bool invoke_function(const FuncType& func_type, uint32_t func_idx, Instance& instance, - OperandStack& stack, int depth) -{ - const auto func = [func_idx](Instance& _instance, const Value* args, int _depth) { - return execute(_instance, func_idx, args, _depth); - }; - return invoke_function(func_type, func, instance, stack, depth); -} } // namespace ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args, int depth) @@ -634,12 +625,14 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args, goto trap; // check actual type against expected type - const auto& actual_type = called_func->type; + const auto& actual_type = + called_func->instance->module->get_function_type(called_func->func_idx); const auto& expected_type = instance.module->typesec[expected_type_idx]; if (expected_type != actual_type) goto trap; - if (!invoke_function(actual_type, called_func->function, instance, stack, depth)) + if (!invoke_function( + actual_type, called_func->func_idx, *called_func->instance, stack, depth)) goto trap; break; } diff --git a/lib/fizzy/instantiate.cpp b/lib/fizzy/instantiate.cpp index 57fd6834e..6ffc5d8c2 100644 --- a/lib/fizzy/instantiate.cpp +++ b/lib/fizzy/instantiate.cpp @@ -330,11 +330,7 @@ std::unique_ptr instantiate(std::unique_ptr module, auto it_table = instance->table->begin() + elementsec_offsets[i]; for (const auto idx : instance->module->elementsec[i].init) { - auto func = [idx, &instance_ref = *instance](fizzy::Instance&, const Value* args, - int depth) { return execute(instance_ref, idx, args, depth); }; - - *it_table++ = - ExternalFunction{std::move(func), instance->module->get_function_type(idx)}; + *it_table++ = {instance.get(), idx, {}}; } } @@ -360,12 +356,8 @@ std::unique_ptr instantiate(std::unique_ptr module, auto it_table = shared_instance->table->begin() + elementsec_offsets[i]; for ([[maybe_unused]] auto _ : shared_instance->module->elementsec[i].init) { - // Wrap the function with the lambda capturing shared instance - auto& table_function = (*it_table)->function; - table_function = [shared_instance, func = std::move(table_function)]( - fizzy::Instance& _instance, const Value* args, - int depth) { return func(_instance, args, depth); }; - ++it_table; + // Capture shared instance in table element. + (*it_table++)->shared_instance = shared_instance; } } } diff --git a/lib/fizzy/instantiate.hpp b/lib/fizzy/instantiate.hpp index b66cd98f9..9f6b3e923 100644 --- a/lib/fizzy/instantiate.hpp +++ b/lib/fizzy/instantiate.hpp @@ -28,7 +28,18 @@ struct ExternalFunction FuncType type; }; -using table_elements = std::vector>; +// Table element, which references a function in any instance. +struct TableElement +{ + Instance* instance; + FuncIdx func_idx; + // This pointer is empty most of the time and is used only to keep instance alive in one edge + // case, when start function traps, but instantiate has already modified some elements of a + // shared (imported) table. + std::shared_ptr shared_instance; +}; + +using table_elements = std::vector>; using table_ptr = std::unique_ptr; struct ExternalTable diff --git a/test/unittests/api_test.cpp b/test/unittests/api_test.cpp index 61e317d1b..af335857d 100644 --- a/test/unittests/api_test.cpp +++ b/test/unittests/api_test.cpp @@ -412,8 +412,12 @@ TEST(api, find_exported_table) ASSERT_TRUE(opt_table); EXPECT_EQ(opt_table->table, instance->table.get()); EXPECT_EQ(opt_table->table->size(), 2); - EXPECT_THAT((*opt_table->table)[0]->function(*instance, {}, 0), Result(2)); - EXPECT_THAT((*opt_table->table)[1]->function(*instance, {}, 0), Result(1)); + ASSERT_TRUE((*opt_table->table)[0].has_value()); + EXPECT_EQ((*opt_table->table)[0]->instance, instance.get()); + EXPECT_EQ((*opt_table->table)[0]->func_idx, 1); + ASSERT_TRUE((*opt_table->table)[1].has_value()); + EXPECT_EQ((*opt_table->table)[1]->instance, instance.get()); + EXPECT_EQ((*opt_table->table)[1]->func_idx, 0); EXPECT_EQ(opt_table->limits.min, 2); ASSERT_TRUE(opt_table->limits.max.has_value()); EXPECT_EQ(opt_table->limits.max, 20); diff --git a/test/unittests/execute_call_test.cpp b/test/unittests/execute_call_test.cpp index 1bb4b5cc5..e4013b76f 100644 --- a/test/unittests/execute_call_test.cpp +++ b/test/unittests/execute_call_test.cpp @@ -157,6 +157,25 @@ TEST(execute_call, call_indirect_with_argument) TEST(execute_call, call_indirect_imported_table) { + /* wat2wasm + (module + (table 5 20 anyfunc) + (export "t" (table 0)) + (elem (i32.const 0) $f3 $f2 $f1 $f4 $f5) + (func $f1 (result i32) (i32.const 1)) + (func $f2 (result i32) (i32.const 2)) + (func $f3 (result i32) (i32.const 3)) + (func $f4 (result i64) (i64.const 4)) + (func $f5 (result i32) unreachable) + ) + */ + const auto bin_exported_table = from_hex( + "0061736d010000000109026000017f6000017e03060500000001000405017001051407050101740100090b0100" + "41000b0502010003040a1905040041010b040041020b040041030b040042040b0300000b"); + auto instance_exported_table = instantiate(parse(bin_exported_table)); + auto table = find_exported_table(*instance_exported_table, "t"); + ASSERT_TRUE(table.has_value()); + /* wat2wasm (module (type $out_i32 (func (result i32))) @@ -171,19 +190,7 @@ TEST(execute_call, call_indirect_imported_table) "0061736d01000000010a026000017f60017f017f020a01016d01740170010514030201010a0901070020001100" "000b"); - auto f1 = [](Instance&, const Value*, int) { return Value{1}; }; - auto f2 = [](Instance&, const Value*, int) { return Value{2}; }; - auto f3 = [](Instance&, const Value*, int) { return Value{3}; }; - auto f4 = [](Instance&, const Value*, int) { return Value{4}; }; - auto f5 = [](Instance&, const Value*, int) { return Trap; }; - - auto out_i32 = FuncType{{}, {ValType::i32}}; - auto out_i64 = FuncType{{}, {ValType::i64}}; - - table_elements table{ - {{f3, out_i32}}, {{f2, out_i32}}, {{f1, out_i32}}, {{f4, out_i64}}, {{f5, out_i32}}}; - - auto instance = instantiate(parse(bin), {}, {{&table, {5, 20}}}); + auto instance = instantiate(parse(bin), {}, {*table}); for (const auto param : {0u, 1u, 2u}) { diff --git a/test/unittests/instantiate_test.cpp b/test/unittests/instantiate_test.cpp index abf0af6a4..62104cb0a 100644 --- a/test/unittests/instantiate_test.cpp +++ b/test/unittests/instantiate_test.cpp @@ -18,7 +18,7 @@ namespace uint32_t call_table_func(Instance& instance, size_t idx) { const auto& elem = (*instance.table)[idx]; - const auto res = elem->function(instance, {}, 0); + const auto res = execute(*elem->instance, elem->func_idx, {}, 0); EXPECT_TRUE(res.has_value); return as_uint32(res.value); } @@ -622,6 +622,21 @@ TEST(instantiate, element_section_offset_too_large) TEST(instantiate, element_section_fills_imported_table) { + /* wat2wasm + (module + (table 4 anyfunc) + (export "t" (table 0)) + (elem (i32.const 0) $f0) + (func $f0 (result i32) (i32.const 0)) + ) + */ + const auto bin_exported_table = from_hex( + "0061736d010000000105016000017f03020100040401700004070501017401000907010041000b01000a060104" + "0041000b"); + auto instance_exported_table = instantiate(parse(bin_exported_table)); + auto table = find_exported_table(*instance_exported_table, "t"); + ASSERT_TRUE(table.has_value()); + /* wat2wasm (module (table (import "m" "tab") 4 funcref) @@ -637,11 +652,7 @@ TEST(instantiate, element_section_fills_imported_table) "0061736d010000000105016000017f020b01016d037461620170000403050400000000090f020041010b020001" "0041020b0202030a1504040041010b040041020b040041030b040041040b"); - auto f0 = [](Instance&, const Value*, int) { return Value{0}; }; - - table_elements table(4); - table[0] = ExternalFunction{f0, FuncType{{}, {ValType::i32}}}; - auto instance = instantiate(parse(bin), {}, {{&table, {4, std::nullopt}}}); + auto instance = instantiate(parse(bin), {}, {*table}); ASSERT_EQ(instance->table->size(), 4); EXPECT_EQ(call_table_func(*instance, 0), 0); @@ -652,6 +663,21 @@ TEST(instantiate, element_section_fills_imported_table) TEST(instantiate, element_section_out_of_bounds_doesnt_change_imported_table) { + /* wat2wasm + (module + (table 3 anyfunc) + (export "t" (table 0)) + (elem (i32.const 0) $f0) + (func $f0 (result i32) (i32.const 0)) + ) + */ + const auto bin_exported_table = from_hex( + "0061736d010000000105016000017f03020100040401700003070501017401000907010041000b01000a060104" + "0041000b"); + auto instance_exported_table = instantiate(parse(bin_exported_table)); + auto table = find_exported_table(*instance_exported_table, "t"); + ASSERT_TRUE(table.has_value()); + /* wat2wasm (module (table (import "m" "tab") 3 funcref) @@ -664,18 +690,15 @@ TEST(instantiate, element_section_out_of_bounds_doesnt_change_imported_table) "0061736d010000000105016000017f020b01016d037461620170000303020100090f020041000b020000004102" "0b0200000a0601040041010b"); - auto f0 = [](Instance&, const Value*, int) { return Value{0}; }; - - table_elements table(3); - table[0] = ExternalFunction{f0, FuncType{{}, {ValType::i32}}}; - - EXPECT_THROW_MESSAGE(instantiate(parse(bin), {}, {{&table, {3, std::nullopt}}}), - instantiate_error, "element segment is out of table bounds"); + EXPECT_THROW_MESSAGE(instantiate(parse(bin), {}, {*table}), instantiate_error, + "element segment is out of table bounds"); - ASSERT_EQ(table.size(), 3); - EXPECT_EQ(*table[0]->function.target(), f0); - EXPECT_FALSE(table[1].has_value()); - EXPECT_FALSE(table[2].has_value()); + const auto& table_elements = *table->table; + ASSERT_EQ(table_elements.size(), 3); + EXPECT_EQ(table_elements[0]->instance, instance_exported_table.get()); + EXPECT_EQ(table_elements[0]->func_idx, 0); + EXPECT_FALSE(table_elements[1].has_value()); + EXPECT_FALSE(table_elements[2].has_value()); } TEST(instantiate, data_section) From 77fb4db58fce4b1c2a8adcd9ead6264e9d6f56fb Mon Sep 17 00:00:00 2001 From: Andrei Maiboroda Date: Wed, 21 Oct 2020 15:35:57 +0200 Subject: [PATCH 2/2] refactor: Instead of optional for table elements, check instance is set --- lib/fizzy/execute.cpp | 6 +++--- lib/fizzy/instantiate.cpp | 2 +- lib/fizzy/instantiate.hpp | 8 +++++--- test/unittests/api_test.cpp | 10 ++++------ test/unittests/instantiate_test.cpp | 30 ++++++++++++++--------------- 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/lib/fizzy/execute.cpp b/lib/fizzy/execute.cpp index 25794d3a4..afce983de 100644 --- a/lib/fizzy/execute.cpp +++ b/lib/fizzy/execute.cpp @@ -621,18 +621,18 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args, goto trap; const auto called_func = (*instance.table)[elem_idx]; - if (!called_func.has_value()) + if (!called_func.instance) // Table element not initialized. goto trap; // check actual type against expected type const auto& actual_type = - called_func->instance->module->get_function_type(called_func->func_idx); + called_func.instance->module->get_function_type(called_func.func_idx); const auto& expected_type = instance.module->typesec[expected_type_idx]; if (expected_type != actual_type) goto trap; if (!invoke_function( - actual_type, called_func->func_idx, *called_func->instance, stack, depth)) + actual_type, called_func.func_idx, *called_func.instance, stack, depth)) goto trap; break; } diff --git a/lib/fizzy/instantiate.cpp b/lib/fizzy/instantiate.cpp index 6ffc5d8c2..f7d9db29d 100644 --- a/lib/fizzy/instantiate.cpp +++ b/lib/fizzy/instantiate.cpp @@ -357,7 +357,7 @@ std::unique_ptr instantiate(std::unique_ptr module, for ([[maybe_unused]] auto _ : shared_instance->module->elementsec[i].init) { // Capture shared instance in table element. - (*it_table++)->shared_instance = shared_instance; + (*it_table++).shared_instance = shared_instance; } } } diff --git a/lib/fizzy/instantiate.hpp b/lib/fizzy/instantiate.hpp index 9f6b3e923..8a80fcb00 100644 --- a/lib/fizzy/instantiate.hpp +++ b/lib/fizzy/instantiate.hpp @@ -31,15 +31,17 @@ struct ExternalFunction // Table element, which references a function in any instance. struct TableElement { - Instance* instance; - FuncIdx func_idx; + // Pointer to function's instance or nullptr when table element is not initialized. + Instance* instance = nullptr; + // Index of the function in instance. + FuncIdx func_idx = 0; // This pointer is empty most of the time and is used only to keep instance alive in one edge // case, when start function traps, but instantiate has already modified some elements of a // shared (imported) table. std::shared_ptr shared_instance; }; -using table_elements = std::vector>; +using table_elements = std::vector; using table_ptr = std::unique_ptr; struct ExternalTable diff --git a/test/unittests/api_test.cpp b/test/unittests/api_test.cpp index af335857d..30487ca6e 100644 --- a/test/unittests/api_test.cpp +++ b/test/unittests/api_test.cpp @@ -412,12 +412,10 @@ TEST(api, find_exported_table) ASSERT_TRUE(opt_table); EXPECT_EQ(opt_table->table, instance->table.get()); EXPECT_EQ(opt_table->table->size(), 2); - ASSERT_TRUE((*opt_table->table)[0].has_value()); - EXPECT_EQ((*opt_table->table)[0]->instance, instance.get()); - EXPECT_EQ((*opt_table->table)[0]->func_idx, 1); - ASSERT_TRUE((*opt_table->table)[1].has_value()); - EXPECT_EQ((*opt_table->table)[1]->instance, instance.get()); - EXPECT_EQ((*opt_table->table)[1]->func_idx, 0); + EXPECT_EQ((*opt_table->table)[0].instance, instance.get()); + EXPECT_EQ((*opt_table->table)[0].func_idx, 1); + EXPECT_EQ((*opt_table->table)[1].instance, instance.get()); + EXPECT_EQ((*opt_table->table)[1].func_idx, 0); EXPECT_EQ(opt_table->limits.min, 2); ASSERT_TRUE(opt_table->limits.max.has_value()); EXPECT_EQ(opt_table->limits.max, 20); diff --git a/test/unittests/instantiate_test.cpp b/test/unittests/instantiate_test.cpp index 62104cb0a..493ee7c57 100644 --- a/test/unittests/instantiate_test.cpp +++ b/test/unittests/instantiate_test.cpp @@ -18,7 +18,7 @@ namespace uint32_t call_table_func(Instance& instance, size_t idx) { const auto& elem = (*instance.table)[idx]; - const auto res = execute(*elem->instance, elem->func_idx, {}, 0); + const auto res = execute(*elem.instance, elem.func_idx, {}, 0); EXPECT_TRUE(res.has_value); return as_uint32(res.value); } @@ -551,7 +551,7 @@ TEST(instantiate, element_section) auto instance = instantiate(parse(bin)); ASSERT_EQ(instance->table->size(), 4); - EXPECT_FALSE((*instance->table)[0].has_value()); + EXPECT_EQ((*instance->table)[0].instance, nullptr); EXPECT_EQ(call_table_func(*instance, 1), 1); EXPECT_EQ(call_table_func(*instance, 2), 3); EXPECT_EQ(call_table_func(*instance, 3), 3); @@ -576,10 +576,10 @@ TEST(instantiate, element_section_offset_from_global) auto instance = instantiate(parse(bin)); ASSERT_EQ(instance->table->size(), 4); - EXPECT_FALSE((*instance->table)[0].has_value()); + EXPECT_EQ((*instance->table)[0].instance, nullptr); EXPECT_EQ(call_table_func(*instance, 1), 1); EXPECT_EQ(call_table_func(*instance, 2), 2); - EXPECT_FALSE((*instance->table)[3].has_value()); + EXPECT_EQ((*instance->table)[3].instance, nullptr); } TEST(instantiate, element_section_offset_from_imported_global) @@ -601,10 +601,10 @@ TEST(instantiate, element_section_offset_from_imported_global) auto instance = instantiate(parse(bin), {}, {}, {}, {g}); ASSERT_EQ(instance->table->size(), 4); - EXPECT_FALSE((*instance->table)[0].has_value()); + EXPECT_EQ((*instance->table)[0].instance, nullptr); EXPECT_EQ(call_table_func(*instance, 1), 1); EXPECT_EQ(call_table_func(*instance, 2), 2); - EXPECT_FALSE((*instance->table)[3].has_value()); + EXPECT_EQ((*instance->table)[3].instance, nullptr); } TEST(instantiate, element_section_offset_too_large) @@ -695,10 +695,10 @@ TEST(instantiate, element_section_out_of_bounds_doesnt_change_imported_table) const auto& table_elements = *table->table; ASSERT_EQ(table_elements.size(), 3); - EXPECT_EQ(table_elements[0]->instance, instance_exported_table.get()); - EXPECT_EQ(table_elements[0]->func_idx, 0); - EXPECT_FALSE(table_elements[1].has_value()); - EXPECT_FALSE(table_elements[2].has_value()); + EXPECT_EQ(table_elements[0].instance, instance_exported_table.get()); + EXPECT_EQ(table_elements[0].func_idx, 0); + EXPECT_EQ(table_elements[1].instance, nullptr); + EXPECT_EQ(table_elements[2].instance, nullptr); } TEST(instantiate, data_section) @@ -815,8 +815,8 @@ TEST(instantiate, data_elem_section_errors_dont_change_imports) instantiate(parse(bin_data_error), {}, {{&table, {3, std::nullopt}}}, {{&memory, {1, 1}}}), instantiate_error, "data segment is out of memory bounds"); - EXPECT_FALSE(table[0].has_value()); - EXPECT_FALSE(table[1].has_value()); + EXPECT_EQ(table[0].instance, nullptr); + EXPECT_EQ(table[1].instance, nullptr); EXPECT_EQ(memory[0], 0); /* wat2wasm @@ -837,9 +837,9 @@ TEST(instantiate, data_elem_section_errors_dont_change_imports) instantiate(parse(bin_elem_error), {}, {{&table, {3, std::nullopt}}}, {{&memory, {1, 1}}}), instantiate_error, "element segment is out of table bounds"); - EXPECT_FALSE(table[0].has_value()); - EXPECT_FALSE(table[1].has_value()); - EXPECT_FALSE(table[2].has_value()); + EXPECT_EQ(table[0].instance, nullptr); + EXPECT_EQ(table[1].instance, nullptr); + EXPECT_EQ(table[2].instance, nullptr); EXPECT_EQ(memory[0], 0); }