Skip to content

refactor: Allocate module dynamically #575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gumb0 merged 2 commits into from
Oct 12, 2020
Merged

refactor: Allocate module dynamically #575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb7ed9e
refactor: Allocate module dynamically
gumb0 Oct 9, 2020
f5f5c3b
Allow returning different type without std::move
gumb0 Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ if(WEVERYTHING)
-Wno-double-promotion
-Wno-float-equal
-Wno-padded
-Wno-return-std-move-in-c++11
Copy link
Copy Markdown
Collaborator Author

@gumb0 gumb0 Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chfast is this ok or should I disable it only for clang somehow?

Copy link
Copy Markdown
Collaborator

@chfast chfast Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You disabled it only for -Weverything. If that's intentional, then we are fine as -Weverything only make sense for Clang. I'm assuming -Wreturn-std-move-in-c++11 is not enabled in default build.

-Wno-switch-enum
)
endif()
Expand Down
20 changes: 10 additions & 10 deletions lib/fizzy/execute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,14 +522,14 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
if (depth > CallStackLimit)
return Trap;

const auto& func_type = instance.module.get_function_type(func_idx);
const auto& func_type = instance.module->get_function_type(func_idx);

assert(instance.module.imported_function_types.size() == instance.imported_functions.size());
assert(instance.module->imported_function_types.size() == instance.imported_functions.size());
if (func_idx < instance.imported_functions.size())
return instance.imported_functions[func_idx].function(
instance, {args, func_type.inputs.size()}, depth);

const auto& code = instance.module.get_code(func_idx);
const auto& code = instance.module->get_code(func_idx);
auto* const memory = instance.memory.get();

