Skip to content

Refactor compileViaYul test setting #15819

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 28 additions & 6 deletions test/TestCase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,17 +149,26 @@ void EVMVersionRestrictedTestCase::processBytecodeFormatSetting()
// EOF only available since Osaka
solAssert(!eofVersion.has_value() || solidity::test::CommonOptions::get().evmVersion().supportsEOF());

std::string bytecodeFormatString = m_reader.stringSetting("bytecodeFormat", "legacy,>=EOFv1");
if (bytecodeFormatString == "legacy,>=EOFv1" || bytecodeFormatString == ">=EOFv1,legacy")
auto const bytecodeFormatStr = m_reader.stringSetting("bytecodeFormat", "legacy,>=EOFv1");
if (bytecodeFormatStr == "legacy,>=EOFv1" || bytecodeFormatStr == ">=EOFv1,legacy")
m_bytecodeFormat = {BytecodeFormat::Legacy, BytecodeFormat::EOFv1};
else if (bytecodeFormatStr == "legacy")
m_bytecodeFormat = {BytecodeFormat::Legacy};
else if (bytecodeFormatStr == ">=EOFv1")
m_bytecodeFormat = {BytecodeFormat::EOFv1};
else if (bytecodeFormatStr.empty())
BOOST_THROW_EXCEPTION(std::runtime_error{"The 'bytecodeFormat' setting requires at least one value."});
else
BOOST_THROW_EXCEPTION(std::runtime_error{"Invalid bytecodeFormat flag: \"" + bytecodeFormatStr + "\""});

if (bytecodeFormat().contains(BytecodeFormat::Legacy) && bytecodeFormat().contains(BytecodeFormat::EOFv1))
return;

// TODO: This is naive implementation because for now we support only one EOF version.
if (bytecodeFormatString == "legacy" && eofVersion.has_value())
if (!bytecodeFormat().contains(BytecodeFormat::EOFv1) && eofVersion.has_value())
m_shouldRun = false;
else if (bytecodeFormatString == ">=EOFv1" && !eofVersion.has_value())
else if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && !eofVersion.has_value())
Copy link
Member

Choose a reason for hiding this comment

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

To clarify, this is what I meant by "same for the legacy run" in #15819 (comment):

Suggested change
else if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && !eofVersion.has_value())
else if (!bytecodeFormat().contains(BytecodeFormat::Legacy) && !eofVersion.has_value())

m_shouldRun = false;
else if (bytecodeFormatString != "legacy" && bytecodeFormatString != ">=EOFv1" )
BOOST_THROW_EXCEPTION(std::runtime_error{"Invalid bytecodeFormat flag: \"" + bytecodeFormatString + "\""});
}

EVMVersionRestrictedTestCase::EVMVersionRestrictedTestCase(std::string const& _filename):
Expand All @@ -168,3 +177,16 @@ EVMVersionRestrictedTestCase::EVMVersionRestrictedTestCase(std::string const& _f
processEVMVersionSetting();
processBytecodeFormatSetting();
}

std::ostream& solidity::frontend::test::operator<<(std::ostream& _output, CompileViaYul _compileViaYul)
{
switch (_compileViaYul)
{
case CompileViaYul::False: _output << "false"; break;
case CompileViaYul::True: _output << "true"; break;
case CompileViaYul::Also: _output << "also"; break;
case CompileViaYul::OnlyOnEOF: _output << "onlyOnEOF"; break;
}
return _output;
}

20 changes: 20 additions & 0 deletions test/TestCase.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,34 @@ class TestCase
bool m_shouldRun = true;
};

enum class BytecodeFormat
{
Legacy,
EOFv1
};

class EVMVersionRestrictedTestCase: public TestCase
{
private:
std::set<BytecodeFormat> m_bytecodeFormat;

void processEVMVersionSetting();
void processBytecodeFormatSetting();

protected:
std::set<BytecodeFormat> const& bytecodeFormat() const { return m_bytecodeFormat; }

EVMVersionRestrictedTestCase(std::string const& _filename);
};

enum class CompileViaYul
{
False,
True,
Also,
OnlyOnEOF,
};

std::ostream& operator<<(std::ostream& _output, CompileViaYul _compileViaYul);

}
30 changes: 22 additions & 8 deletions test/libsolidity/SemanticTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,8 @@ SemanticTest::SemanticTest(
m_enforceGasCost(_enforceGasCost),
m_enforceGasCostMinValue(std::move(_enforceGasCostMinValue))
{
static std::set<std::string> const compileViaYulAllowedValues{"also", "true", "false"};
static std::set<std::string> const yulRunTriggers{"also", "true"};
static std::set<std::string> const legacyRunTriggers{"also", "false", "default"};
static std::set<CompileViaYul> const yulRunTriggers{CompileViaYul::Also, CompileViaYul::True};
static std::set<CompileViaYul> const legacyRunTriggers{CompileViaYul::Also, CompileViaYul::False};

m_requiresYulOptimizer = m_reader.enumSetting<RequiresYulOptimizer>(
"requiresYulOptimizer",
Expand All @@ -92,18 +91,33 @@ SemanticTest::SemanticTest(
m_shouldRun = false;

auto const eofEnabled = solidity::test::CommonOptions::get().eofVersion().has_value();
std::string compileViaYul = m_reader.stringSetting("compileViaYul", eofEnabled ? "true" : "also");

if (compileViaYul == "false" && eofEnabled)
auto const compileViaYul = m_reader.enumSetting<CompileViaYul>(
"compileViaYul",
{
{toString(CompileViaYul::False), CompileViaYul::False},
{toString(CompileViaYul::True), CompileViaYul::True},
{toString(CompileViaYul::Also), CompileViaYul::Also},
{toString(CompileViaYul::OnlyOnEOF), CompileViaYul::OnlyOnEOF}
},
eofEnabled ? toString(CompileViaYul::True) : toString(CompileViaYul::Also)
);

if (compileViaYul == CompileViaYul::OnlyOnEOF)
solUnimplemented("'compileViaYul: onlyOnEOF' is not yet supported in semantic tests.");

if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && compileViaYul == CompileViaYul::False)
BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));

if (compileViaYul == CompileViaYul::False && eofEnabled)
m_shouldRun = false;
Comment on lines +109 to 113
Copy link
Member

Choose a reason for hiding this comment

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

The eofEnabled check should be converted to an assert, like in the syntax test. And we adjust the message the same way:

Suggested change
if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && compileViaYul == CompileViaYul::False)
BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));
if (compileViaYul == CompileViaYul::False && eofEnabled)
m_shouldRun = false;
if (m_compileViaYul == CompileViaYul::False)
{
soltestAssert(!eofEnabled);
if (bytecodeFormat().contains(BytecodeFormat::EOFv1))
BOOST_THROW_EXCEPTION(std::runtime_error(
"Conflicting test settings: 'bytecodeFormat' includes EOF, which requires compilation via IR, but 'compileViaYul' is set to false."
));
}


if (m_runWithABIEncoderV1Only && compileViaYul != "false")
if (m_runWithABIEncoderV1Only && compileViaYul != CompileViaYul::False)
BOOST_THROW_EXCEPTION(std::runtime_error(
"ABIEncoderV1Only tests cannot be run via yul, "
"so they need to also specify ``compileViaYul: false``"
));
if (!util::contains(compileViaYulAllowedValues, compileViaYul))
BOOST_THROW_EXCEPTION(std::runtime_error("Invalid compileViaYul value: " + compileViaYul + "."));

m_testCaseWantsYulRun = util::contains(yulRunTriggers, compileViaYul);
m_testCaseWantsLegacyRun = util::contains(legacyRunTriggers, compileViaYul);

Expand Down
26 changes: 17 additions & 9 deletions test/libsolidity/SyntaxTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,22 @@ SyntaxTest::SyntaxTest(
CommonSyntaxTest(_filename, _evmVersion),
m_minSeverity(_minSeverity)
{
static std::set<std::string> const compileViaYulAllowedValues{"true", "false"};

auto const eofEnabled = solidity::test::CommonOptions::get().eofVersion().has_value();
m_compileViaYul = m_reader.enumSetting<CompileViaYul>(
"compileViaYul",
{
{toString(CompileViaYul::False), CompileViaYul::False},
{toString(CompileViaYul::True), CompileViaYul::True},
{toString(CompileViaYul::Also), CompileViaYul::Also},
{toString(CompileViaYul::OnlyOnEOF), CompileViaYul::OnlyOnEOF}
},
toString(CompileViaYul::OnlyOnEOF)
);

m_compileViaYul = m_reader.stringSetting("compileViaYul", eofEnabled ? "true" : "false");
if (!util::contains(compileViaYulAllowedValues, m_compileViaYul))
BOOST_THROW_EXCEPTION(std::runtime_error("Invalid compileViaYul value: " + m_compileViaYul + "."));
solUnimplementedAssert(m_compileViaYul != CompileViaYul::Also,
"'compileViaYul: also' is not yet supported in syntax tests.");
Comment on lines +60 to +61
Copy link
Member

Choose a reason for hiding this comment

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

One left from #15819 (comment):

Suggested change
solUnimplementedAssert(m_compileViaYul != CompileViaYul::Also,
"'compileViaYul: also' is not yet supported in syntax tests.");
if (m_compileViaYul == CompileViaYul::Also)
solUnimplemented("'compileViaYul: also' is not yet supported in syntax tests.");


if (m_compileViaYul == "false" && eofEnabled)
m_shouldRun = false;
if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && m_compileViaYul == CompileViaYul::False)
BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));
Comment on lines +63 to +64
Copy link
Member

Choose a reason for hiding this comment

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

Still needs an assert.

While at it, let's also make the message a bit clearer. We should mention which settings it's about because the error won't point at a specific line in the test.

Suggested change
if (bytecodeFormat().contains(BytecodeFormat::EOFv1) && m_compileViaYul == CompileViaYul::False)
BOOST_THROW_EXCEPTION(std::runtime_error("Compilation to EOF requires using Yul IR"));
if (m_compileViaYul == CompileViaYul::False)
{
soltestAssert(!eofEnabled);
if (bytecodeFormat().contains(BytecodeFormat::EOFv1))
BOOST_THROW_EXCEPTION(std::runtime_error(
"Conflicting test settings: 'bytecodeFormat' includes EOF, which requires compilation via IR, but 'compileViaYul' is set to false."
));
}


m_optimiseYul = m_reader.boolSetting("optimize-yul", true);

Expand All @@ -80,7 +86,9 @@ void SyntaxTest::setupCompiler(CompilerStack& _compiler)
OptimiserSettings::full() :
OptimiserSettings::minimal()
);
_compiler.setViaIR(m_compileViaYul == "true");

auto const eofEnabled = solidity::test::CommonOptions::get().eofVersion().has_value();
_compiler.setViaIR(m_compileViaYul == CompileViaYul::True || (m_compileViaYul == CompileViaYul::OnlyOnEOF && eofEnabled));
_compiler.setMetadataFormat(CompilerStack::MetadataFormat::NoMetadata);
_compiler.setMetadataHash(CompilerStack::MetadataHash::None);
}
Expand Down
2 changes: 1 addition & 1 deletion test/libsolidity/SyntaxTest.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class SyntaxTest: public AnalysisFramework, public solidity::test::CommonSyntaxT
virtual void filterObtainedErrors();

