Pass object name to `AsmAnalyzer` #15536

rodiazet · 2024-10-22T13:58:35Z

This is part of eof contract creation prep

github-actions · 2024-10-22T13:58:48Z

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

Looks good overall, but there are a few small tweaks that would be nice to have before we merge.

libyul/AsmAnalysis.h

libsolidity/experimental/analysis/TypeInference.cpp

libyul/AsmAnalysis.h

libyul/AsmAnalysis.cpp

libyul/Object.cpp

libyul/optimiser/StackCompressor.cpp

libyul/AsmAnalysis.cpp

test/libyul/yulSyntaxTests/eof/object_name_in_eof.yul

rodiazet · 2024-10-25T12:37:34Z

Because of an issue with mixing data and object names in one set I implemented in a separated commit a change which separates these two name sets. This allows to properly report an error when trying to use Data instead of Object name in eofcreate or returncontract buildins.

cameel

There are some small issues here - m_objectName is still present, but now not getting initialized. Also a missing check against blocks with no debug data, which could lead to a segfault with --debug-info none.

Other than that, just a few minor things, like names or variables that you defined but left unused.

Overall I like the idea of the struct though. I initially wanted a different one (don't get scared by my giant comment there - it's just for context :)) but actually let's go with yours.

cameel · 2024-10-25T15:19:47Z

libyul/AsmAnalysis.h

-	std::set<std::string> m_dataNames;
+	Object::QualifiedNames m_dataNames;
+	/// Current object name
+	std::string m_objectName;


With the switch to Object::QualifiedNames you no longer initialize this field in constructor so any checks relying on it are broken.

You should remove the field and make the checks use QualifiedNames.rootObjectName instead.

Let's also rename m_dataNames while at it, since it's no longer just data names.

Sure. Fixed.

cameel · 2024-10-25T16:24:52Z

libyul/Object.h

+	struct QualifiedNames
+	{
+		std::string rootObjectName;
+		std::set<std::string> objectPaths;
+		std::set<std::string> dataPaths;
+
+		bool contains(std::string const& _path) const { return containsObject(_path) || containsData(_path); }
+		bool containsObject(std::string const& _path) const { return objectPaths.count(_path) > 0; }
+		bool containsData(std::string const& _path) const { return dataPaths.count(_path) > 0; }
+	};


I like the idea. I'd just rename some things:

Maybe it would be better to call the struct Object::Structure? And rename qualifiedDataNames() to something like summarizeStructure(). It will better reflect its purpose. The Paths bit in member names is enough to show that they contain whole paths, not just names (and some of the paths in it are actually unqualified names).

I'd also rename rootObjectName to just objectName or even name to avoid having it mistaken for the root of the whole object tree.

Just for the record, initially I wanted to propose a slightly different structure that would separate things along different lines. Here's how it would have looked like:

Suggested change

struct QualifiedNames

{

std::string rootObjectName;

std::set<std::string> objectPaths;

std::set<std::string> dataPaths;

bool contains(std::string const& _path) const { return containsObject(_path) || containsData(_path); }

bool containsObject(std::string const& _path) const { return objectPaths.count(_path) > 0; }

bool containsData(std::string const& _path) const { return dataPaths.count(_path) > 0; }

};

/// Summarizes the structure of the subtree rooted at a given object,

/// in particular the paths that can be used from within to refer to nested nodes (objects and data).

/// WARNING: Be careful when changing the set of available paths.

/// It affects the language and removing paths a breaking change.

struct Structure

{

std::string objectName;

/// All valid paths that can be used to reference a subnode from within the current object.

/// This includes all ObjectNodes, i.e. both Object and Data.

/// Note that the same node may be reachable via multiple paths, some of them

/// being unqualified names and some fully-qualified paths.

/// Note also that the name of the object itself is a valid path.

std::set<std::string> allSubNodePaths;

/// Paths that can be used to reach children of the current object that are themselves Objects (and not Data).

/// This is a subset of allSubNodePaths.

std::set<std::string> immediateSubObjectPaths;

};

But after looking at the PR more I'm fine with yours. It has more regularity to it which makes it simpler in some ways.

Finally, getTopLevelSubobjectNames() would actually fit as a helper in this struct now.

What do you think about renaming the struct and moving the helper?

cameel · 2024-10-25T19:20:21Z

libyul/Object.cpp

+		{
+			qualifiedNames.objectPaths.insert(subObjectNode->name);
+
+			auto const subObjectQualifiedDataNames = subObject->qualifiedDataNames();


Unused. Looks like you intended to use it to replace the two calls to subObject->qualifiedDataNames() below, but forgot?

Also, this function is in libyul so solAssert -> yulAssert :)

Yeah. This commit is more like a draft. I will improve naming and add doc comments if it's fine for you

cameel · 2024-10-25T19:46:22Z

libyul/AsmAnalysis.cpp

@@ -68,6 +68,8 @@ bool AsmAnalyzer::analyze(Block const& _block)
 	auto watcher = m_errorReporter.errorWatcher();
 	try
 	{
+		// FIXME: Pass location of the object name. Now it's a location of first code section in yul
+		validateObjectStructure(_block.debugData->nativeLocation);


debugData is not always present. It may be null. You need a check against this. Or just use the helper that does that for you:

Suggested change

validateObjectStructure(_block.debugData->nativeLocation);

validateObjectStructure(nativeLocationOf(_block));

cameel · 2024-10-25T20:11:17Z

libyul/AsmAnalysis.cpp

@@ -766,3 +768,35 @@ bool AsmAnalyzer::validateInstructions(FunctionCall const& _functionCall)
 {
 	return validateInstructions(_functionCall.functionName.name.str(), nativeLocationOf(_functionCall.functionName));
 }
+
+bool AsmAnalyzer::validateObjectStructure(langutil::SourceLocation _astRootLocation)


Maybe we could drop the return value here? You're not checking if anyway.

More importantly, it's the opposite of what I'd expect - true for invalid. Are you maybe following what our weird validateInstruction() was doing? I wanted to change that actually, because that was confusing as hell. Let's not let it spread :)

I just wanted to be consisted with this convention "Some errors -> return true". :) But yeah let's skip return value.

Yeah, it's good to be consistent, just not sure we actually have that convention. I only recall it in that one function :)

cameel · 2024-10-25T20:26:40Z

BTW, many of the problems we had here, like m_objectName not getting initialized or the source location not being present in test output would normally have been detected in CI by the test suite. We should really add a run with EOF enabled in soltest_all.sh (in a separate PR). Not all tests will pass yet (especially not semantic tests), but we could whitelist only the suites known to work, like yulSyntaxTests and objectCompilerTests.

rodiazet · 2024-10-28T10:59:51Z

BTW, many of the problems we had here, like m_objectName not getting initialized or the source location not being present in test output would normally have been detected in CI by the test suite. We should really add a run with EOF enabled in soltest_all.sh (in a separate PR). Not all tests will pass yet (especially not semantic tests), but we could whitelist only the suites known to work, like yulSyntaxTests and objectCompilerTests.

I would do it too. Even started looking on solltest_all.sh but now idea where this should be added. Can someone assist with this?

cameel · 2024-10-28T19:13:19Z

I guess I can add it myself. Probably better to keep you focused on the actual EOF bits anyway.

cameel · 2024-10-29T14:45:08Z

PR that adds an EOF pass: #15552.

cameel

This is fine now in terms of correctness.

I still have some small nitpicks. Just some wording tweaks and also parts of what I suggested in #15536 (comment). But I don't want to add another round just for those so I did them myself and pushed 3 commits to the object-name-to-analysis branch. So just pull them in (all or some of them; I separated the rename so that it can be easily dropped if you don't like it) squash and then we can merge this.

rodiazet · 2024-10-30T09:21:51Z

This is fine now in terms of correctness.

I still have some small nitpicks. Just some wording tweaks and also parts of what I suggested in #15536 (comment). But I don't want to add another round just for those so I did them myself and pushed 3 commits to the object-name-to-analysis branch. So just pull them in (all or some of them; I separated the rename so that it can be easily dropped if you don't like it) squash and then we can merge this.

Ok. thanks. I understood from the comment that it's fine finally. But yest your wording is better.

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

rodiazet mentioned this pull request Oct 22, 2024

eof: new contract creation #15512

Merged

rodiazet force-pushed the object-name-to-analysis branch from 4b116b9 to 58bfddb Compare October 22, 2024 14:01

cameel added refactor EOF labels Oct 22, 2024

This comment was marked as resolved.

Sign in to view

rodiazet force-pushed the object-name-to-analysis branch 2 times, most recently from 4bca0b1 to 510cdab Compare October 22, 2024 21:09

This comment was marked as resolved.

Sign in to view

cameel previously approved these changes Oct 22, 2024

View reviewed changes

libyul/AsmAnalysis.h Outdated Show resolved Hide resolved

libyul/AsmAnalysis.h Outdated Show resolved Hide resolved

libsolidity/experimental/analysis/TypeInference.cpp Outdated Show resolved Hide resolved

rodiazet dismissed cameel’s stale review via 38004f4 October 23, 2024 08:25

rodiazet force-pushed the object-name-to-analysis branch 4 times, most recently from 68d9801 to bd29212 Compare October 23, 2024 14:50

cameel reviewed Oct 23, 2024

View reviewed changes

libyul/AsmAnalysis.h Outdated Show resolved Hide resolved

rodiazet force-pushed the object-name-to-analysis branch from bd29212 to 77ea2bb Compare October 24, 2024 10:48

cameel reviewed Oct 24, 2024

View reviewed changes

libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved

libyul/Object.cpp Outdated Show resolved Hide resolved

