eof: Support relative jumps #15547

rodiazet · 2024-10-25T14:47:14Z

~~Depends on #15512~~ Merged

github-actions · 2024-10-25T14:47:29Z

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

I did an initial pass through the PR (without the libevmasm optimizer changes yet) and so far did not find any substantial problems. There are a few small things to correct and some suggestions.

libevmasm/Instruction.cpp

libevmasm/Instruction.h

libevmasm/PeepholeOptimiser.cpp

libevmasm/AssemblyItem.h

libevmasm/Assembly.cpp

cameel

Two things are still not right: missing gas tier for RJUMP and excluding 0x7fff as a relative offset. Other than that, just a few missing asserts and cosmetic changes.

libevmasm/Assembly.cpp

cameel · 2024-10-30T23:21:13Z

libevmasm/Assembly.cpp

+		solAssert(tagPos != std::numeric_limits<size_t>::max(), "Reference to tag without position.");
+
+		ptrdiff_t const relativeJumpOffset = static_cast<ptrdiff_t>(tagPos) - (static_cast<ptrdiff_t>(refPos) + 2);
+		solRequire(relativeJumpOffset < 0x7FFF && relativeJumpOffset >= -0x8000, AssemblyException, "Relative jump too far");


Also, we should think what we're going to do when this happens in practice. Do we just refuse to generate code for functions that are too long?

BTW, please also assert that the offset is not -1 or -2 since that would end up in the middle of the jump instruction itself.

Actually, it would be nice to have a more general sanity check that we're not landing in the middle of another multi-byte instruction as well, but I guess that one would be a bit complex so maybe not worth the effort. Having the offset generated from a tag should make this kind of mistake very unlikely anyway.

We could potentially take evmone (https://github.com/ethereum/evmone/blob/master/lib/evmone/eof.cpp#L887) verification code and adjust it to our case by adding proper reporting. It can be done after bytecode generation.

Sounds good but it would be enough as a sanity check.

Still does not solve the issue of functions that are too long though. I guess that will just be a code generation error? If so, we'll need to find a way to point at the function that caused it. Errors without source location are a pain for users (we have a lot of complaints about that with StackTooDeep).

libevmasm/AssemblyItem.cpp

libevmasm/Instruction.cpp

libevmasm/SemanticInformation.cpp

test/libyul/objectCompiler/eof/rjumps.yul

cameel · 2024-10-31T00:31:25Z

test/libyul/objectCompiler/eof/rjumps.yul

+// Bytecode: ef00010100040200010010040000000080ffff6001e1000460205ff360015f52e0fff5
+// Opcodes: 0xEF STOP ADD ADD STOP DIV MUL STOP ADD STOP LT DIV STOP STOP STOP STOP DUP1 SELFDESTRUCT SELFDESTRUCT PUSH1 0x1 RJUMPI 0x4 PUSH1 0x20 PUSH0 RETURN PUSH1 0x1 PUSH0 MSTORE RJUMP 0xFFF5


Would be nice to eventually have a test type that makes verifying this on a bytecode level less tedious. For now I checked it by hand and it seems correct:

RJUMP 0xFFF5 jumps 3 bytes ahead and 11 back, which gives 13 + 3 - 11 = 5 = tag_2

RJUMPI 0x4 jumps to 2 + 3 + 4 = 9 = tag_1

00: PUSH1 0x1 02: RJUMPI 0x4 05: PUSH1 0x20 // tag_2 07: PUSH0 08: RETURN 09: PUSH1 0x1 // tag_1 11: PUSH0 12: MSTORE 13: RJUMP 0xFFF5

I understand it as a comment for the future or we can do something with this now?

Yeah, for the future.

But your suggestion from #15547 (comment) to use validate_eof() sounds like a good and easy solution. We should assert that somewhere.

cameel

Just needs some cleanup and then we can merge: there are some typos in function names and a few other minor things.

libevmasm/Assembly.cpp

cameel · 2024-11-21T19:30:27Z

test/libyul/objectCompiler/eof/rjumps.yul

+// Bytecode: ef00010100040200010010040000000080ffff6001e1000460205ff360015f52e0fff5
+// Opcodes: 0xEF STOP ADD ADD STOP DIV MUL STOP ADD STOP LT DIV STOP STOP STOP STOP DUP1 SELFDESTRUCT SELFDESTRUCT PUSH1 0x1 RJUMPI 0x4 PUSH1 0x20 PUSH0 RETURN PUSH1 0x1 PUSH0 MSTORE RJUMP 0xFFF5


Yeah, for the future.

But your suggestion from #15547 (comment) to use validate_eof() sounds like a good and easy solution. We should assert that somewhere.

libevmasm/AssemblyItem.h

libevmasm/Assembly.cpp

cameel · 2024-11-21T20:11:29Z

libevmasm/SemanticInformation.cpp

@@ -613,6 +621,7 @@ bool SemanticInformation::invalidInPureFunctions(Instruction _instruction)

 bool SemanticInformation::invalidInViewFunctions(Instruction _instruction)
 {
+	// Relative jumps cannot jump out of the current code section of EOF so they are valid in view function.


Suggested change

// Relative jumps cannot jump out of the current code section of EOF so they are valid in view function.

// Relative jumps cannot jump out of the current code section of EOF so they are valid in view functions

// (under the assumption that every Solidity function actually gets its own code section).

What about this one?

OK. Missed this one

cameel · 2024-11-27T01:25:08Z

test/libyul/objectCompiler/eof/rjumps.yul

+//     /* "source":46:70   */
+//   rjumpi{tag_1}
+//     /* "source":22:112   */
+// tag_2:


@rodiazet I just noticed an odd thing while reviewing #15550. RJUMPI seems to always insert an extra tag that often goes unused. It did get used in the case you added here so this went unnoticed, but you can see it in objectCompiler/eof/functions.yul in the CALLF PR. And it's reproducible without functions as well:

test.yul:

{ if calldataload(0) { return(0, 0) } }

solc --strict-assembly test.yul --asm --debug-info none --experimental-eof-version 1 --evm-version prague

0x00 calldataload rjumpi{tag_1} tag_2: stop tag_1: 0x00 dup1 return

It does not happen with JUMPI so it must be something introduced by this PR:

solc --strict-assembly test.yul --asm --debug-info none --evm-version prague

0x00 calldataload tag_1 jumpi stop tag_1: 0x00 dup1 return

It's related to lack of optimizations for EOF. In this case PeepholeOptimizer is disabled for EOF. For legacy tag_2 is removed by the UnreachableCode optimizer step. I checked that for legacy the tag_2 is present in the code section 0 before optimization.

Ah, ok then. I didn't consider that it may be the optimizer's responsibility but it makes sense.

github-actions bot added the external contribution ⭐ label Oct 25, 2024

rodiazet force-pushed the eof-rjumps branch 3 times, most recently from 713beaa to 3c9e0f8 Compare October 25, 2024 15:10

cameel added has dependencies The PR depends on other PRs that must be merged first EOF labels Oct 25, 2024

cameel reviewed Oct 25, 2024

View reviewed changes

rodiazet force-pushed the eof-rjumps branch 4 times, most recently from ef51b68 to 19f9469 Compare October 28, 2024 16:26

rodiazet marked this pull request as ready for review October 29, 2024 09:37

rodiazet mentioned this pull request Oct 29, 2024

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

Merged

rodiazet force-pushed the eof-rjumps branch 3 times, most recently from b936bd0 to 3f08232 Compare October 30, 2024 11:43

cameel removed the has dependencies The PR depends on other PRs that must be merged first label Oct 30, 2024

rodiazet force-pushed the eof-rjumps branch from 3f08232 to 56b3d7b Compare October 30, 2024 15:00

cameel reviewed Oct 31, 2024

View reviewed changes

rodiazet force-pushed the eof-rjumps branch 2 times, most recently from 4506bff to 675680b Compare October 31, 2024 11:53

cameel reviewed Nov 21, 2024

View reviewed changes

rodiazet force-pushed the eof-rjumps branch 3 times, most recently from 64a6aa1 to eaab6b3 Compare November 22, 2024 09:53

cameel previously approved these changes Nov 22, 2024

View reviewed changes

eof: Implement relative jumps support

379b724

rodiazet dismissed cameel’s stale review via 379b724 November 22, 2024 12:26

rodiazet force-pushed the eof-rjumps branch from eaab6b3 to 379b724 Compare November 22, 2024 12:26

cameel approved these changes Nov 22, 2024

View reviewed changes

cameel enabled auto-merge November 22, 2024 12:31

cameel merged commit 4231717 into ethereum:develop Nov 22, 2024
73 checks passed

cameel reviewed Nov 27, 2024

View reviewed changes

rodiazet deleted the eof-rjumps branch December 9, 2024 21:41

cameel mentioned this pull request Dec 20, 2024

Some EOF-specific Yul builtin names are reserved even outside of EOF #15672

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eof: Support relative jumps #15547

eof: Support relative jumps #15547

rodiazet commented Oct 25, 2024 •

edited by cameel

Loading

github-actions bot commented Oct 25, 2024

cameel left a comment •

edited

Loading

cameel left a comment

This comment was marked as resolved.

cameel Oct 30, 2024

cameel Oct 31, 2024

rodiazet Oct 31, 2024

cameel Nov 21, 2024 •

edited

Loading

cameel Oct 31, 2024

rodiazet Oct 31, 2024 •

edited

Loading

cameel Nov 21, 2024

cameel left a comment

cameel Nov 21, 2024

cameel Nov 21, 2024

cameel Nov 22, 2024

rodiazet Nov 22, 2024

cameel Nov 27, 2024 •

edited

Loading

rodiazet Nov 27, 2024

cameel Nov 28, 2024

		// Bytecode: ef00010100040200010010040000000080ffff6001e1000460205ff360015f52e0fff5
		// Opcodes: 0xEF STOP ADD ADD STOP DIV MUL STOP ADD STOP LT DIV STOP STOP STOP STOP DUP1 SELFDESTRUCT SELFDESTRUCT PUSH1 0x1 RJUMPI 0x4 PUSH1 0x20 PUSH0 RETURN PUSH1 0x1 PUSH0 MSTORE RJUMP 0xFFF5

	// Relative jumps cannot jump out of the current code section of EOF so they are valid in view function.
	// Relative jumps cannot jump out of the current code section of EOF so they are valid in view functions
	// (under the assumption that every Solidity function actually gets its own code section).

eof: Support relative jumps #15547

eof: Support relative jumps #15547

Conversation

rodiazet commented Oct 25, 2024 • edited by cameel Loading

github-actions bot commented Oct 25, 2024

cameel left a comment • edited Loading

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodiazet Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodiazet commented Oct 25, 2024 •

edited by cameel

Loading

cameel left a comment •

edited

Loading

cameel Nov 21, 2024 •

edited

Loading

rodiazet Oct 31, 2024 •

edited

Loading

cameel Nov 27, 2024 •

edited

Loading