eof: Fix EOF builtin names unintentionally reserved outside of EOF #15700
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
7c98754 to
4e697bf
Compare
| function dataloadn() {} | ||
| function rjump() {} | ||
| function rjumpi() {} | ||
| function callf() {} | ||
| function jumpf() {} | ||
| function retf() {} |
There was a problem hiding this comment.
Are any of these implemented in EOF, i.e. callable, like ext* calls?
There was a problem hiding this comment.
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.
4e697bf to
a6c3c05
Compare
libyul/AsmAnalysis.cpp
Outdated
| _instr != evmasm::Instruction::RETF, | ||
| ""); |
There was a problem hiding this comment.
| _instr != evmasm::Instruction::RETF, | |
| ""); | |
| _instr != evmasm::Instruction::RETF | |
| ); |
libyul/AsmAnalysis.cpp
Outdated
| _instr == evmasm::Instruction::EXTDELEGATECALL || | ||
| _instr == evmasm::Instruction::EXTSTATICCALL | ||
| _instr == evmasm::Instruction::EXTSTATICCALL || | ||
| _instr == evmasm::Instruction::EXTDELEGATECALL |
There was a problem hiding this comment.
This doesn't change anything, does it?
| object "a" { | ||
| code { | ||
| function auxdataloadn() {} |
There was a problem hiding this comment.
Just a general note: in most types of Yul tests you don't have to include the object boilerplate. I'd remove it to make tests simpler:
| object "a" { | |
| code { | |
| function auxdataloadn() {} | |
| { | |
| function auxdataloadn() {} |
I think the only place where it might be required is objectCompilerTests. And I'm not even 100% sure about that.
| // bytecodeFormat: >=EOFv1 | ||
| // ---- | ||
| // ParserError 5568: (41-53): Cannot use builtin function name "auxdataloadn" as identifier name. | ||
| // ParserError 8143: (41-53): Expected keyword "data" or "object" or "}". |
There was a problem hiding this comment.
Looks like for some reason a clash with a builtin name is a fatal error. There's no good reason for that. I submitted a PR to fix that: #15712. When it's merged, you'll be able to put all the EOF builtins in a single test.
There was a problem hiding this comment.
Worth noting that when I changed the test to version without object and code section. The test results looks differently. Not sure if it's expected behavior.
yulSyntaxTests/eof/auxdataloadn_reserved_in_eof.yul: FAIL
Contract:
{
function auxdataloadn() {}
}
// ====
// bytecodeFormat: >=EOFv1
Expected result:
ParserError 5568: (41-53): Cannot use builtin function name "auxdataloadn" as identifier name.
ParserError 8143: (41-53): Expected keyword "data" or "object" or "}".
Obtained result:
ParserError 5568: (15-27): Cannot use builtin function name "auxdataloadn" as identifier name.
| // DeclarationError 4619: (32-44): Function "auxdataloadn" not found. | ||
| // DeclarationError 4619: (56-65): Function "dataloadn" not found. | ||
| // DeclarationError 4619: (77-86): Function "eofcreate" not found. | ||
| // DeclarationError 4619: (115-129): Function "returncontract" not found. |
There was a problem hiding this comment.
auxdataloadn, eofcreate, returncontract should also get an error saying they're only available in EOF.
There was a problem hiding this comment.
It has to be handled separately in AsmAnalysis.cpp. It doesn't pass validateInstructions check because they are builtins. I will add some additional if to handle these too.
a6c3c05 to
577e2cc
Compare
Resolves: #15672