Skip to content
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

Testing enhancements for MCOPY #14790

Merged
merged 6 commits into from
Jan 22, 2024
Merged

Testing enhancements for MCOPY #14790

merged 6 commits into from
Jan 22, 2024

Conversation

cameel
Copy link
Member

@cameel cameel commented Jan 20, 2024

Prerequisite for #14779.

A bunch of enhancements to the test framework that I did while working on MSIZE. Some of my tests depend on them:

  • Restricting minimum EVM version in boost tests and memory guard tests.
  • Memory guard tests were ignoring EVM version selected on the command line.
  • Memory guard tests were hiding the errors from Yul parsing, which includes ones you get when you try to use an opcode that's not available on the selected EVM version.
  • All tests in GasMeter.cpp were apparently disabled on abicoder v2 in Run all end-to-end tests with ABIEncoderV2 #5102. Most of them work fine though, so I changed it back so that both v1 and v2 run by default and only those few tests that are broken have a bypass.
  • Small formatting tweak in a few AnalysisFramework based test to make them print errors properly prefixed with the right indent.
  • Small EVMInstructionInterpreter.h refactors that I originally had in Yul builtin for MCOPY #14779.

@cameel cameel self-assigned this Jan 20, 2024
@cameel cameel mentioned this pull request Jan 20, 2024
8 tasks
@cameel cameel force-pushed the testing-tweaks-before-mcopy branch from 2222ea4 to b26d5fd Compare January 20, 2024 03:37
nikola-matic
nikola-matic previously approved these changes Jan 20, 2024
test/Common.cpp Outdated Show resolved Hide resolved
@@ -58,13 +61,14 @@ TestCase::TestResult MemoryGuardTest::run(std::ostream& _stream, std::string con
ErrorList errors;
auto [object, analysisInfo] = yul::test::parse(
compiler().yulIR(contractName),
EVMDialect::strictAssemblyForEVMObjects({}),
EVMDialect::strictAssemblyForEVMObjects(CommonOptions::get().evmVersion()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I assumed we had no memory guards tests with anything other than the default version then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we didn't. I don't think it's dependent on EVM version in a significant way though. Even now, I only had to change it because this preparatory Yul parsing step won't parse mcopy on earlier EVMs and not because of anything specifically related to the memory guard.

@cameel cameel mentioned this pull request Jan 22, 2024
@@ -129,6 +119,9 @@ BOOST_AUTO_TEST_CASE(simple_contract)
BOOST_AUTO_TEST_CASE(store_keccak256)
{
char const* sourceCode = R"(
// TODO: We should enable v2 again once the yul optimizer is activated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, but where the yul optimizer was deactivated? Should we mention a specific open issue?

Copy link
Member Author

@cameel cameel Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. The only context I have is #5102 and there's no discussion of that in there. My assumption is that abicoder v2 encoder must be using some EVM feature that GasMeter did not properly account for at that time and the assumption was that it would no longer be a problem when it becomes the default.

I don't think we have an issue for it. Actually, could you create one?

@cameel cameel merged commit f05d0a9 into develop Jan 22, 2024
65 checks passed
@cameel cameel deleted the testing-tweaks-before-mcopy branch January 22, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants