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

eof: Fix EOF builtin names unintentionally reserved outside of EOF #15700

Merged
merged 4 commits into from
Jan 23, 2025

Conversation

rodiazet
Copy link
Contributor

@rodiazet rodiazet commented Jan 9, 2025

Resolves: #15672

This comment was marked as resolved.

@rodiazet rodiazet force-pushed the eof-reserved-names-fix branch from 7c98754 to 4e697bf Compare January 9, 2025 11:29
Comment on lines 3 to 8
function dataloadn() {}
function rjump() {}
function rjumpi() {}
function callf() {}
function jumpf() {}
function retf() {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are any of these implemented in EOF, i.e. callable, like ext* calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not callable from yul level, but legacy jump and jumpi are reserved but also not callable from yul. So I assumed they should be reserved also.

libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
@rodiazet rodiazet force-pushed the eof-reserved-names-fix branch 2 times, most recently from 577e2cc to d597ad7 Compare January 20, 2025 12:41
Comment on lines 161 to 173
(
_instr == evmasm::Instruction::EXTCALL ||
_instr == evmasm::Instruction::EXTSTATICCALL ||
_instr == evmasm::Instruction::EXTDELEGATECALL
_instr == evmasm::Instruction::EXTDELEGATECALL ||
_instr == evmasm::Instruction::DATALOADN ||
_instr == evmasm::Instruction::EOFCREATE ||
_instr == evmasm::Instruction::RETURNCONTRACT ||
_instr == evmasm::Instruction::RJUMP ||
_instr == evmasm::Instruction::RJUMPI ||
_instr == evmasm::Instruction::CALLF ||
_instr == evmasm::Instruction::JUMPF ||
_instr == evmasm::Instruction::RETF
);
Copy link
Member

Choose a reason for hiding this comment

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

You could use hasOpcode() to avoid repeating these:

EVMVersion::prague().hasOpcode(_instr, 1) && !EVMVersion::prague().hasOpcode(_instr, std::nullopt)

With an extra assert that EOF version is 1 and EVM version is <= Prague because this will need to be adjusted once new versions are out (if they and add/remove instructions).

Copy link
Member

Choose a reason for hiding this comment

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

BTW, validateInstructions() should have such an assert too, because it hard-coded EOF version to 1 and we can easily forget to update it once we have EOF v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make it clear.
Function eofIdentifiersException returns true if identifier based on an instruction should not be reserved. false otherwise. So in case when _eofVersion == 1 this function should always return false and in case when _eofVersion == std::nullopt we should omit reserving EOF specific identifiers related to EOF instructions.
Right? I think it would be better to change the assumption of these functions to return false on "do not reserve" and true otherwise. It's very confusing now.

Copy link
Member

Choose a reason for hiding this comment

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

The functions say whether to apply the exception from the rule (which is to treat instruction names as reserved identifiers). The current return values do make sense to me with that naming.

I guess we could rename them if that's not clear. I'd actually consolidate them all into one check and call it something like validAsIdentifierForBackwardsCompatibility() or validAsIdentifierOnCurrentEVM(). We could then make the older checks more general as well, by just checking if it's an instruction that exists on current EVM but not on the latest one.

If you want to do that, I'd do it as a separate refactor PR though.

libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
@cameel cameel added the EOF label Jan 21, 2025
@rodiazet rodiazet force-pushed the eof-reserved-names-fix branch 2 times, most recently from b979b96 to 2b1372c Compare January 22, 2025 14:09
cameel
cameel previously approved these changes Jan 22, 2025
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.

Looks good, just needs some final tweaks before we merge (consolidate tests, style fix, squash review fixes).

libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
@rodiazet rodiazet force-pushed the eof-reserved-names-fix branch from 41e4d98 to 9032d58 Compare January 23, 2025 13:23
@cameel cameel merged commit feaf48a into ethereum:develop Jan 23, 2025
73 checks passed
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.

Some EOF-specific Yul builtin names are reserved even outside of EOF
3 participants