Better safeguard against incompatible builtin handles between dialects #15961
+353
−148
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,82 @@ | ||||||
| /* | ||||||
| This file is part of solidity. | ||||||
| solidity is free software: you can redistribute it and/or modify | ||||||
| it under the terms of the GNU General Public License as published by | ||||||
| the Free Software Foundation, either version 3 of the License, or | ||||||
| (at your option) any later version. | ||||||
| solidity is distributed in the hope that it will be useful, | ||||||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||||
| GNU General Public License for more details. | ||||||
| You should have received a copy of the GNU General Public License | ||||||
| along with solidity. If not, see <http://www.gnu.org/licenses/>. | ||||||
| */ | ||||||
| // SPDX-License-Identifier: GPL-3.0 | ||||||
|
|
||||||
| #include <test/libyul/EVMDialectCompatibility.h> | ||||||
|
|
||||||
| #include <boost/test/data/test_case.hpp> | ||||||
| #include <boost/test/data/monomorphic.hpp> | ||||||
|
|
||||||
| namespace bdata = boost::unit_test::data; | ||||||
|
|
||||||
| using namespace solidity; | ||||||
| using namespace solidity::yul; | ||||||
| using namespace solidity::yul::test; | ||||||
|
|
||||||
| BOOST_AUTO_TEST_SUITE(EVMDialectCompatibility) | ||||||
|
|
||||||
| BOOST_DATA_TEST_CASE( | ||||||
| builtin_function_handle_compatibility_non_eof, | ||||||
| bdata::make(generateEVMDialectConfigurationsToTest(std::nullopt)), | ||||||
| evmDialectConfigurationToTest | ||||||
| ) | ||||||
| { | ||||||
| auto const& dialectToTestAgainst = evmDialectConfigurationToTest.dialect(); | ||||||
| // no object access for current | ||||||
| { | ||||||
| auto const& currentDialect = EVMDialect::strictAssemblyForEVM({}, std::nullopt); | ||||||
|
There was a problem hiding this comment. I'd keep it more explicit to make places where we hard-code current EVM version easier to spot.
Suggested change
BTW, we should probably add that |
||||||
| for (auto const& builtinFunctionName: currentDialect.builtinFunctionNames()) | ||||||
| requireBuiltinCompatibility(currentDialect, dialectToTestAgainst, builtinFunctionName); | ||||||
| } | ||||||
| // object access for current | ||||||
| { | ||||||
| auto const& currentDialect = EVMDialect::strictAssemblyForEVMObjects({}, std::nullopt); | ||||||
| for (auto const& builtinFunctionName: currentDialect.builtinFunctionNames()) | ||||||
| requireBuiltinCompatibility(currentDialect, dialectToTestAgainst, builtinFunctionName); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| BOOST_DATA_TEST_CASE( | ||||||
| builtin_function_handle_compatibility_eof, | ||||||
| bdata::monomorphic::grid( | ||||||
| bdata::make(generateEVMDialectConfigurationsToTest(std::nullopt)) + bdata::make(generateEVMDialectConfigurationsToTest(1)), | ||||||
|
Comment on lines
+54
to
+56
There was a problem hiding this comment. This seems to actually be testing both EOF and non-EOF despite the name. There was a problem hiding this comment. And a much more relevant difference is that it's using the latest rather than the default EVM version. In not so distant future both current and latest will have EOF, but the fact that current is not always the latest will remain. If you replace There was a problem hiding this comment. Also, I think we should have test case where we cover the "inline" -> "with objects" conversion for all dialects. Converting between distinct dialects is nice to have but these pairs are the most relevant, because for them we actually do perform such a conversion in practice. |
||||||
| bdata::make({false, true}) | ||||||
| ), | ||||||
| evmDialectConfigurationToTest, | ||||||
| withEOF | ||||||
| ) | ||||||
| { | ||||||
| auto const& dialectToTestAgainst = evmDialectConfigurationToTest.dialect(); | ||||||
| langutil::EVMVersion latestEVMVersion = langutil::EVMVersion::allVersions().back(); | ||||||
| std::optional<uint8_t> eofVersion = std::nullopt; | ||||||
| if (withEOF) | ||||||
|
There was a problem hiding this comment. It's likely that we'll forget to update this if we ever add a EOF version. We should define |
||||||
| eofVersion = 1; | ||||||
| // no object access for latest | ||||||
| { | ||||||
| auto const& latestDialect = EVMDialect::strictAssemblyForEVM(latestEVMVersion, eofVersion); | ||||||
| for (auto const& builtinFunctionName: latestDialect.builtinFunctionNames()) | ||||||
| requireBuiltinCompatibility(latestDialect, dialectToTestAgainst, builtinFunctionName); | ||||||
| } | ||||||
| // object access for latest | ||||||
| { | ||||||
| auto const& latestDialect = EVMDialect::strictAssemblyForEVMObjects(latestEVMVersion, eofVersion); | ||||||
| for (auto const& builtinFunctionName: latestDialect.builtinFunctionNames()) | ||||||
| requireBuiltinCompatibility(latestDialect, dialectToTestAgainst, builtinFunctionName); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| BOOST_AUTO_TEST_SUITE_END() | ||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| /* | ||
| This file is part of solidity. | ||
| solidity is free software: you can redistribute it and/or modify | ||
| it under the terms of the GNU General Public License as published by | ||
| the Free Software Foundation, either version 3 of the License, or | ||
| (at your option) any later version. | ||
| solidity is distributed in the hope that it will be useful, | ||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| GNU General Public License for more details. | ||
| You should have received a copy of the GNU General Public License | ||
| along with solidity. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
| // SPDX-License-Identifier: GPL-3.0 | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <libyul/backends/evm/EVMDialect.h> | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| #include <fmt/format.h> | ||
|
|
||
| #include <cstdint> | ||
| #include <optional> | ||
| #include <vector> | ||
|
|
||
|
|
||
| namespace solidity::yul::test | ||
| { | ||
|
|
||
| inline void requireBuiltinCompatibility(EVMDialect const& _dialect1, EVMDialect const& _dialect2, std::string_view const _builtin) | ||
| { | ||
| if (auto const currentHandle = _dialect1.findBuiltin(_builtin)) | ||
| if (auto const handle = _dialect2.findBuiltin(_builtin)) | ||
|
Comment on lines
+35
to
+38
There was a problem hiding this comment. This will only ensure that builtins available in both dialects have matching IDs, which is necessary but not sufficient for full compatibility. We also need to ensure that the dialect with objects has all the builtins present in the one without. |
||
| BOOST_REQUIRE_MESSAGE( | ||
| *handle == *currentHandle, | ||
| fmt::format("Failed for \"{}\" with IDs {} =/= {}.", _builtin, currentHandle->id, handle->id) | ||
| ); | ||
| } | ||
|
|
||
| struct EVMDialectConfigurationToTest | ||
| { | ||
| EVMDialect const& dialect() const | ||
| { | ||
| return objectAccess ? EVMDialect::strictAssemblyForEVMObjects(evmVersion, eofVersion) : EVMDialect::strictAssemblyForEVM(evmVersion, eofVersion); | ||
| } | ||
|
|
||
| friend std::ostream& operator<<(std::ostream& _out, EVMDialectConfigurationToTest const& _config) | ||
| { | ||
| _out << fmt::format( | ||
| "EVMConfigurationToTest[{}, eof={}, objectAccess={}]", | ||
| _config.evmVersion.name(), | ||
| _config.eofVersion.has_value() ? std::to_string(*_config.eofVersion) : "null", | ||
| _config.objectAccess | ||
| ); | ||
| return _out; | ||
| } | ||
|
|
||
| langutil::EVMVersion evmVersion; | ||
| std::optional<uint8_t> eofVersion; | ||
| bool objectAccess; | ||
| }; | ||
|
|
||
| inline std::vector<EVMDialectConfigurationToTest> generateEVMDialectConfigurationsToTest(std::optional<uint8_t> _eofVersion) | ||
| { | ||
| std::vector<EVMDialectConfigurationToTest> configs; | ||
| for (bool providesObjectAccess: {false, true}) | ||
| for (auto evmVersion: langutil::EVMVersion::allVersions()) | ||
| if (!_eofVersion || evmVersion >= langutil::EVMVersion::firstWithEOF()) | ||
| configs.push_back(EVMDialectConfigurationToTest{evmVersion, _eofVersion, providesObjectAccess}); | ||
| return configs; | ||
| } | ||
|
|
||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
For builtins based on opcodes, this part of the condition is the declarative part. I'd move it to a separate function that returns a boolean. Then when adding a new opcode we'd only really modify that function.
In fact, I'd actually create more than one function. I think we should make it more semantic by having smaller functions expressing specific properties the excluded opcodes satisfy:
SemanticInformation::isSwapInstruction()/SemanticInformation::isDupInstruction()here.RETF/CALLF: low-level control flow opcodesDATALOADN,EOFCREATE,RETURNCONTRACT: these are actually exposed, but not directly. We have builtins replacing them. We can define a separate function for them, but a nicer alternative would be to be able to declare for a builtin that it replaces an instruction and have the definition logic exclude that instruction automatically.Executing these functions and
EVMVersion::hasOpcode()would then be a fixed part of the definition logic and would not have to be adjusted. It would be the equivalent of checking the scope of a builtin.