eof: `auxdatastore` as a tool to store immutables in data section of EOF #15521

rodiazet · 2024-10-17T15:05:59Z

Depends on: #15529

github-actions · 2024-10-17T15:06:14Z

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'm not done with this yet, but I already have some comments so I thought I may just as well post them already since it's late.

cameel · 2024-10-18T18:03:32Z

libevmasm/Assembly.h

@@ -91,8 +92,10 @@ class Assembly
 	void appendProgramSize() { append(AssemblyItem(PushProgramSize)); }
 	void appendLibraryAddress(std::string const& _identifier) { append(newPushLibraryAddress(_identifier)); }
 	void appendImmutable(std::string const& _identifier) { append(newPushImmutable(_identifier)); }
-	void appendImmutableAssignment(std::string const& _identifier) { append(newImmutableAssignment(_identifier)); }
+	void appendImmutableAssignment(std::string const& _identifier) { append(newImmutableAssignment(_identifier));}


Suggested change

void appendImmutableAssignment(std::string const& _identifier) { append(newImmutableAssignment(_identifier));}

void appendImmutableAssignment(std::string const& _identifier) { append(newImmutableAssignment(_identifier)); }

cameel · 2024-10-18T18:04:40Z

libevmasm/AssemblyItem.h

@@ -55,6 +55,8 @@ enum AssemblyItemType
 	/// Loads 32 bytes from static auxiliary data of EOF data section. The offset does *not* have to be always from the beginning
 	/// of the data EOF section. More details here: https://github.com/ipsilon/eof/blob/main/spec/eof.md#data-section-lifecycle
 	AuxDataLoadN,
+	/// Stores value in a memory which which will be appended to EOF data section as static axuliary data.


Suggested change

/// Stores value in a memory which which will be appended to EOF data section as static axuliary data.

/// Stores value in memory which will be appended to EOF data section as static auxiliary data.

cameel · 2024-10-18T18:05:38Z

libevmasm/Assembly.cpp

+			assertThrow(
+				storesAuxData != loadsAuxData,
+				AssemblyException,
+				"Cannot store and load auxilairy data in the same assembly subroutine."


Suggested change

"Cannot store and load auxilairy data in the same assembly subroutine."

"Cannot store and load auxiliary data in the same assembly subroutine."

Also, please use solRequire() instead of assertThrow().

Yes. Sorry. Legacy from assembleLegacy :)

cameel · 2024-10-18T18:09:36Z

libevmasm/Assembly.cpp

+	bool storesAuxData = false;
+	bool loadsAuxData = false;


These should be inside the loop. Otherwise true from the previous section will carry
over to the next one, and I think that was not the intention here?

I think it's ok. We want to make sure that there is no container which stores and loads auxiliary data. So storing container is init container and loading is runtime container.

cameel · 2024-10-18T18:16:28Z

libyul/backends/evm/EVMDialect.cpp

@@ -173,6 +173,8 @@ std::set<YulName> createReservedIdentifiers(langutil::EVMVersion _evmVersion)
 		"datacopy"_yulname,
 		"setimmutable"_yulname,
 		"loadimmutable"_yulname,
+		"auxdataloadn"_yulname,


Just wanted to note that it would be best to put left-overs from previous PRs like this in a separate commit. That's really a bugfix.

That commit could also include the other similar change, i.e. moving the auxdataloadn test to eof/.

Yes. I will move it.

cameel · 2024-10-18T18:30:18Z

libevmasm/Assembly.cpp

+	for (auto& codeSection: m_codeSections)
+	{
+		for (auto const& item: codeSection.items)
+			if (item.type() == AuxDataStore)
+				storesAuxData = true;
+			else if (item.type() == AuxDataLoadN)
+				loadsAuxData = true;
+		if (storesAuxData || loadsAuxData)
+			assertThrow(
+				storesAuxData != loadsAuxData,
+				AssemblyException,
+				"Cannot store and load auxilairy data in the same assembly subroutine."
+			);
+	}


You asked about testing this check and there's really no good way to do it directly, because any checks here are just the last line of defense. They should be detected during analysis instead, where we have a proper error reporting mechanism.

