eof: Pass `eofVersion` flag to `EVMDialect` #15467

rodiazet · 2024-09-30T13:00:49Z

Some of the future PRs implementing EOF instructions required to use eofVersion flag in the EVMDialect class. This unfortunately introduces a lot of changes in the code base.

github-actions · 2024-09-30T13:01:06Z

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.

libsolidity/codegen/CompilerContext.h

cameel

I found some problems, the biggest one being that keys for cached dialects and optimized IR do not include EOF version. I'm actually surprised that the former does not break anything in CI due to the instructions that were already marked as unavailable on EOF.

There's a few more things but overall it should all be easy to fix.

libevmasm/Assembly.cpp

liblangutil/EVMVersion.cpp

cameel · 2024-09-30T17:49:45Z

libsolidity/ast/ASTJsonImporter.cpp

-	yul::Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(evmVersion.value());
+	std::optional<uint8_t> eofVersion;
+	if (auto const it = _node.find("eofVersion"); it != _node.end())
+		eofVersion = it->get<uint8_t>();
+	astAssert(m_eofVersion == eofVersion, "Imported tree EOF version differs from configured EOF version!");
+
+	yul::Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(evmVersion.value(), eofVersion);


This adds support for reading the field, but it's useless if we never write it. You should add it to AST exporter as well.

BTW, having an EVM version field here seemed odd to me at first, and looking at the PR where it was introduced, I actually found comments indicating that it shouldn't be there in the first place (#7153 (comment), #7153 (comment), #7153 (review)). It was kept though. I suspect that the motivation might have been that Yul parsing depends on the EVM version to some extent, so if you parse with some version, export and import with a different version, you may get wrong AST structure or bypass some checks for that version.

It's weird that this is done for each assembly block though rather than the AST as a whole. If we add some new version-dependent feature in the parser, there will be no such check and no one will remember to add it. Looks like it actually already happened with the experimental pragma. We should consider changing that (it's an unrelated issue though, just FYI).

libyul/ObjectOptimizer.h

libyul/backends/evm/EVMDialect.h

libyul/backends/evm/EVMDialect.cpp

test/libyul/Common.cpp

cameel · 2024-09-30T19:24:08Z

test/libyul/EVMCodeTransformTest.cpp

 	evmasm::Assembly assembly{solidity::test::CommonOptions::get().evmVersion(), false, std::nullopt, {}};
 	EthAssemblyAdapter adapter(assembly);
 	EVMObjectCompiler::compile(
 		*stack.parserResult(),
 		adapter,
-		EVMDialect::strictAssemblyForEVMObjects(EVMVersion{}),
+		// TODO: Make sure that why we cannot pass here solidity::test::CommonOptions::get().evmVersion() and assembly.eofVersion()


We probably can and should. Unless it's not prepared to handle some versions, but I thought it was actually the optimized transform that had that limitation.

But anyway, every test which is hard-coded to use the default EVM version is also fine to leave hardcoded for non-EOF for now.

It would be best to change it to the version from CommonOptions where possible, but I'd consider that a separate enhancement (still worth doing, but optional - we're at least not making the test worse than it was). I'd just make sure that we properly update those that were already respecting versions from the CLI.

BTW, note that currently in CI we're always running tests without setting EOF version via command line. We should add that eventually in soltest_all.sh, next to where we loop over EVM versions. But not sure if the implementation is ready to handle that already. May be running into some asserts on incomplete stuff.

cameel · 2024-09-30T19:39:33Z

test/libyul/ObjectParser.cpp

