EVM instructions table #33
Conversation
chfast
force-pushed
the
evm-instructions
branch
2 times, most recently
from
57d8f7a
to
762cdb9
Compare
|
|
||
| /** @} */ | ||
|
|
||
| static struct evmc_instruction_metrics constantinople_metrics[256] = { |
There was a problem hiding this comment.
This file is 1600 lines, maybe put revisions in separate files? It would be also easier to diff them
There was a problem hiding this comment.
After thinking about this for 2 day, I'd like to keep it as it is. I hope this is single time job and it should depend on testing, not manual review (some parts already tested, I can add test cases for all HFs).
-
To add a new EVM revision you will have to copy the recent one and apply changes. This should not be very complicated. In case it was in separate files, you will also have to alter the CMake config.
-
Having all this in 2 files make it easier to copy to other projects, even translate to other languages.
-
Having the tables in separate files require to make the symbol default visibility.
include/evmc/instructions.h
Outdated
| * from all EVM revisions. Use evmc_get_instruction_metrics_table() to know if an instruction | ||
| * is present in the given EVM revision. | ||
| * | ||
| * @return The pointer to the array of 256 instruction names. |
There was a problem hiding this comment.
Maybe a note that it contains some NULLs besides names?
| * | ||
| * @return The pointer to the array of 256 instruction names. | ||
| */ | ||
| const char* const* evmc_get_instruction_name_table(); |
There was a problem hiding this comment.
Shouldn't this also have revision parameter?
There was a problem hiding this comment.
No, it has names for all opcodes, i.e. from the latest revision. I think it's good enough for debugging.
There was a problem hiding this comment.
Would a good middle ground be a helper which based on the revision generates a table relevant to that revision? It would do that every time, so the callee would need to ensure it stores the result.
There was a problem hiding this comment.
No. I will just add the revision here as well if there is the use case for it.
There was a problem hiding this comment.
I do like the idea of having this static const table too.
| * | ||
| * This table is EVM revision independent and contains the superset of the names of the instructions | ||
| * from all EVM revisions. Use evmc_get_instruction_metrics_table() to know if an instruction | ||
| * is present in the given EVM revision. |
There was a problem hiding this comment.
I thought this was the reason for the decision, but it seems a bit convoluted.
|
Because of the name clash on Windows, I have to rename the enum items. Which one to use?
|
|
Basically it can clash with macros, correct? |
|
Is the "opcode" a more precise name to "instruction"? Everybody seems to be using "instruction" name in Ethereum. |
It clashes with |
|
I think the "instruction" term stands for the abstract instruction, while "opcode" term stands for the encoding of it as bytes. Since EVM is rather simple, there's not much going on in terms of encoding :) |
|
Based on that I think opcode would be the more relevant term here, since you assign the encoded value to it. |
No description provided.