bool m_optimiseYul{};
std::string m_compileViaYul{};
CompileViaYul m_compileViaYul{};
langutil::Error::Severity m_minSeverity{};
PipelineStage m_stopAfter;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f() -> 0x40, 0xa0, 0x40, 0x20, 0x0, 0x0
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f1() -> 0x20, 0x40, 0x20, 0
// f2(string): 0x20, 0 -> 0x20, 0x40, 0x20, 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(bool): true -> true
// f(bool): false -> false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ contract C {
}
// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(bool,bytes,bytes32[2]): true, 0x80, "a", "b", 4, "abcd" -> true, 0x80, "a", "b", 4, "abcd"
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(uint16,int16,address,bytes3,bool): 1, 2, 3, "a", true -> 1, 2, 3, "a", true
// f(uint16,int16,address,bytes3,bool): 0xffffff, 0x1ffff, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff, "abcd", 1 -> 0xffff, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff, 0xffffffffffffffffffffffffffffffffffffffff, "abc", true
Expand Down
1 change: 1 addition & 0 deletions test/libsolidity/semanticTests/abiEncoderV1/enums.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(uint8): 0 -> 0
// f(uint8): 1 -> 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(uint16,uint16): 65534, 0 -> 0xfffe
// f(uint16,uint16): 65536, 0 -> 0x00
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ contract C {
}
// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f() -> 0x6465616462656566000000000000000000000000000000000000000000000010
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ contract C {

// ====
// compileViaYul: false
// bytecodeFormat: legacy
Copy link
Member

Choose a reason for hiding this comment

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

// via yul disabled because of stack issues.

Can you try to remove compileViaYul: false here? The code does compile on 0.8.29 for me. If it still fails, I'd also try onlyOnEOF.

Copy link
Member

Choose a reason for hiding this comment

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

Same for negative_stack_height.sol.

// ----
// f1(bytes[1]): 0x20, 0x20, 0x3, hex"0102030000000000000000000000000000000000000000000000000000000000" -> 0x3, 0x1, 0x2, 0x3
// f2(bytes[1],bytes[1]): 0x40, 0xa0, 0x20, 0x3, hex"0102030000000000000000000000000000000000000000000000000000000000", 0x20, 0x2, hex"0102000000000000000000000000000000000000000000000000000000000000" -> 0x3, 0x1, 0x2, 0x3, 0x2, 0x1, 0x2
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(bool): 0x0 -> 0x0
// f(bool): 0x1 -> 0x1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(address): 0xffff1234567890123456789012345678901234567890 -> 0x0 # We input longer data on purpose.#
// g(address): 0xffff1234567890123456789012345678901234567890 -> 0x0
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ contract C {
}
// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f() -> 0xffffffff00000000000000000000000000000000000000000000000000000000
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(bytes2,uint16): "abc", 0x40102 -> 0x0 # We input longer data on purpose. #
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,6 @@ contract C {
}
// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// t() -> FAILURE
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ contract B is A {
}
// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// x() -> 4
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ contract C {

// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f() -> 10
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ contract C {

// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(bool): false -> 1
// f(bool): true -> 2
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ contract C {

// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(bool): false -> 1
// f(bool): true -> 2
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ contract C {
}
// ====
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// l(uint8): 64 -> 0x3930313233343536373839300000000000000000000000000000000000000000
// r(uint8): 64 -> 0x313233343536373839303132000000000000000000000000
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(int8,uint8): 0x00, 0x03 -> 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe
// f(int8,uint8): 0x00, 0x04 -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(uint8,uint8): 0x00, 0x04 -> 0x0f
// f(uint8,uint8): 0x00, 0x1004 -> 0x0f
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(int16,uint16): 0xff99, 0x00 -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff99
// f(int16,uint16): 0xff99, 0x01 -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffcc
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(int32,uint32): 0xffffff99, 0x00 -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff99
// f(int32,uint32): 0xffffff99, 0x01 -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffcc
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ contract C {
// ====
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(int8,uint8): 0x99, 0x00 -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff99
// f(int8,uint8): 0x99, 0x01 -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffcc
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ contract C {
// revertStrings: debug
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// d(bytes): 0x20, 0x01, 0x0000000000000000000000000000000000000000000000000000000000000000 -> FAILURE, hex"08c379a0", 0x20, 18, "Calldata too short"
1 change: 1 addition & 0 deletions test/libsolidity/semanticTests/revertStrings/empty_v1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ contract C {
// EVMVersion: >=byzantium
// revertStrings: debug
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f() -> FAILURE, hex"08c379a0", 0x20, 0
// g(string): 0x20, 0, "" -> FAILURE, hex"08c379a0", 0x20, 0
Expand Down
1 change: 1 addition & 0 deletions test/libsolidity/semanticTests/revertStrings/enum_v1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ contract C {
// revertStrings: debug
// ABIEncoderV1Only: true
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// f(uint8[]): 0x20, 2, 3, 3 -> FAILURE, hex"08c379a0", 0x20, 17, "Enum out of range"
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ contract C {
// ABIEncoderV1Only: true
// revertStrings: debug
// compileViaYul: false
// bytecodeFormat: legacy
// ----
// t(uint256) -> FAILURE, hex"08c379a0", 0x20, 0x12, "Calldata too short"
Loading