Skip to content
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

Execution context generalization #779

Merged
merged 6 commits into from
Apr 9, 2021
Merged

Execution context generalization #779

merged 6 commits into from
Apr 9, 2021

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Apr 1, 2021

Before #777 lands I propose to generalize ExecutionContext:

  • move it to execution_context.hpp,
  • rename Guard to something like LocalContext,
  • rename increment_call_depth() to something like create_local_context().

@chfast chfast requested review from gumb0 and axic April 1, 2021 10:05
@codecov
Copy link

codecov bot commented Apr 1, 2021

Codecov Report

Merging #779 (6095fb6) into master (a1a1b61) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #779   +/-   ##
=======================================
  Coverage   99.24%   99.24%           
=======================================
  Files          78       79    +1     
  Lines       11962    11962           
=======================================
  Hits        11872    11872           
  Misses         90       90           
Flag Coverage Δ
rust 99.87% <ø> (ø)
spectests 90.54% <100.00%> (ø)
unittests 99.20% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/fizzy/execute.hpp 100.00% <ø> (ø)
lib/fizzy/execute.cpp 99.53% <100.00%> (ø)
lib/fizzy/execution_context.hpp 100.00% <100.00%> (ø)
test/unittests/execute_call_depth_test.cpp 100.00% <100.00%> (ø)

@gumb0
Copy link
Collaborator

gumb0 commented Apr 8, 2021

Nit: the commit message Make ExecutionContext::Guard non-copyable/non-movable I think is wrong, as it's still movable, isnt't it?

@@ -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().

@chfast
Copy link
Collaborator Author

chfast commented Apr 8, 2021

Nit: the commit message Make ExecutionContext::Guard non-copyable/non-movable I think is wrong, as it's still movable, isnt't it?

To my understanding, move constructor is not implicitly generated if you declare a copy constructor.

@gumb0
Copy link
Collaborator

gumb0 commented Apr 8, 2021

Nit: the commit message Make ExecutionContext::Guard non-copyable/non-movable I think is wrong, as it's still movable, isnt't it?

To my understanding, move constructor is not implicitly generated if you declare a copy constructor.

I think only if you define a custom one, but I might be wrong.

@chfast
Copy link
Collaborator Author

chfast commented Apr 8, 2021

Nit: the commit message Make ExecutionContext::Guard non-copyable/non-movable I think is wrong, as it's still movable, isnt't it?

To my understanding, move constructor is not implicitly generated if you declare a copy constructor.

I think only if you define a custom one, but I might be wrong.

https://godbolt.org/z/14osqo3rG

@gumb0
Copy link
Collaborator

gumb0 commented Apr 8, 2021

Nit: the commit message Make ExecutionContext::Guard non-copyable/non-movable I think is wrong, as it's still movable, isnt't it?

To my understanding, move constructor is not implicitly generated if you declare a copy constructor.

I think only if you define a custom one, but I might be wrong.

https://godbolt.org/z/14osqo3rG

I would add explicit deletions of move contructor and move assignment then.

@chfast
Copy link
Collaborator Author

chfast commented Apr 8, 2021

I would add explicit deletions of move contructor and move assignment then.

OK.

@axic axic merged commit 5b99b52 into master Apr 9, 2021
@axic axic deleted the execution_context branch April 9, 2021 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants