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

Refactor interpreter instructions to return Result #26

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

vlopes11
Copy link
Contributor

@vlopes11 vlopes11 commented Oct 2, 2021

Interpreter instructions should have uniform execution. Since inc_pc
is fallible - it can overflow the vm memory - then every instruction is
fallible with the exception of RET under specific circumstances.

A single exception is not a good reason to have divergent
implementations for all the opcodes.

As TODO optimization, the half-word should parsed only once and
gas/execution match statement should be simplified and executed only
once as well. Having an uniform implementation for all opcodes will
facilitate that optimization.

Interpreter instructions should have uniform execution. Since `inc_pc`
is fallible - it can overflow the vm memory - then every instruction is
fallible with the exception of `RET` under specific circumstances.

A single exception is not a good reason to have divergent
implementations for all the opcodes.

As TODO optimization, the half-word should parsed only once and
gas/execution match statement should be simplified and executed only
once as well. Having an uniform implementation for all opcodes will
facilitate that optimization.
@vlopes11 vlopes11 self-assigned this Oct 2, 2021
@vlopes11 vlopes11 requested review from adlerjohn and Voxelot October 2, 2021 12:36
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Looks fine to me, will defer to @Voxelot

&& self.gas_charge(&op).is_ok()
&& self
.alu_overflow(ra, Word::overflowing_add, self.registers[rb], self.registers[rc])
.is_ok() => {}
Copy link
Member

@Voxelot Voxelot Oct 4, 2021

Choose a reason for hiding this comment

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

Why do we check .is_ok() instead of doing an immediate return of the appropriate error? With this approach we mask the actual error with a generic opcode failure.

I don't see a case where we'd need to fall-through to other matching arms, so an immediate error return seems functionally safe and more idiomatic to me. Personally, I'd prefer it if we didn't have a catch-all _ case in this match, that way we'd know at compile time which opcodes are unimplemented etc.

Here's a rust playground I made showing a way to chain multiple results together and still get the boolean comparison at the end:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=86cf4317f406310f40a40dfa671c44eb

In this case it could look something like:

Opcode::ADD(ra, rb, rc)
                if Self::is_register_writable(ra)
                    && self.gas_charge(&op)
                          .and_then(|_| self.alu_overflow(ra, Word::overflowing_add, self.registers[rb], self.registers[rc]))
                          .map(|_| true)? => {}

Copy link
Member

Choose a reason for hiding this comment

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

is_register_writable should probably also have a result with a unique error type btw

Copy link
Member

@Voxelot Voxelot Oct 4, 2021

Choose a reason for hiding this comment

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

If we didn't use the if syntax, these combinators could be cleaned up a bit:

Opcode::ADD(ra, rb, rc) => {
    Self::is_register_writable(ra)?;
    self.gas_charge(&op)?;
    self.alu_overflow(ra, Word::overflowing_add, self.registers[rb], self.registers[rc])?;
}

This seems more natural to me in the case of the call to alu_overflow than putting the op-code impl as part of the if-match statement, which makes it look like a validation function instead of the actual implementation logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of the moment, it is desirable to centralize the errors into the opcode failure so the feedback for sway (under development) is clearer on where the problem is.

The error module is to be improved to include detailed stack trace information whenever an error occur.

The gas charge is supposed to be entirely refactored in order to avoid pattern matching for constant cost functions. Also is the execute function itself that currently is performing several matching patterns to execute an opcode, when it should be only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issues:
#28
#29
#30

@vlopes11 vlopes11 merged commit d11a93c into master Oct 6, 2021
@vlopes11 vlopes11 deleted the vlopes11/refactor-instr-result branch October 6, 2021 18:31
ControlCplusControlV pushed a commit that referenced this pull request Nov 24, 2022
ControlCplusControlV pushed a commit that referenced this pull request Dec 1, 2022
mitchmindtree pushed a commit that referenced this pull request Dec 5, 2022
@mitchmindtree mitchmindtree added the fuel-vm Related to the `fuel-vm` crate. label Dec 9, 2022
xgreenx pushed a commit that referenced this pull request Dec 20, 2022
xgreenx pushed a commit that referenced this pull request Dec 20, 2022
xgreenx pushed a commit that referenced this pull request Dec 20, 2022
xgreenx pushed a commit that referenced this pull request Dec 20, 2022
xgreenx pushed a commit that referenced this pull request Dec 20, 2022
xgreenx pushed a commit that referenced this pull request Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuel-vm Related to the `fuel-vm` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants