libevmasm: refactor asm-json export & add support for source list. #12799

aarlt · 2022-03-15T20:59:37Z

Needed for #11787.
Replaces part of #12704.

This PR refactors the asm-json export and adds support for source list (e.g. needed for srcmaps during asm-json import).

libevmasm/Assembly.cpp

test/cmdlineTests/asm_json/output

libevmasm/Assembly.cpp

libevmasm/Assembly.h

chriseth · 2022-03-17T09:36:02Z

I think this can be even further simplified by creating a function (can be a method of AssemblyItem) that returns pair<string, string> which is the name and the value. The function would then just be

switch (_item.type())
{
case Operation: return {instructionInfo(_item.instruction()).name, ""};
case Push: return {"PUSH", toStringInHex(_item.data())};
...
}

The rest of the values is then added in the loop inside Assembly.cpp and inside this loop, we also add the additional jumpdest for tags.

What do you think?

libevmasm/Assembly.h

cameel · 2022-03-17T14:19:09Z

libevmasm/Assembly.h

@@ -39,6 +39,7 @@
 #include <sstream>
 #include <memory>
 #include <map>
+#include <utility>


Interesting. Never knew that C++ had such a catch-all header for basic stuff. I wonder if we should be using it more often instead of including things individually.

cameel · 2022-03-17T14:25:10Z

libevmasm/Assembly.cpp

+	if (_item.m_modifierDepth != 0)
+		value["modifierDepth"] = static_cast<int>(_item.m_modifierDepth);
 	if (!_value.empty())
 		value["value"] = _value;
-	if (!_jumpType.empty())
-		value["jumpType"] = _jumpType;


I see that after your changes, "jumpType" field is no longer being filled. On the other hand you're now filling "modifierDepth". This seems like an API change rather than just a refactor. Is that intentional?

Yes, I removed jumpType yesterday, because I saw that it is just stored in value (for Operation). Regarding the old code, jumpType is only set for PUSH. Not sure why.

@chriseth why is jumpType needed for PUSH?

Or is it just a typo in the code?

I don't see a problem with the old code. Yes, jumptype was output for push, but also for operation.

Oh you are right, it was stored in the wrong location. Why did nobody ever complain?

I guess nobody really used it yet. Decision from call: store jump type's always in jumpType node.

libevmasm/Assembly.cpp

cameel · 2022-03-17T15:54:18Z

test/cmdlineTests/asm_json/output

        }
      ]
    }
-  }
+  },


Why is the removal of "jumpType" and addition of "modifierDepth" not reflected in this test? I'd expect it to fail after these changes.

I'm now also wondering. Yesterday I changed exactly this part. The thing is that in our current tests we have it like this:

{ "begin": 77, "end": 158, "name": "JUMP", "source": 0, "value": "[in]" }

So I saw yesterday, that the jumpType is stored in value - so I just changed that - without really checking the original code. The strange thing is that the original code reads like that there should be an jumpType node. Something seem to be wrong. I will look into this. Thx for that hint!

For modifierDepth it is just needed to be able to import the asm json correctly. Without this information it is not possible to generate the original byte code.

So that JSON the function creates is not the final form but later goes through some extra processing?

Not sure what you mean. I think modifierDepth was just forgotten when asm-json was first implemented.

Oh. From your comment I thought that modifierDepth is used internally but then not written out on purpose - so there must be some postprocessing step that removes it (and maybe does other stuff).

I just needed to add modifierDepth. It was just missing in the original asm-json implementation. There is no post processing or any other extra processing involved. This information was just missing, and now this information will be exported correctly.

Just to also write it here in this thread: decision from call: always store jump type in a jumpType node. @cameel thx for making us aware of that!

chriseth · 2022-03-21T13:16:13Z

libevmasm/Assembly.cpp

-		default:
-			assertThrow(false, InvalidOpcode, "");
+			if (name == "JUMP")
+				item["value"] = jumpType;


Please add a comment that this is replicating a bug.

I replaced this now with

if (!jumpType.empty()) item["jumpType"] = jumpType;

so whenever a jump type was defined, it will be stored in an jumpType node.