The thing is, we don't have analysis stage for assembly, since we don't support it as input. We can get here only via Yul or through assembly import. So this check should really be at Yul level (probably in AsmAnalysis.cpp) and later be also added as validation in the import. Here it should just be an assert (and I'm not sure this is really the right spot for this assert, since we have to scan the whole bytecode to check it, maybe we can detect it when processing AuxDataStore and/or AuxDataLoadN instead?).

BTW, was the problem with testing it that AssemblyException crashes the compiler instead of showing up as an error in syntax tests? I'm not sure we're actually catching AssemblyException up the stack, but we definitely should.

TBH, most of the errors in Assembly.cpp should instead be assertions since they are things that should be caught by analysis. There are probably only a few that can't and have a legit reason to have validations here. And then they should be presented to to user as as errors, not crashes.

cameel

From the current discussion looks like we won't need auxdatastore after all. For now I'll focus on #15512 and we'll see later whether there's a need to return to this.

I still have a half-finished review from last week with some corrections, so I may just as well post those.

Also, good that you made #15529 not just a commit but actually a separate PR, since that part we do want.

cameel · 2024-10-18T20:42:23Z

libevmasm/Assembly.cpp

+			case AuxDataStore:
+			{
+				// Expect 3 elements on stack (source, dest_base, value_offset_dest)
+				ret.bytecode.push_back(uint8_t(Instruction::ADD));
+				ret.bytecode.push_back(uint8_t(Instruction::MSTORE));
+				break;
+			}


I think we don't need a separate assembly item for this. I think you can simply define the Yul builtin with:

_assembly.appendInstruction(evmasm::Instruction::ADD); _assembly.appendInstruction(evmasm::Instruction::MSTORE);

cameel · 2024-10-18T20:43:38Z

libevmasm/AssemblyItem.cpp

@@ -167,6 +169,8 @@ size_t AssemblyItem::bytesRequired(size_t _addressLength, langutil::EVMVersion _
 		return std::get<2>(*m_verbatimBytecode).size();
 	case AuxDataLoadN:
 		return 1 + 2;
+	case AuxDataStore:
+		return 1;


Doesn't it take up 2 bytes? 1 for ADD and 1 for MSTORE?

cameel · 2024-10-18T20:46:47Z

libyul/backends/evm/AbstractAssembly.h

@@ -110,7 +110,10 @@ class AbstractAssembly
 	/// Appends loading an immutable variable.
 	virtual void appendImmutable(std::string const& _identifier) = 0;
 	/// Appends an assignment to an immutable variable.
-	virtual void appendImmutableAssignment(std::string const& _identifier) = 0;
+	virtual void appendImmutableAssignment(std::string const&) = 0;


I'd leave the variable name in. Especially since this does not have an implementation, the name of the argument is an important hint as to what it means.

rodiazet · 2024-10-22T11:00:06Z

This idea of having ausdatastore was dropped.

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

cameel added the EOF label Oct 17, 2024

rodiazet force-pushed the immutables-eof branch 2 times, most recently from c801f5b to f6a3a6e Compare October 17, 2024 19:43

rodiazet changed the title ~~eof: loadimmutable and setimmutable in EOF~~ eof: auxdatastore in EOF Oct 17, 2024

rodiazet changed the title ~~eof: auxdatastore in EOF~~ eof: auxdatastore as a tool to store immutables in data section of EOF Oct 17, 2024

rodiazet force-pushed the immutables-eof branch 2 times, most recently from 97c8d46 to 6fc82fe Compare October 17, 2024 19:57

rodiazet marked this pull request as ready for review October 18, 2024 08:54

rodiazet force-pushed the immutables-eof branch 2 times, most recently from 90e0cf2 to 5539222 Compare October 18, 2024 12:40

rodiazet mentioned this pull request Oct 18, 2024

eof: new contract creation #15512

Merged

ethereum deleted a comment from ibrahimkhled Oct 18, 2024

cameel reviewed Oct 18, 2024

View reviewed changes

rodiazet force-pushed the immutables-eof branch 2 times, most recently from ffbd5b2 to ba4cd55 Compare October 21, 2024 09:31

rodiazet added 2 commits October 21, 2024 11:41

eof: Add auxdataloadn to reserved identifiers. Move test to eof dir

5b54415

eof: auxdatastore in EOF

35e0796

rodiazet force-pushed the immutables-eof branch from ba4cd55 to 35e0796 Compare October 21, 2024 09:45

This comment was marked as resolved.

Sign in to view

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

cameel requested changes Oct 21, 2024

View reviewed changes

rodiazet closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eof: `auxdatastore` as a tool to store immutables in data section of EOF #15521

eof: `auxdatastore` as a tool to store immutables in data section of EOF #15521

rodiazet commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024

cameel left a comment

cameel Oct 18, 2024

cameel Oct 18, 2024

cameel Oct 18, 2024

cameel Oct 18, 2024

rodiazet Oct 21, 2024

cameel Oct 18, 2024

rodiazet Oct 21, 2024

cameel Oct 18, 2024

rodiazet Oct 21, 2024

cameel Oct 18, 2024

cameel Oct 18, 2024

This comment was marked as resolved.

cameel left a comment •

edited

Loading

cameel Oct 18, 2024

cameel Oct 18, 2024

cameel Oct 18, 2024

rodiazet commented Oct 22, 2024

	void appendImmutableAssignment(std::string const& _identifier) { append(newImmutableAssignment(_identifier));}
	void appendImmutableAssignment(std::string const& _identifier) { append(newImmutableAssignment(_identifier)); }

	/// Stores value in a memory which which will be appended to EOF data section as static axuliary data.
	/// Stores value in memory which will be appended to EOF data section as static auxiliary data.

	"Cannot store and load auxilairy data in the same assembly subroutine."
	"Cannot store and load auxiliary data in the same assembly subroutine."

eof: auxdatastore as a tool to store immutables in data section of EOF #15521

eof: auxdatastore as a tool to store immutables in data section of EOF #15521

Conversation

rodiazet commented Oct 17, 2024 • edited Loading

github-actions bot commented Oct 17, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

cameel left a comment • edited Loading

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

rodiazet commented Oct 22, 2024

eof: `auxdatastore` as a tool to store immutables in data section of EOF #15521

eof: `auxdatastore` as a tool to store immutables in data section of EOF #15521

rodiazet commented Oct 17, 2024 •

edited

Loading

cameel left a comment •

edited

Loading