-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Implementation of EOF in Solidity [DO NOT MERGE] #15294
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
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
c49b88b to
88cf606
Compare
| m_items.back().setLocation(m_currentSourceLocation); | ||
| m_items.back().m_modifierDepth = m_currentModifierDepth; | ||
| return m_items.back(); | ||
| auto& currentItems = m_codeSections.at(m_currentCodeSection).items; |
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.
m_codeSection.at(...) can throw - either a util::contains check should go here, or even better, an assertion if you're sure that the current code section is present.
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.
Right. I'm adding an assertion as m_currentCodeSection is changed on in void beginFunction(uint16_t _functionID) and similar assertion is there too.
| for (unsigned tagSize = subTagSize; true; ++tagSize) | ||
| { | ||
| size_t ret = 1; | ||
| for (auto const& i: m_data) |
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.
Is m_data no longer taken into account?
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.
That part of the calculation moved to ::assemble - it's part of bytesRequiredForDataUpperBound there (both for EOF and legacy)...
But we should probably rethink this as well at the latest as part of #15294 (comment) - For example: since in EOF there won't be any PushTags, the weird outer loop here becomes superfluous as well, since code size no longer depends on the tag size (the "tag size" in general becomes irrelevant).
For now, maybe a comment in the header is enough, clarifying that even for legacy this now only counts "pure" code size without data?
|
|
||
| std::vector<size_t> successors; | ||
|
|
||
| if ( |
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 assume that ConditionalRelativeJump is not here because both 'branches' need to have their stack heights calculated? I.e. both the next assembly item as well as the tagged one that we're (possibly) jumping to?
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.
Yes. For ConditionalRelativeJump we need to calculated both branches and one of them starts with the next instruction but second is the jump target. It's being added to successors by next if
libevmasm/Assembly.cpp
Outdated
| { | ||
| uint16_t maxStackHeight = _args; | ||
| std::stack<size_t> worklist; | ||
| std::vector<int32_t> stack_heights(_items.size(), -1); |
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.
| std::vector<int32_t> stack_heights(_items.size(), -1); | |
| std::vector<size_t> stack_heights(_items.size(), std::numeric_limits<size_t>::max); |
stackHeight should also be unsigned - I would assume our stack height is never going to be negative?
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.
Fixed
| Functionalizer f(_out, _prefix, _sourceCodes, *this); | ||
|
|
||
| for (auto const& i: m_items) | ||
| // TODO: support EOF |
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.
In cases like this (and e.g. similarly for Assembly::assemblyJSON and possibly a few of other similar TODOs like this), it'd be better to add an explicit
solUnimplementedAssert(!m_eofVersion.has_value(), "Assembly output for EOF is not yet implemented.");Better to error out and then implement it properly later in a subsequent PR than to give a partial output of only the first code section with the rest missing.
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.
Done. Additionally I switched off the assembly optimization for EOF as event first step requires verification.
5ab6923 to
64e4050
Compare
…typo in input parsing Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
…EIP-3540) Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
Add `DATALOAD` instruction
Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
Preparation to implement new contract creation Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
Co-authored-by: Daniel Kirchner <daniel@ekpyron.org>
…endMessage Co-authored-by: Rodrigo Q. Saramago <rodrigoqsaramago@gmail.com>
| return !reachabilityCheck.visited.count(entry); | ||
| }); | ||
|
|
||
| // Remove functions which are never referenced. |
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.
This change in cleanUnreachable could be split out into a separate PR and merged directly into develop up front, right?
EDIT: we can revisit once we get to the commits containing it, whether splitting really makes things easier, fine for now.
| if (!settings["eofVersion"].is_number_unsigned()) | ||
| return formatFatalError(Error::Type::JSONError, "eofVersion must be an unsigned integer."); | ||
| auto eofVersion = settings["evmVersion"].get<uint8_t>(); | ||
| auto eofVersion = settings["eofVersion"].get<uint8_t>(); |
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.
This and the related evm-version check change can and should separately go into develop :-).
EDIT: as per chat, let's just merge the first three commits for now :-).
| Contract& compiledContract = m_contracts.at(_contract.fullyQualifiedName()); | ||
|
|
||
| std::shared_ptr<Compiler> compiler = std::make_shared<Compiler>(m_evmVersion, m_revertStrings, m_optimiserSettings); | ||
| std::shared_ptr<Compiler> compiler = std::make_shared<Compiler>(m_evmVersion, m_eofVersion, m_revertStrings, m_optimiserSettings); |
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.
Instead of passing m_eofVersion around here, we can just assert
solUnimplementedAssert(!m_eofVersion, "EOF is not supported for legacy code generation");
(this is only-legacy, see the solAssert(!m_viaIR, ""); below)
And thereby leave libsolidity/codegen/Compiler.h and libsolidity/codegen/CompilerContext.h largely untouched (resp. where they need an eof version just use std::nullopt directly).
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.
| text = "eofcreate(" + std::to_string(static_cast<size_t>(data())) + ")"; | ||
| break; | ||
| case ReturnContract: | ||
| text = "returcontract(" + std::to_string(static_cast<size_t>(data())) + ")"; |
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.
typo in assembly text of RETURNCONTRACT
| text = "returcontract(" + std::to_string(static_cast<size_t>(data())) + ")"; | |
| text = "returncontract(" + std::to_string(static_cast<size_t>(data())) + ")"; |
|
This PR is splitted into many separated PRs which are in the review. This PRs is supposed to be closed without merging after those PRs are merged. |
EOF Mega Spec
DATALOADN)sendMessageinterfacecompileToEOFflag in semantic tests settingsTODO:
Immutables bug reported by Paradigm needs to be fixed.FIXEDsoltest.