Side-effect-free Yul interpreter

[quangloc99]

This interpreter implementation is following this specification from #15435 , and is intended to use in #15358 .

This PR has followed spec for the implementation.

For testing, the existing YulInterpreterTest was not used. This is because this pure interpreter has no memory/storage access, it can not use the same trace as the current interpreter from 'test/libyul/tools'. But to keep it minimal, the YulInterpreterTest test suit was copied and applied minimal changes. The new test suit now uses the function call trace and the outter most variable values for snapshot comparison. Most of the existing test from the YulInterpreterTest was ported to the new test suit. New tests were also added to test all return status, as well as to test all evm instructions.

[github-actions]

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.

[cameel]

Thanks for the PR! I won't manage to do a proper review until next week, but for now here's some initial feedback from a quick pass over it.

[cameel]

Instead of hard-coding those, you should just check that the instruction field in BuiltinFunctionForEVM is empty.

[cameel]

For testing, the existing YulInterpreterTest was not used. This is because this pure interpreter has no memory/storage access, it can not use the same trace as the current interpreter from 'test/libyul/tools'.

That's actually easy to solve. Just add a setting you can set to mark a test as one that requires access to memory or storage. Then add it to those tests that do not pass without it. As you can see in the snippet above, adding settings is pretty easy.

For this we could call the setting sideEffectFree and it true by default.

[quangloc99]

I understand how to mark a test that requires memory access. However, I’d still insist to keep the two test suites separate. While they do share the parsing part, the settings and trace printing are quite different. I’m concerned that merging the two might make it harder to manage the tests.

I also believe the two interpreter implementations might grow in different directions, so separating the test suites makes more sense to me.

[cameel]

Some general notes on style to get that out of the way:

• We always put return on a new line.
• Please also avoid abbreviations in names. Things like fun, cnt, x, g, vec, res are hard to read and often ambiguous. TBH, we have a lot of bad names already since the code is old and it's also hard to enforce, so some of that might have been just copied, but when writing new code, please try to avoid them.
• Constructor spacing: ExecutionOk{ControlFlowState::Default};
• 4-space indents, including in .yul files.
• You don't have to include the empty string as message in asserts. We made it optional at some point, but did not strip it from everywhere. When writing new code, you can omit the message.
• I'd avoid making files hidden (with a leading dot in the name).
• In Python code we use the same style for splitting long calls as in C++. I.e.

      gen_test(
          'sgt',
          param_cnt=2,
          calc=lambda p: u2s(p[0]) > u2s(p[1])
      )

[quangloc99]

Thank you for commenting on the styling. For this part, I have a question about the formatter.

For C++ code style, I see that there is .clang-format configuration included. However, using clang-format changes the code drastically. So I deliberately turned off the formatter. Should I use clang format for the new code, or should I try to adjust the code manually? And also the same question applied to Python code.

The is the only questions. I will address the other comments.

[quangloc99]

Besides the code style, is it OK to use macro for the terminated case? I think that check is repetitive, and because it also return immediately, I can only think of macro to clean up the code.

[cameel]

Suggested change

	case Instruction::SWAP16:
	{
	yulAssert(false, "");
	return EvaluationOk(0);
	}
	}
	yulAssert(false, "Unknown instruction with opcode " + std::to_string(static_cast<uint8_t>(_instruction)));
	return EvaluationOk(0);
	case Instruction::SWAP16:
	yulAssert(false, "Instruction not allowed in strict assembly.");
	}
	util::unreachable();

[quangloc99]

Thank you for pointing out the util::unreachable. I actually don't know about it.

However, I think the assert with error message should be used. Currently all the instruction are checked with a switch statement, which is not guaranteed to be exhaustive check at compile time. I am not sure if there is a way to ensure its correctness at compile time. For the runtime check, I think it should fail loudly. When failure happens, there must be a new instruction. It should then be added to this function.

[cameel]

Are they though? I'd expect dataoffset(), datasize() and linkersymbol() at least to be pure. We won't be able to evaluate them without information that's only available at bytecode generation time, but technically they end up being just constants :)

[quangloc99]

I understand the point. I think my comment here is misleading too, so I will change that.

I think for the implementation, we can try returning special values for these cases. But that sounds more complicated, and it is go beyond the point of code interpretation.

[cameel]

I'd drop the tools/ dir and put it simply under interpreter/. The old interpreter is under test/tools/ because it's a part of yulrun, which is a testing tool (and compiles into a separate executable). The new interpreter becomes a part of libyul instead, which goes into solc.

[cameel]

Why not place this next to the other types like this we already have, like u256?

[quangloc99]

I did not notice that type was there. I was too focused on modifying the original interpreter 🙏 .

I will move it.