OperandStack stack(args, func_type.inputs.size(), code.local_count,
Expand Down Expand Up @@ -616,7 +616,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
case Instr::call:
{
const auto called_func_idx = read<uint32_t>(immediates);
const auto& called_func_type = instance.module.get_function_type(called_func_idx);
const auto& called_func_type = instance.module->get_function_type(called_func_idx);

if (!invoke_function(called_func_type, called_func_idx, instance, stack, depth))
goto trap;
Expand All @@ -627,7 +627,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
assert(instance.table != nullptr);

const auto expected_type_idx = read<uint32_t>(immediates);
assert(expected_type_idx < instance.module.typesec.size());
assert(expected_type_idx < instance.module->typesec.size());

const auto elem_idx = stack.pop().as<uint32_t>();
if (elem_idx >= instance.table->size())
Expand All @@ -639,7 +639,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,

// check actual type against expected type
const auto& actual_type = called_func->type;
const auto& expected_type = instance.module.typesec[expected_type_idx];
const auto& expected_type = instance.module->typesec[expected_type_idx];
if (expected_type != actual_type)
goto trap;

Expand Down Expand Up @@ -693,7 +693,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
else
{
const auto module_global_idx = idx - instance.imported_globals.size();
assert(module_global_idx < instance.module.globalsec.size());
assert(module_global_idx < instance.module->globalsec.size());
stack.push(instance.globals[module_global_idx]);
}
break;
Expand All @@ -709,8 +709,8 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
else
{
const auto module_global_idx = idx - instance.imported_globals.size();
assert(module_global_idx < instance.module.globalsec.size());
assert(instance.module.globalsec[module_global_idx].type.is_mutable);
assert(module_global_idx < instance.module->globalsec.size());
assert(instance.module->globalsec[module_global_idx].type.is_mutable);
instance.globals[module_global_idx] = stack.pop();
}
break;
Expand Down Expand Up @@ -1562,7 +1562,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,

end:
assert(pc == &code.instructions[code.instructions.size()]); // End of code must be reached.
assert(stack.size() == instance.module.get_function_type(func_idx).outputs.size());
assert(stack.size() == instance.module->get_function_type(func_idx).outputs.size());

return stack.size() != 0 ? ExecutionResult{stack.top()} : Void;

Expand Down
2 changes: 1 addition & 1 deletion lib/fizzy/execute.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ ExecutionResult execute(
inline ExecutionResult execute(
Instance& instance, FuncIdx func_idx, std::initializer_list<Value> args) noexcept
{
assert(args.size() == instance.module.get_function_type(func_idx).inputs.size());
assert(args.size() == instance.module->get_function_type(func_idx).inputs.size());
return execute(instance, func_idx, args.begin());
}
} // namespace fizzy
68 changes: 32 additions & 36 deletions lib/fizzy/instantiate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,22 +237,22 @@ std::optional<uint32_t> find_export(const Module& module, ExternalKind kind, std

} // namespace

std::unique_ptr<Instance> instantiate(Module module,
std::unique_ptr<Instance> instantiate(std::unique_ptr<const Module> module,
std::vector<ExternalFunction> imported_functions, std::vector<ExternalTable> imported_tables,
std::vector<ExternalMemory> imported_memories, std::vector<ExternalGlobal> imported_globals,
uint32_t memory_pages_limit /*= DefaultMemoryPagesLimit*/)
{
assert(module.funcsec.size() == module.codesec.size());
assert(module->funcsec.size() == module->codesec.size());

match_imported_functions(module.imported_function_types, imported_functions);
match_imported_tables(module.imported_table_types, imported_tables);
match_imported_memories(module.imported_memory_types, imported_memories);
match_imported_globals(module.imported_global_types, imported_globals);
match_imported_functions(module->imported_function_types, imported_functions);
match_imported_tables(module->imported_table_types, imported_tables);
match_imported_memories(module->imported_memory_types, imported_memories);
match_imported_globals(module->imported_global_types, imported_globals);

// Init globals
std::vector<Value> globals;
globals.reserve(module.globalsec.size());
for (auto const& global : module.globalsec)
globals.reserve(module->globalsec.size());
for (auto const& global : module->globalsec)
{
// Constraint to use global.get only with imported globals is checked at validation.
assert(global.expression.kind != ConstantExpression::Kind::GlobalGet ||
Expand All @@ -262,10 +262,10 @@ std::unique_ptr<Instance> instantiate(Module module,
globals.emplace_back(value);
}

auto [table, table_limits] = allocate_table(module.tablesec, imported_tables);
auto [table, table_limits] = allocate_table(module->tablesec, imported_tables);

auto [memory, memory_limits] =
allocate_memory(module.memorysec, imported_memories, memory_pages_limit);
allocate_memory(module->memorysec, imported_memories, memory_pages_limit);
// In case upper limit for local/imported memory is defined,
// we adjust the hard memory limit, to ensure memory.grow will fail when exceeding it.
// Note: allocate_memory ensures memory's max limit is always below memory_pages_limit.
Expand All @@ -278,8 +278,8 @@ std::unique_ptr<Instance> instantiate(Module module,
// Before starting to fill memory and table,
// check that data and element segments are within bounds.
std::vector<uint64_t> datasec_offsets;
datasec_offsets.reserve(module.datasec.size());
for (const auto& data : module.datasec)
datasec_offsets.reserve(module->datasec.size());
for (const auto& data : module->datasec)
{
// Offset is validated to be i32, but it's used in 64-bit calculation below.
const uint64_t offset =
Expand All @@ -291,10 +291,10 @@ std::unique_ptr<Instance> instantiate(Module module,
datasec_offsets.emplace_back(offset);
}

assert(module.elementsec.empty() || table != nullptr);
assert(module->elementsec.empty() || table != nullptr);
std::vector<ptrdiff_t> elementsec_offsets;
elementsec_offsets.reserve(module.elementsec.size());
for (const auto& element : module.elementsec)
elementsec_offsets.reserve(module->elementsec.size());
for (const auto& element : module->elementsec)
{
// Offset is validated to be i32, but it's used in 64-bit calculation below.
const uint64_t offset =
Expand All @@ -307,10 +307,10 @@ std::unique_ptr<Instance> instantiate(Module module,
}

// Fill out memory based on data segments
for (size_t i = 0; i < module.datasec.size(); ++i)
for (size_t i = 0; i < module->datasec.size(); ++i)
{
// NOTE: these instructions can overlap
std::copy(module.datasec[i].init.begin(), module.datasec[i].init.end(),
std::copy(module->datasec[i].init.begin(), module->datasec[i].init.end(),
memory->data() + datasec_offsets[i]);
}

Expand All @@ -321,41 +321,41 @@ std::unique_ptr<Instance> instantiate(Module module,
std::move(imported_functions), std::move(imported_globals));

// Fill the table based on elements segment
for (size_t i = 0; i < instance->module.elementsec.size(); ++i)
for (size_t i = 0; i < instance->module->elementsec.size(); ++i)
{
// Overwrite table[offset..] with element.init
auto it_table = instance->table->begin() + elementsec_offsets[i];
for (const auto idx : instance->module.elementsec[i].init)
for (const auto idx : instance->module->elementsec[i].init)
{
auto func = [idx, &instance_ref = *instance](fizzy::Instance&, span<const Value> args,
int depth) { return execute(instance_ref, idx, args.data(), depth); };

*it_table++ =
ExternalFunction{std::move(func), instance->module.get_function_type(idx)};
ExternalFunction{std::move(func), instance->module->get_function_type(idx)};
}
}

// Run start function if present
if (instance->module.startfunc)
if (instance->module->startfunc)
{
const auto funcidx = *instance->module.startfunc;
assert(funcidx < instance->imported_functions.size() + instance->module.funcsec.size());
const auto funcidx = *instance->module->startfunc;
assert(funcidx < instance->imported_functions.size() + instance->module->funcsec.size());
if (execute(*instance, funcidx, {}).trapped)
{
// When element section modified imported table, and then start function trapped,
// modifications to the table are not rolled back.
// Instance in this case is not being returned to the user, so it needs to be kept alive
// as long as functions using it are alive in the table.
if (!imported_tables.empty() && !instance->module.elementsec.empty())
if (!imported_tables.empty() && !instance->module->elementsec.empty())
{
// Instance may be used by several functions added to the table,
// so we need a shared ownership here.
std::shared_ptr<Instance> shared_instance = std::move(instance);

for (size_t i = 0; i < shared_instance->module.elementsec.size(); ++i)
for (size_t i = 0; i < shared_instance->module->elementsec.size(); ++i)
{
auto it_table = shared_instance->table->begin() + elementsec_offsets[i];
for ([[maybe_unused]] auto _ : shared_instance->module.elementsec[i].init)
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;
Expand Down Expand Up @@ -427,7 +427,7 @@ std::optional<FuncIdx> find_exported_function(const Module& module, std::string_

std::optional<ExternalFunction> find_exported_function(Instance& instance, std::string_view name)
{
const auto opt_index = find_export(instance.module, ExternalKind::Function, name);
const auto opt_index = find_export(*instance.module, ExternalKind::Function, name);
if (!opt_index.has_value())
return std::nullopt;

Expand All @@ -436,12 +436,12 @@ std::optional<ExternalFunction> find_exported_function(Instance& instance, std::
return execute(instance, idx, args.data(), depth);
};

return ExternalFunction{std::move(func), instance.module.get_function_type(idx)};
return ExternalFunction{std::move(func), instance.module->get_function_type(idx)};
}

std::optional<ExternalGlobal> find_exported_global(Instance& instance, std::string_view name)
{
const auto opt_index = find_export(instance.module, ExternalKind::Global, name);
const auto opt_index = find_export(*instance.module, ExternalKind::Global, name);
if (!opt_index.has_value())
return std::nullopt;

Expand All @@ -457,27 +457,23 @@ std::optional<ExternalGlobal> find_exported_global(Instance& instance, std::stri
// global owned by instance
const auto module_global_idx = global_idx - instance.imported_globals.size();
return ExternalGlobal{&instance.globals[module_global_idx],
instance.module.globalsec[module_global_idx].type};
instance.module->globalsec[module_global_idx].type};
}
}

std::optional<ExternalTable> find_exported_table(Instance& instance, std::string_view name)
{
const auto& module = instance.module;

// Index returned from find_export is discarded, because there's no more than 1 table
if (!find_export(module, ExternalKind::Table, name))
if (!find_export(*instance.module, ExternalKind::Table, name))
return std::nullopt;

return ExternalTable{instance.table.get(), instance.table_limits};
}

std::optional<ExternalMemory> find_exported_memory(Instance& instance, std::string_view name)
{
const auto& module = instance.module;

// Index returned from find_export is discarded, because there's no more than 1 memory
if (!find_export(module, ExternalKind::Memory, name))
if (!find_export(*instance.module, ExternalKind::Memory, name))
return std::nullopt;

return ExternalMemory{instance.memory.get(), instance.memory_limits};
Expand Down
11 changes: 5 additions & 6 deletions lib/fizzy/instantiate.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ using bytes_ptr = std::unique_ptr<bytes, void (*)(bytes*)>;
// The module instance.
struct Instance
{
Module module;
std::unique_ptr<const Module> module;
// Memory is either allocated and owned by the instance or imported as already allocated bytes
// and owned externally.
// For these cases unique_ptr would either have a normal deleter or noop deleter respectively
Expand All @@ -70,9 +70,9 @@ struct Instance
std::vector<ExternalFunction> imported_functions;
std::vector<ExternalGlobal> imported_globals;

Instance(Module _module, bytes_ptr _memory, Limits _memory_limits, uint32_t _memory_pages_limit,
table_ptr _table, Limits _table_limits, std::vector<Value> _globals,
std::vector<ExternalFunction> _imported_functions,
Instance(std::unique_ptr<const Module> _module, bytes_ptr _memory, Limits _memory_limits,
uint32_t _memory_pages_limit, table_ptr _table, Limits _table_limits,
std::vector<Value> _globals, std::vector<ExternalFunction> _imported_functions,
std::vector<ExternalGlobal> _imported_globals)
: module(std::move(_module)),
Comment thread
chfast marked this conversation as resolved.
memory(std::move(_memory)),
Expand All @@ -87,14 +87,13 @@ struct Instance
};

// Instantiate a module.
std::unique_ptr<Instance> instantiate(Module module,
std::unique_ptr<Instance> instantiate(std::unique_ptr<const Module> module,
std::vector<ExternalFunction> imported_functions = {},
std::vector<ExternalTable> imported_tables = {},
std::vector<ExternalMemory> imported_memories = {},
std::vector<ExternalGlobal> imported_globals = {},
uint32_t memory_pages_limit = DefaultMemoryPagesLimit);


// Function that should be used by instantiate as imports, identified by module and function name.
struct ImportedFunction
{
Expand Down
Loading