The original behaviour for JUMP (and maybe also other operations) was to store it in value. We will now store this in jumpType.

chriseth · 2022-03-21T13:16:22Z

libevmasm/Assembly.cpp

-			assertThrow(false, InvalidOpcode, "");
+			if (name == "JUMP")
+				item["value"] = jumpType;
+			if (name == "PUSH")


Suggested change

if (name == "PUSH")

else

We will store this now always in jumpType.

chriseth · 2022-03-21T13:20:15Z

libevmasm/AssemblyItem.cpp

@@ -56,6 +57,51 @@ pair<size_t, size_t> AssemblyItem::splitForeignPushTag() const
 	return make_pair(subId, tag);
 }

+string AssemblyItem::toStringInHex(u256 _value)


Don't we already have a utility function for that? In any case, please turn this into a function inside an anonnymous namespace at least.

Yes, but I was not able to put it in exactly the needed hex format with the other utility functions. Moved to anonymous namespace.

chriseth · 2022-03-21T17:00:40Z

Since this also fixes a bug now, it needs a changelog entry.

libevmasm/Assembly.cpp

chriseth · 2022-03-21T17:05:35Z

libevmasm/Assembly.cpp

+		for (auto const& [name, index]: _sourceIndices)
+			sourceNames[index] = name;
+		for (auto const& item: sourceNames)
+			jsonSourceList.append(item);


Can't you make this a json array and then avoid the vector?

The actual order will be defined with the first for loop. The json array seem not to allow the insertion of items at specific positions. I just use the vector to store the final list of source names, ordered according to its index.

chriseth · 2022-03-21T17:06:26Z

libevmasm/Assembly.h

@@ -147,7 +148,8 @@ class Assembly

 	/// Create a JSON representation of the assembly.
 	Json::Value assemblyJSON(
-		std::map<std::string, unsigned> const& _sourceIndices = std::map<std::string, unsigned>()
+		std::map<std::string, unsigned> const& _sourceIndices = std::map<std::string, unsigned>(),
+		bool _includeSourceList = true


Shouldn't this default to false to retain the old behaviour? Do we need a default value?

Right now I set includeSourceList to false, only if a sub-assembly is exported. In CompilerStack::assemblyJSON(..) we want to have the source list included. If called with default value, the top-level assembly json will always include the source list, but it's sub-assemblies not.

Oh, this should also be mentioned in the changelog.

Thats true!

libevmasm/AssemblyItem.h

chriseth · 2022-03-21T17:08:52Z

Can you add a modifier to the commandline test so we can test the modifier depth?

chriseth · 2022-03-22T14:06:10Z

Changelog.md

@@ -8,6 +8,7 @@ Compiler Features:


 Bugfixes:
+ * Assembly: Fix assembly json export to store jump types of operations in `jumpType` field (instead of `value`).


Suggested change

* Assembly: Fix assembly json export to store jump types of operations in `jumpType` field (instead of `value`).

* Assembly-Json: Fix assembly json export to store jump types of operations in `jumpType` field instead of `value`.

chriseth · 2022-03-22T14:09:54Z

libevmasm/Assembly.cpp

+		root["sourceList"] = Json::arrayValue;
+		Json::Value& jsonSourceList = root["sourceList"];
+		vector<string> sourceNames(_sourceIndices.size());
+		for (auto const& [name, index]: _sourceIndices)
+			sourceNames[index] = name;
+		for (auto const& item: sourceNames)
+			jsonSourceList.append(item);


Suggested change

root["sourceList"] = Json::arrayValue;

Json::Value& jsonSourceList = root["sourceList"];

vector<string> sourceNames(_sourceIndices.size());

for (auto const& [name, index]: _sourceIndices)

sourceNames[index] = name;

for (auto const& item: sourceNames)

jsonSourceList.append(item);

root["sourceList"] = Json::arrayValue;

for (auto const& [name, index]: _sourceIndices)

root["sourceList"][index] = name;

I'm pretty sure this does it.

Oh, great! Somehow I did not check the operators. Thx!

Changelog.md

libevmasm/Assembly.cpp

chriseth · 2022-03-23T14:40:08Z

Only cosmetic changes left.

Sticknmove · 2022-10-19T12:57:33Z

Thanks

aarlt force-pushed the asm-json-export branch from 69be6bd to 078255f Compare March 15, 2022 21:39

aarlt commented Mar 15, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

chriseth reviewed Mar 16, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the asm-json-export branch 2 times, most recently from 578998d to 9d7cc21 Compare March 16, 2022 17:11

aarlt commented Mar 16, 2022

View reviewed changes

test/cmdlineTests/asm_json/output Show resolved Hide resolved

aarlt force-pushed the asm-json-export branch from 9d7cc21 to f37f081 Compare March 16, 2022 17:28

chriseth reviewed Mar 16, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

chriseth reviewed Mar 16, 2022

View reviewed changes

libevmasm/Assembly.h Outdated Show resolved Hide resolved

aarlt force-pushed the asm-json-export branch from f37f081 to 6502a4b Compare March 16, 2022 21:53

cameel reviewed Mar 17, 2022

View reviewed changes

aarlt force-pushed the asm-json-export branch from 6502a4b to 87a0819 Compare March 17, 2022 21:52

chriseth reviewed Mar 21, 2022

View reviewed changes

aarlt force-pushed the asm-json-export branch from 87a0819 to f09e542 Compare March 21, 2022 14:50

chriseth reviewed Mar 21, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

chriseth reviewed Mar 21, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

chriseth reviewed Mar 21, 2022

View reviewed changes

libevmasm/Assembly.cpp Show resolved Hide resolved

chriseth reviewed Mar 21, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

chriseth reviewed Mar 21, 2022

View reviewed changes

libevmasm/Assembly.cpp Show resolved Hide resolved

chriseth reviewed Mar 21, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

chriseth reviewed Mar 21, 2022

View reviewed changes

libevmasm/AssemblyItem.h Show resolved Hide resolved

aarlt force-pushed the asm-json-export branch 2 times, most recently from 2f386ea to a3d5c9d Compare March 21, 2022 17:42

aarlt force-pushed the asm-json-export branch 2 times, most recently from aeab412 to e4acc25 Compare March 21, 2022 22:36

chriseth reviewed Mar 22, 2022

View reviewed changes

aarlt force-pushed the asm-json-export branch from e4acc25 to b9504ee Compare March 22, 2022 16:00

This was referenced Mar 22, 2022

Add --import-asm-json input mode. #12704

Closed

libevmasm: Add support to import evm assembly json. #12834

Closed

chriseth reviewed Mar 23, 2022

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

chriseth reviewed Mar 23, 2022

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the asm-json-export branch from b9504ee to f3dd7cd Compare March 23, 2022 20:50

libevmasm: refactor asm-json export & add support for source list.

1a0988e

aarlt force-pushed the asm-json-export branch from f3dd7cd to 1a0988e Compare March 23, 2022 20:58

chriseth approved these changes Mar 24, 2022

View reviewed changes

chriseth merged commit b08190c into develop Mar 24, 2022

chriseth deleted the asm-json-export branch March 24, 2022 11:13

aarlt mentioned this pull request Nov 15, 2022

Add support to import evm assembly json (updated). #13673

Merged

6 tasks

cameel added the refactor label Feb 1, 2023

		@@ -8,6 +8,7 @@ Compiler Features:


		Bugfixes:
		* Assembly: Fix assembly json export to store jump types of operations in `jumpType` field (instead of `value`).

	* Assembly: Fix assembly json export to store jump types of operations in `jumpType` field (instead of `value`).
	* Assembly-Json: Fix assembly json export to store jump types of operations in `jumpType` field instead of `value`.

libevmasm: refactor asm-json export & add support for source list. #12799

libevmasm: refactor asm-json export & add support for source list. #12799

Conversation

aarlt commented Mar 15, 2022 • edited by cameel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chriseth commented Mar 17, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cameel Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Mar 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt commented Mar 15, 2022 •

edited by cameel

Loading

cameel Mar 17, 2022 •

edited

Loading

aarlt Mar 21, 2022 •

edited

Loading