libyul/optimiser/StackCompressor.cpp Outdated Show resolved Hide resolved

rodiazet force-pushed the object-name-to-analysis branch 4 times, most recently from d263069 to 6bc3aed Compare October 24, 2024 15:46

cameel previously approved these changes Oct 25, 2024

View reviewed changes

libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved

cameel reviewed Oct 25, 2024

View reviewed changes

test/libyul/yulSyntaxTests/eof/object_name_in_eof.yul Outdated Show resolved Hide resolved

rodiazet dismissed cameel’s stale review via f323d7f October 25, 2024 08:51

rodiazet force-pushed the object-name-to-analysis branch 2 times, most recently from f323d7f to 9c33cb0 Compare October 25, 2024 09:07

rodiazet force-pushed the object-name-to-analysis branch from d0e7890 to 642722e Compare October 25, 2024 13:16

cameel reviewed Oct 25, 2024

View reviewed changes

rodiazet force-pushed the object-name-to-analysis branch 3 times, most recently from 3c7b9b7 to 3937a25 Compare October 28, 2024 09:26

cameel previously approved these changes Oct 29, 2024

View reviewed changes

rodiazet dismissed cameel’s stale review via ec95423 October 30, 2024 09:21

rodiazet force-pushed the object-name-to-analysis branch from 3937a25 to ec95423 Compare October 30, 2024 09:21

Pass object name to AsmAnalyzer

e6b0856

rodiazet force-pushed the object-name-to-analysis branch from ec95423 to e6b0856 Compare October 30, 2024 09:44

cameel approved these changes Oct 30, 2024

View reviewed changes

cameel merged commit fd96fcf into ethereum:develop Oct 30, 2024
73 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass object name to `AsmAnalyzer` #15536

Pass object name to `AsmAnalyzer` #15536

rodiazet commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024

This comment was marked as resolved.

This comment was marked as resolved.

cameel left a comment

rodiazet commented Oct 25, 2024

cameel left a comment

cameel Oct 25, 2024

cameel Oct 25, 2024

rodiazet Oct 28, 2024

cameel Oct 25, 2024

cameel Oct 25, 2024

cameel Oct 25, 2024

cameel Oct 29, 2024

cameel Oct 25, 2024

cameel Oct 25, 2024

rodiazet Oct 28, 2024

cameel Oct 25, 2024

cameel Oct 25, 2024

rodiazet Oct 28, 2024

cameel Oct 28, 2024

cameel commented Oct 25, 2024

rodiazet commented Oct 28, 2024

cameel commented Oct 28, 2024

cameel commented Oct 29, 2024

cameel left a comment •

edited

Loading

rodiazet commented Oct 30, 2024

-	struct QualifiedNames
-	{
-		std::string rootObjectName;
-		std::set<std::string> objectPaths;
-		std::set<std::string> dataPaths;
-		bool contains(std::string const& _path) const { return containsObject(_path) || containsData(_path); }
-		bool containsObject(std::string const& _path) const { return objectPaths.count(_path) > 0; }
-		bool containsData(std::string const& _path) const { return dataPaths.count(_path) > 0; }
-	};
+	/// Summarizes the structure of the subtree rooted at a given object,
+	/// in particular the paths that can be used from within to refer to nested nodes (objects and data).
+	/// WARNING: Be careful when changing the set of available paths.
+	/// It affects the language and removing paths a breaking change.
+	struct Structure
+	{
+		std::string objectName;
+		/// All valid paths that can be used to reference a subnode from within the current object.
+		/// This includes all ObjectNodes, i.e. both Object and Data.
+		/// Note that the same node may be reachable via multiple paths, some of them
+		/// being unqualified names and some fully-qualified paths.
+		/// Note also that the name of the object itself is a valid path.
+		std::set<std::string> allSubNodePaths;
+        /// Paths that can be used to reach children of the current object that are themselves Objects (and not Data).
+		/// This is a subset of allSubNodePaths.
+		std::set<std::string> immediateSubObjectPaths;
+	};

	validateObjectStructure(_block.debugData->nativeLocation);
	validateObjectStructure(nativeLocationOf(_block));

Pass object name to AsmAnalyzer #15536

Pass object name to AsmAnalyzer #15536

Conversation

rodiazet commented Oct 22, 2024 • edited Loading

github-actions bot commented Oct 22, 2024

This comment was marked as resolved.

This comment was marked as resolved.

cameel left a comment

Choose a reason for hiding this comment

rodiazet commented Oct 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Oct 25, 2024

rodiazet commented Oct 28, 2024

cameel commented Oct 28, 2024

cameel commented Oct 29, 2024

cameel left a comment • edited Loading

Choose a reason for hiding this comment

rodiazet commented Oct 30, 2024

Pass object name to `AsmAnalyzer` #15536

Pass object name to `AsmAnalyzer` #15536

rodiazet commented Oct 22, 2024 •

edited

Loading

cameel left a comment •

edited

Loading