From 08c270a4aec0b983a3280f2f89207c51168b9c0b Mon Sep 17 00:00:00 2001 From: Andrei Maiboroda Date: Wed, 21 Oct 2020 13:00:55 +0200 Subject: [PATCH] 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 ff54229560..25794d3a44 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 57fd6834e7..6ffc5d8c2d 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 b66cd98f92..9f6b3e9235 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 61e317d1b3..af335857d5 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 1bb4b5cc53..e4013b76f1 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 abf0af6a42..62104cb0a8 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)