Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

eof: Support functions (CALLF, RETF, JUMPF) #15550

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

rodiazet
Copy link
Contributor

@rodiazet rodiazet commented Oct 29, 2024

Implement support for functions calls and returns for EOF.

Depends on: #15547

@rodiazet rodiazet marked this pull request as draft October 29, 2024 10:42
Copy link

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.

@rodiazet rodiazet force-pushed the eof-functions branch 12 times, most recently from 19c3760 to aa13c9c Compare October 30, 2024 12:43
@cameel cameel added has dependencies The PR depends on other PRs that must be merged first EOF labels Oct 30, 2024
@rodiazet rodiazet force-pushed the eof-functions branch 2 times, most recently from fd78a06 to 8ceb5c4 Compare October 30, 2024 15:00
@rodiazet rodiazet marked this pull request as ready for review October 30, 2024 15:04
@rodiazet rodiazet force-pushed the eof-functions branch 2 times, most recently from 436f00e to e4ab717 Compare October 30, 2024 21:18
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Just a few minor things from a quick pass over the PR.

Comment on lines 336 to 338
{Instruction::RETF, {"RETF", 0, 0, 0, true, Tier::Special}},
{Instruction::CALLF, {"CALLF", 2, 0, 0, true, Tier::Special}},
{Instruction::JUMPF, {"JUMPF", 2, 0, 0, true, Tier::Special}},
Copy link
Member

Choose a reason for hiding this comment

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

These should get dedicated tiers too, to match execution specs.

Copy link
Contributor Author

@rodiazet rodiazet Oct 31, 2024

Choose a reason for hiding this comment

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

uint8_t argsNum;
uint8_t retsNum;
};
std::optional<FunctionSignature> m_functionSignature; ///< Only valid if m_type == CallF or JumpF
Copy link
Member

Choose a reason for hiding this comment

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

This should have a getter, similarly to instruction(), with an assert that the item type allows it.

Comment on lines 72 to 78
AssemblyItem newFunctionCall(uint16_t _functionID)
{
solAssert(_functionID < m_codeSections.size(), "Call to undeclared function.");
solAssert(_functionID > 0, "Cannot call call section 0");
auto const& section = m_codeSections.at(_functionID);
if (section.outputs != 0x80)
return AssemblyItem::functionCall(_functionID, section.inputs, section.outputs);
else
return AssemblyItem::jumpF(_functionID, section.inputs);
}

AssemblyItem newFunctionReturn()
{
return AssemblyItem::functionReturn(m_codeSections.at(m_currentCodeSection).outputs);
}

uint16_t createFunction(uint8_t _args, uint8_t _rets)
{
size_t functionID = m_codeSections.size();
solAssert(functionID < 1024, "Too many functions.");
solAssert(m_currentCodeSection == 0, "Functions need to be declared from the main block.");
solAssert(_rets <= 0x80, "Too many function returns.");
m_codeSections.emplace_back(CodeSection{_args, _rets, {}});
return static_cast<uint16_t>(functionID);
}

void beginFunction(uint16_t _functionID)
{
solAssert(m_currentCodeSection == 0, "Atempted to begin a function before ending the last one.");
solAssert(_functionID < m_codeSections.size(), "Attempt to begin an undeclared function.");
auto& section = m_codeSections.at(_functionID);
solAssert(section.items.empty(), "Function already defined.");
m_currentCodeSection = _functionID;
}
void endFunction()
{
solAssert(m_currentCodeSection != 0, "End function without begin function.");
m_currentCodeSection = 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

These are long enough that I'd rather put them in the .cpp file.

Copy link
Contributor Author

@rodiazet rodiazet Oct 31, 2024

Choose a reason for hiding this comment

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

Right. Fixed. const added to two of them.

Comment on lines 1485 to 1543
case CallF:
{
ret.bytecode.push_back(static_cast<uint8_t>(Instruction::CALLF));
solAssert(item.data() <= std::numeric_limits<uint16_t>::max(), "Invalid callf index value.");
appendBigEndianUint16(ret.bytecode, item.data());
break;
}
case JumpF:
{
ret.bytecode.push_back(static_cast<uint8_t>(Instruction::JUMPF));
solAssert(item.data() <= std::numeric_limits<uint16_t>::max(), "Invalid jumpf index value.");
appendBigEndianUint16(ret.bytecode, item.data());
break;
}
case RetF:
{
ret.bytecode.push_back(static_cast<uint8_t>(Instruction::RETF));
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

This should have sanity checks that the numbers of inputs and outputs actually match the function. It's technically possible to create AssemblyItem without using the static helpers and get it wrong (or we might just forget when implementing asm import later). We should be able to catch this kind of mistake.

@rodiazet rodiazet force-pushed the eof-functions branch 2 times, most recently from 4f6d769 to 9a5d5a1 Compare October 31, 2024 12:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
EOF external contribution ⭐ has dependencies The PR depends on other PRs that must be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants