Skip to content

Execution context generalization #779

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.

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
merged 6 commits into from
Apr 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 lib/fizzy/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ target_sources(
exceptions.hpp
execute.cpp
execute.hpp
execution_context.hpp
instantiate.cpp
instantiate.hpp
instructions.cpp
Expand Down
3 changes: 2 additions & 1 deletion lib/fizzy/execute.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,6 @@ inline bool invoke_function(const FuncType& func_type, uint32_t func_idx, Instan
assert(stack.size() >= num_args);
const auto call_args = stack.rend() - num_args;

const auto ctx_guard = ctx.increment_call_depth();
const auto ret = execute(instance, func_idx, call_args, ctx);
// Bubble up traps
if (ret.trapped)
Expand Down Expand Up @@ -574,6 +573,8 @@ ExecutionResult execute(
const auto& code = instance.module->get_code(func_idx);
auto* const memory = instance.memory.get();

const auto local_ctx = ctx.create_local_context();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does moving this action from invoke to execute change the depth that host functions see?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. We record the value in many tests:

recorded_depth = ctx.depth;
.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought so, but why?
Before: call opcode calls invoke_function, it increments depth, calls execute, it calls host function.
After: call opcode calls invoke_function, calls execute, it calls host function. (increment only if it's not imported)

Copy link
Collaborator Author

@chfast chfast Apr 8, 2021

Choose a reason for hiding this comment

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

Before: execute(depth: D), call opcode, invoke_function(): increment depth, execute(depth: D+1).
After: execute(depth: D): increment depth, call opcode, invoke_function(), execute(depth: D+1).

So this changes the value of the depth only inside the execute().

This is needed because later I want to get access the ExecutionContext shared stack space in the same place where depth is incremented.

I was not able to also move the depth check to the same place easily. This may be handled later, e.g. by trapping inside the invoke_function().


OperandStack stack(args, func_type.inputs.size(), code.local_count,
static_cast<size_t>(code.max_stack_height));

Expand Down
28 changes: 1 addition & 27 deletions lib/fizzy/execute.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "cxx20/span.hpp"
#include "exceptions.hpp"
#include "execution_context.hpp"
#include "instantiate.hpp"
#include "limits.hpp"
#include "module.hpp"
Expand Down Expand Up @@ -41,33 +42,6 @@ constexpr ExecutionResult Void{true};
/// Shortcut for execution that resulted in a trap.
constexpr ExecutionResult Trap{false};

/// The storage for information shared by calls in the same execution "thread".
/// Users may decide how to allocate the execution context, but some good defaults are available.
class ExecutionContext
{
/// Call depth increment guard.
/// It will automatically decrement the call depth to the original value
/// when going out of scope.
class [[nodiscard]] Guard
{
ExecutionContext& m_execution_context; ///< Reference to the guarded execution context.

public:
explicit Guard(ExecutionContext& ctx) noexcept : m_execution_context{ctx} {}
~Guard() noexcept { --m_execution_context.depth; }
};

public:
int depth = 0; ///< Current call depth.

/// Increments the call depth and returns the guard object which decrements
/// the call depth back to the original value when going out of scope.
Guard increment_call_depth() noexcept
{
++depth;
return Guard{*this};
}
};

/// Execute a function from an instance.
///
Expand Down
41 changes: 41 additions & 0 deletions lib/fizzy/execution_context.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Fizzy: A fast WebAssembly interpreter
// Copyright 2021 The Fizzy Authors.
// SPDX-License-Identifier: Apache-2.0

#pragma once

namespace fizzy
{
/// The storage for information shared by calls in the same execution "thread".
/// Users may decide how to allocate the execution context, but some good defaults are available.
class ExecutionContext
{
/// Call local execution context.
/// It will automatically decrement the call depth to the original value
/// when going out of scope.
class [[nodiscard]] LocalContext
{
ExecutionContext& m_shared_ctx; ///< Reference to the shared execution context.

public:
LocalContext(const LocalContext&) = delete;
LocalContext(LocalContext&&) = delete;
LocalContext& operator=(const LocalContext&) = delete;
LocalContext& operator=(LocalContext&&) = delete;

explicit LocalContext(ExecutionContext& ctx) noexcept : m_shared_ctx{ctx}
{
++m_shared_ctx.depth;
}

~LocalContext() noexcept { --m_shared_ctx.depth; }
};

public:
int depth = 0; ///< Current call depth.

/// Increments the call depth and returns the local call context which
/// decrements the call depth back to the original value when going out of scope.
LocalContext create_local_context() noexcept { return LocalContext{*this}; }
};
} // namespace fizzy
8 changes: 4 additions & 4 deletions test/unittests/execute_call_depth_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ TEST(execute_call_depth, call_host_function_calling_wasm_function_inclusive)
constexpr auto host_f = [](std::any&, Instance& instance, const Value*,
ExecutionContext& ctx) noexcept {
recorded_depth = ctx.depth;
const auto ctx_guard = ctx.increment_call_depth();
const auto local_ctx = ctx.create_local_context();
return fizzy::execute(instance, 2 /* $leaf */, {}, ctx);
};

Expand Down Expand Up @@ -304,7 +304,7 @@ TEST(execute_call_depth, call_host_function_calling_another_wasm_module)
ExecutionContext& ctx) noexcept {
recorded_depth = ctx.depth;
auto instance = *std::any_cast<Instance*>(&host_context);
const auto ctx_guard = ctx.increment_call_depth();
const auto local_ctx = ctx.create_local_context();
return fizzy::execute(*instance, 0, {}, ctx);
};

Expand Down Expand Up @@ -466,7 +466,7 @@ TEST(execute_call_depth, execute_host_function_within_wasm_recursion_limit)
constexpr auto host_f = [](std::any&, Instance& instance, const Value*,
ExecutionContext& ctx) noexcept {
max_recorded_wasm_recursion_depth = std::max(max_recorded_wasm_recursion_depth, ctx.depth);
const auto ctx_guard = ctx.increment_call_depth();
const auto local_ctx = ctx.create_local_context();
return fizzy::execute(instance, 0, {}, ctx);
};

Expand Down Expand Up @@ -537,7 +537,7 @@ TEST(execute_call, call_host_function_calling_wasm_interleaved_infinite_recursio
ExecutionContext& ctx) noexcept {
EXPECT_LT(ctx.depth, DepthLimit);
++counter;
const auto ctx_guard = ctx.increment_call_depth();
const auto local_ctx = ctx.create_local_context();
return fizzy::execute(instance, 1, {}, ctx);
};

Expand Down