Introduce subassembly offset output artifact #15710

nikola-matic · 2025-01-13T15:25:59Z

No description provided.

cameel

There are some bugs, some missing parts and the overall implementation could be done more robustly.

I'm also not sure if the whole design actually accomplishes our goal. Only giving sub locations may not be enough to locate metadata without heuristics because data objects are not placed in separate subs at evmasm level.

See comments below for details.

cameel · 2025-01-17T16:14:45Z

libevmasm/Assembly.cpp

@@ -1215,6 +1216,7 @@ LinkerObject const& Assembly::assembleLegacy() const
 			);
 			immutableReferencesBySub = linkerObject.immutableReferences;
 		}
+		subAssemblies.push_back(linkerObject);


The linker object is already available as m_assembledObject on each Assembly in m_subs. No need to create another copy.

cameel · 2025-01-17T16:20:55Z

libevmasm/LinkerObject.h

+		size_t start;
+		size_t length;
+		bool isCreation;
+		std::string bytecode;


It's not the same thing as bytecode stored directly in LinkerObject. The name should contain enough information to make that clear:

Suggested change

std::string bytecode;

std::string linkedBytecodeHex;

Actually, why do you even store it? It's relatively large (on the same order as the size of the whole LinkerObject) and all you ever do with it is check its size (which you could get from length instead).

cameel · 2025-01-17T16:24:08Z

libsolidity/interface/StandardCompiler.cpp