@@ -112,7 +112,8 @@ std::tuple<std::optional<SourceNameMap>, ErrorList> tryGetSourceLocationMapping(

 	ErrorList errors;
 	ErrorReporter reporter(errors);
-	Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(EVMVersion::berlin());
+	// TODO: Add EOF support
+	Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(EVMVersion::berlin(), std::nullopt);


Not sure why this one is hard-coded to berlin, but it should at least be the default version. I'd change it to that if it won't test fail:

Suggested change

Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(EVMVersion::berlin(), std::nullopt);

Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(EVMVersion{}, std::nullopt);

Ideally we should later change it to properly use the version from CommonOptions.

BTW: I'd put any changes related to EVM version at least in a separate commit, since they're really unrelated to introduction of EOF version.

cameel · 2024-09-30T19:54:25Z

libyul/AsmAnalysis.cpp

 	// NOTE: This function uses the default EVM version instead of the currently selected one.
-	auto const builtin = EVMDialect::strictAssemblyForEVM(EVMVersion{}).builtin(YulName(_instructionIdentifier));
+	// TODO: Add EOF support
+	auto const builtin = EVMDialect::strictAssemblyForEVM(EVMVersion{}, std::nullopt).builtin(YulName(_instructionIdentifier));
 	if (builtin && builtin->instruction.has_value())
 		return validateInstructions(builtin->instruction.value(), _location);


The reason we use hard-coded EVM version here is that historically this was always the one that had all possible builtins. With EOF this changes so I guess we'll have to use a different strategy for checking if the thing is a builtin - we should check both EOF and non-EOF dialect here and call validateInstruction() for whichever has the instruction (or maybe even for both).

Let me know if you want this to be added in this PR

It would probably be best to still do it here. I wouldn't be against doing it later in another PR in principle, but we have so many of these TODOs now that I'm a bit afraid that things like this will just get forgotten and lost, only to surface as bugs much later :)

Done. But we need test for this. Current SyntaxTest does not support compileToEOF flag. I would add it to EVMVersionRestrictedTestCase constructor to have it available in all derived TaseCase classes. WDYT? If so here on additional PR? :)

It has to be done in this PR because CI requires to cover all possible error codes

Either way sounds fine, but it's related so having it here is probably a good idea. And yeah, the error code - we have a whitelist for unused codes in scripts/error_codes.py so you could temporarily add it there if necessary, but that's always a last resort :)

Though one thing to consider here - we have two different approaches to these settings, and you need to choose one:

EVMVersion setting: you set a supported range of versions in the test, but a single isoltest run always executes only for one specific version (selected via --evm-version). If that matches the range, the test runs. Otherwise it's skipped. To get full coverage we run soltest with all relevant EVM versions in CI (via soltest_all.sh).

compileViaYul setting in semantic tests: you select compilation via Yul or not or both. And each run does exactly that, running it twice if necessary. There's no CLI option.

The downside of (1) is that you'll have to run tests twice locally to test both EOF and legacy. It also adds another dimension to our test matrix. Currently we have two dimensions: optimizer on/off and EVM version. EOF version will be the third one, so you'll basically have to multiply the number of runs by two (though fortunately we can exclude older EVM versions from that, which lowers that number significantly).

Also, with (1) we end up unnecessarily running tests that are not affected by the flag multiple times, so it's only good for things that affect almost every part of the compiler.

The upside is that you get it supported almost for free in all test cases just by inheriting from EVMVersionRestrictedTestCase, while compiling things multiple times for (2) requires adding special support for it in each case. This matters especially for the smaller test cases (i.e. the ones where you used the flag from CommonOptions), where we definitely won't be going out of our way to make them use EOF, but it would be nice to have for more comprehensive coverage.

Since we already have that --eof-version flag in soltest and the option is closely related to EVM version, I think it still makes the most sense to go with (1). And we'll need to add that additional run with EOF enabled anyway, because otherwise the tests using EOF versions from CommonOptions now would still only be tested with legacy.

So, in short, my suggestion would be to extend EVMVersionRestrictedTestCase to make it support a setting like EOFVersion, which takes a list of comma-separated values: for now only legacy and >=1. The values should really be ranges, like for EVM, but since we only have one version, I'd not bother with that - I'd just make it support >=1 as a single, hard-coded value. If we ever get EOF v2, it will be easy to extend it by just changing how the value is parsed, without having to update it in all the tests.

We agreed on addind EOF support for AsmAnalysis in a separated PR #15471

cameel

Approved. Could use a few small tweaks (see comments below), but other than that it's good.

libyul/ObjectOptimizer.cpp

cameel · 2024-10-01T18:19:46Z

test/libsolidity/Assembly.cpp

+	Parser parser(
+		errorReporter,
+		solidity::test::CommonOptions::get().evmVersion(),
+		solidity::test::CommonOptions::get().eofVersion());


Suggested change

solidity::test::CommonOptions::get().eofVersion());

solidity::test::CommonOptions::get().eofVersion()

);

Same in all the dialect() calls you modified in tests.

cameel · 2024-10-09T18:17:53Z

liblangutil/EVMVersion.cpp

 	case Instruction::STATICCALL:
-		return hasStaticCall();
+		return !_eofVersion.has_value() && hasStaticCall();
 	case Instruction::SHL:
 	case Instruction::SHR:
 	case Instruction::SAR:
 		return hasBitwiseShifting();
 	case Instruction::CREATE2:
-		return hasCreate2();
+		return !_eofVersion.has_value() && hasCreate2();
 	case Instruction::EXTCODEHASH:
-		return hasExtCodeHash();
+		return !_eofVersion.has_value() && hasExtCodeHash();


We should make the EOF version a parameter in hasStaticCall(), hasCreate2() and hasExtCodeHash(). I wasn't sure about it back when I reviewed this PR, but now I saw this bit again in #15456 and I think it is necessary after all.

I actually just tried to do this myself as a quick tweak, but looks like it requires the EOF version to be passed into a few more places. And in some of those places it actually needs to be handled since they make assumptions based on the availability of these instructions. For example there are fallback to call when staticcall is not available, but on EOF they should result in extstaticcall instead.

github-actions bot added the external contribution ⭐ label Sep 30, 2024

rodiazet mentioned this pull request Sep 30, 2024

eof: Support DATALOADN (EIP-7480) #15456

Merged

rodiazet force-pushed the eof-pass-flag-to-evm-dialect branch 3 times, most recently from d2f6a89 to 60bfab0 Compare September 30, 2024 15:22

ekpyron reviewed Sep 30, 2024

View reviewed changes

libsolidity/codegen/CompilerContext.h Outdated Show resolved Hide resolved

cameel added the EOF label Sep 30, 2024

cameel requested changes Sep 30, 2024

View reviewed changes

rodiazet force-pushed the eof-pass-flag-to-evm-dialect branch 3 times, most recently from ccd9d66 to 0b4fdb9 Compare October 1, 2024 16:20

rodiazet mentioned this pull request Oct 1, 2024

eof: Support EOF asm analysis. #15471

Merged

rodiazet force-pushed the eof-pass-flag-to-evm-dialect branch from 0b4fdb9 to dedc058 Compare October 1, 2024 16:28

cameel previously approved these changes Oct 1, 2024

View reviewed changes

eof: Pass eofVersion flag to EVMDialect

8d8ac1c

rodiazet dismissed cameel’s stale review via 8d8ac1c October 1, 2024 20:29

rodiazet force-pushed the eof-pass-flag-to-evm-dialect branch from dedc058 to 8d8ac1c Compare October 1, 2024 20:29

cameel approved these changes Oct 1, 2024

View reviewed changes

cameel merged commit 4ab3e36 into ethereum:develop Oct 1, 2024
72 checks passed

cameel reviewed Oct 9, 2024

View reviewed changes

This was referenced Oct 10, 2024

test: Pass evmVersion and eofVersion to YulStack in ObjectCompilerTest #15503

Merged

Side-effect-free Yul interpreter #15464

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eof: Pass `eofVersion` flag to `EVMDialect` #15467

eof: Pass `eofVersion` flag to `EVMDialect` #15467

rodiazet commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

cameel left a comment •

edited

Loading

cameel Sep 30, 2024

cameel Sep 30, 2024

cameel Sep 30, 2024

cameel Sep 30, 2024

cameel Sep 30, 2024

cameel Sep 30, 2024

cameel Sep 30, 2024

rodiazet Oct 1, 2024

cameel Oct 1, 2024 •

edited

Loading

rodiazet Oct 1, 2024

rodiazet Oct 1, 2024

cameel Oct 1, 2024

cameel Oct 1, 2024 •

edited

Loading

cameel Oct 1, 2024

rodiazet Oct 1, 2024 •

edited

Loading

cameel left a comment

cameel Oct 1, 2024

cameel Oct 1, 2024

cameel Oct 9, 2024

	Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(EVMVersion::berlin(), std::nullopt);
	Dialect const& dialect = yul::EVMDialect::strictAssemblyForEVM(EVMVersion{}, std::nullopt);

	solidity::test::CommonOptions::get().eofVersion());
	solidity::test::CommonOptions::get().eofVersion()
	);

eof: Pass eofVersion flag to EVMDialect #15467

eof: Pass eofVersion flag to EVMDialect #15467

Conversation

rodiazet commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

cameel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodiazet Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eof: Pass `eofVersion` flag to `EVMDialect` #15467

eof: Pass `eofVersion` flag to `EVMDialect` #15467

cameel left a comment •

edited

Loading

cameel Oct 1, 2024 •

edited

Loading

cameel Oct 1, 2024 •

edited

Loading

rodiazet Oct 1, 2024 •

edited

Loading