-
Notifications
You must be signed in to change notification settings - Fork 81
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
EVM-C: introduce new status codes #173
Conversation
cc @ehildenb |
enum evm_status_code { | ||
EVM_SUCCESS = 0, ///< Execution finished with success. | ||
EVM_FAILURE = 1, ///< Generic execution failure. | ||
EVM_OUT_OF_GAS = 2, | ||
EVM_BAD_INSTRUCTION = 3, | ||
EVM_UNDEFINED_INSTRUCTION = 3, ///< Unknown instruction encountered by the VM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add EVM_INVALID_INSTRUCTION
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew I've forgotten something 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
/// Tried to read outside memory bounds. | ||
/// | ||
/// An example is RETURNDATACOPY reading past the available buffer. | ||
EVM_INVALID_MEMORY_ACCESS = 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a note that this could be thrown by VMs instead of OUT_OF_GAS
if something like CODECOPY
access outside the range of addressable local memory? (because it uses index + offset addressing). I think having more specific error codes is a plus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a note that this could be thrown by VMs instead of OUT_OF_GAS if something like CODECOPY access outside the range of addressable local memory?
I can add the comment, but not sure a compliant VM actually can do that, since the specification says memory expansion must be done first and if that fails it is an OOG case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I suppose that we would have to make an EIP then? Then never mind, we can worry about that with the design of ewasm which will have more cases where INVALID_MEMORY_ACCESS can happen anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely makes sense for ewasm and we should preferably try to clean up the mess EVM is as a second task 😉
I will update KEVM to use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
Fixes #168.