Please add the feature to the CLI too. We should keep them at parity (and it's a pain for testing/development if the only way to access a feature is through StandardJSON).

Also, Yul compilation is not covered. Aside from general inconsistency, this makes two-step compilation less powerful, which may be a problem for future parallelization.

cameel · 2025-01-17T16:48:29Z

libevmasm/Assembly.cpp

This needs to be implemented for EOF as well (i.e. in assembleEOF(), or, if possible, just in assemble() covering both with the same code).

Since we're at the stage where EOF is passing semantic tests (and they will be enabled by default quite soon), we should start requiring all new features for work on EOF as well.

cameel · 2025-01-17T17:17:43Z

libevmasm/Assembly.cpp

+{
+	for (auto& subAssembly: _subAssemblies)
+	{
+		subAssembly.start = _currentBytecodeSize - subAssembly.length;


I'm either missing something or this assumes that all subassemblies overlap and extend to the end of their parent assembly. Does it even work with more than one subassembly? If you have two subassemblies of the same length then you will end up with the same start location for both. And even if they're of different lengths it will be wrong.

EDIT: Yeah, you even have a test showing this overlap (standard_subassembly_offsets/):

{ "isCreation": false, "length": 780, "start": 1007 }, { "isCreation": true, "length": 130, "start": 1657, "subs": [

You should also have some asserts here. At the very least that a subassembly does not stick outside of its parent assembly.

cameel · 2025-01-17T17:57:13Z

test/cmdlineTests/standard_import_ast_select_bytecode/output.json

+                        "subAssemblyOffsets": {
+                            "subs": [
+                                {
+                                    "isCreation": true,
+                                    "length": 130,
+                                    "start": 0,
+                                    "subs": [
+                                        {
+                                            "isCreation": false,
+                                            "length": 104,
+                                            "start": 26
+                                        }
+                                    ]
+                                }
+                            ]
+                        }


Actually, is Sourcify ok with getting only subassembly locations? I've always been thinking about metadata as a separate subassembly myself, because it's separate data Object in Yul, but looking at the PR I remembered that it's actually not the case at evmasm level. Metadata goes into Assembly::m_auxiliaryData and logically becomes a part of each assembly's bytecode, not a separate object. It's simply appended after all the subs and other data. I think that due to this it will still be necessary to use heuristics to fish out its location within the assembly, even knowing location of all subassemblies.

I think we may need to include the start and length of Assembly::m_auxiliaryData separately for each sub (when it's non-empty). For completeness we may want to simply list all the data chunks (that would actually have been nice to have for compiler debugging in some cases).

CC @kuzdogan.

By the way, we should have some tests that include data objects between assemblies.

Also, if we had the exact location of metadata, we could create a more robust, Boost-based test that would get the structure info and the bytecode and diff the CBOR bit. It's not easy to spot a problem just looking at the values in command-line tests and we're not even shown the bytecode.

cameel · 2025-01-17T18:05:49Z

test/cmdlineTests/standard_no_append_cbor/output.json

+                        "subAssemblyOffsets": {
+                            "subs": [
+                                {
+                                    "isCreation": true,
+                                    "length": 27,
+                                    "start": 0,
+                                    "subs": [


The top-level assembly is not nested in anything. Seems weird to put its data under 'subs'. It should be at the top level. Similarly with subAssemblyOffsets - it should rather be assemblyOffsets since it covers the whole binary, including the top.

Finally, we might end up including more info there later so maybe assemblyStructure would be a more future-proof name?

Suggested change

"subAssemblyOffsets": {

"subs": [

{

"isCreation": true,

"length": 27,

"start": 0,

"subs": [

"assemblyStructure": {

"isCreation": true,

"length": 27,

"start": 0,

"subassemblies": [

I'd use that name for LinkerObject::SubAssembly too, i.e. LinkerObject::Structure, which would nicely mirror Object::Structure which is similar in the sense that it stores metadata about the hierarchy (but on Yul level).

cameel · 2025-01-17T18:15:27Z

test/cmdlineTests/standard_subassembly_offsets/input.json

+    "outputSelection": {
+      "*": {
+        "Storage": [
+          "evm.bytecode.subAssemblyOffsets"
+        ]
+      }
+    }


You're not testing evm.deployedBytecode.

Also, bad indentation. And I'd put the contract in a separate file.

cameel · 2025-01-17T18:39:00Z

libsolidity/interface/StandardCompiler.cpp

+			{
+				Json ret = Json::object();
+				ret["subs"] = formatSubAssemblyOffsets(compilerStack.object(contractName).subAssemblyData);
+				creationJSON["subAssemblyOffsets"] = std::move(ret);
+			}


Just FYI, with nlohmann-json you can create JSON objects in place:

Suggested change

{

Json ret = Json::object();

ret["subs"] = formatSubAssemblyOffsets(compilerStack.object(contractName).subAssemblyData);

creationJSON["subAssemblyOffsets"] = std::move(ret);

}

creationJSON["subAssemblyOffsets"] = {

{"subs", formatSubAssemblyOffsets(compilerStack.object(contractName).subAssemblyData)}

};

We should generally use that style whenever possible, because the intermediate variables does not add anything of value, especially one with a horrible name like ret. And the reversed order always makes things harder to follow.

cameel · 2025-01-17T18:56:26Z

test/cmdlineTests/standard_import_asm_json_immutable_references/output.json

+                        "subAssemblyOffsets": {
+                            "subs": [
+                                {
+                                    "isCreation": false,
+                                    "length": 87,
+                                    "start": 0
+                                }
+                            ]
+                        }


The input file has two assemblies but the output shows only one. Is this a bug in your feature or an instance of #15725 (because the nested assembly is unreferenced)?

cameel · 2025-01-17T19:22:25Z

test/cmdlineTests/standard_import_asm_json_immutable_references/output.json

+                        "subAssemblyOffsets": {
+                            "subs": [
+                                {
+                                    "isCreation": false,


By the way, I wonder why this isn't true. Does exporting and reimporting asm JSON lose the creation status or is it just because of how the artificial input was crafted? If the status gets lost, this would be a bug (and should be reported).

cameel · 2025-01-17T19:25:22Z

test/cmdlineTests/standard_subassembly_offsets/input.json

Oh, we also need some coverage for optimized compilation.

nikola-matic requested review from cameel and ekpyron January 13, 2025 15:26

nikola-matic added 4 commits January 17, 2025 11:39

Introduce sub-assembly offset output artifact

c5408cd

fixup! Introduce sub-assembly offset output artifact

4ac9132

fixup! fixup! Introduce sub-assembly offset output artifact

6edeb8e

fixup! fixup! fixup! Introduce sub-assembly offset output artifact

e4f8896

nikola-matic force-pushed the introduce-subassembly-offset-output-artifact branch from d086ad9 to e4f8896 Compare January 17, 2025 10:39

cameel requested changes Jan 17, 2025

View reviewed changes

cameel reviewed Jan 17, 2025

View reviewed changes

test/cmdlineTests/standard_subassembly_offsets/input.json

Copy link

Member

cameel Jan 17, 2025

Choose a reason for hiding this comment

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

Oh, we also need some coverage for optimized compilation.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce subassembly offset output artifact #15710

Introduce subassembly offset output artifact #15710

nikola-matic commented Jan 13, 2025

cameel left a comment •

edited

Loading

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025

cameel Jan 17, 2025 •

edited

Loading

cameel Jan 17, 2025

Introduce subassembly offset output artifact #15710

Are you sure you want to change the base?

Introduce subassembly offset output artifact #15710

Conversation

nikola-matic commented Jan 13, 2025

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

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 Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel left a comment •

edited

Loading

cameel Jan 17, 2025 •

edited

Loading