-
Notifications
You must be signed in to change notification settings - Fork 305
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
cpp: Extend API for creating results #333
Conversation
fd7fffb
to
42c8d26
Compare
include/evmc/evmc.hpp
Outdated
{ | ||
if (output_size != 0) | ||
{ | ||
auto mem = static_cast<uint8_t*>(std::malloc(output_size)); |
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.
Apparently this can return nullptr
. Why not use new
and delete
here instead?
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.
Not practical. I want to keep VM implementation low profile and require only C standard library if possible.
But good point raised. How about converting it to EVMC_INTERNAL_ERROR
?
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.
How about converting it to EVMC_INTERNAL_ERROR?
I think that's a bad idea, because it masks what's going on. The caller would think the called contract failed.
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'm not sure what do you mean by a caller here but in any case it should not take EVMC internal errors as indicators for contract execution failures.
I believe we only have 2 options here: ignore it or report it as an internal error.
d55ce64
to
7184dd4
Compare
|
||
if (!buffer) | ||
{ | ||
result.status_code = EVMC_OUT_OF_MEMORY; |
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.
Still not convinced this it the right way to do it, given it confuses status of the execution vs. helpers, but if OOM is hit there is a bigger underlying issue. Perhaps worth highlighting this in the description above?
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.
but if OOM is hit there is a bigger underlying issue
What?
I don't thing this is the best way of passing output buffer. This will rather be only useful for Go, so we can drop this PR.
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.
but if OOM is hit there is a bigger underlying issue
What?
OOM = out of memory
Basically I'm ok with this change given this new specific error code is introduced.
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.
No, I had issue with understanding the sentence after "OOM". The OOM itself is difficult to handle in general. E.g. it will never happen on Linux in practice as it will happily try to allocate 2 TB of memory and in the end kill the process.
Anyway, the new specific error code was introduced: https://github.com/ethereum/evmc/pull/333/files#diff-8e9caa43721b131d9f7897e2bfd98299R309.
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.
Yeah I know, that's the line I commented on. Just having this documented on make_result
would be useful.
I used this in Go: |
d64965c
to
2d5cb58
Compare
This moves the evmc_result construction logic from evmc::result() constructor to helper.h. This function is now exposed as a C helper, C++ evmc::make_result() helper and used in the evmc::result() constructor. This is convenient for VM implementations which need to construct C evmc_result, not C++ evmc::result.
All fixed. |
Closes #258.
Usage check:
Test in Aleth (EvmCHost::call()
,EvmCHost::create()
)