Add support for importing asm-json. #11807

aarlt · 2021-08-16T23:16:41Z

Note:

PUSH tag got removed, see Remove unused AssemblyItemType::PushString. #11948

libevmasm/Assembly.cpp

chriseth · 2021-08-17T09:12:37Z

solc/CommandLineParser.cpp

+			("Import assembly to be used, assumes input holds assembly in JSON format. "
+			 "Supported Input is the output of the --" + g_strAsmJson + ".").c_str()


This has to be clarified a bit more, I think.

Maybe, what should be clarified a bit more?

libevmasm/AssemblyItem.cpp

libevmasm/Assembly.cpp

aarlt · 2021-08-20T00:45:28Z

Ok, finally I found out what created the mismatching bytecode. I added the auxiliary data to the wrong assembly object.

ekpyron · 2021-08-20T08:42:31Z

If exporting and importing anything really works and produces identical bytecode everywhere, it would probably be nice to add a test run similar to the AST import tests, that just compiles all sources in the repo once directly to bytecode and once via exporting assembly and then importing it back first, comparing the resulting bytecode :-).
If we do that, then one more dedicated test with an artificial assembly JSON containing one of each possible assembly items would probably be enough.

aarlt · 2021-08-26T23:26:30Z

After checking what assembly items where exactly imported by AsmJsonImportTest.sh, it turned out that the following assembly items never get imported due to missing tests.

PUSH tag
PUSH [ErrorTag]
PUSHLIB
PUSHDEPLOYADDRESS
PUSHIMMUTABLE
VERBATIM

TODO: Add specific tests for these assembly items.

libevmasm/Assembly.cpp

aarlt · 2021-08-29T23:47:34Z

Added json re-import - and byte code equivalence test to test/libevmasm/Assembler.cpp.
Still missing assembly items:

PUSH tag
PUSH [ErrorTag]
VERBATIM

cameel · 2021-09-01T17:43:59Z

libevmasm/Assembly.cpp

+
+	auto updateLibraries = [&](string const& value) -> h256 {
+		h256 hash(util::keccak256(value));
+		_assembly.m_libraries[hash] = value;


We might want to check that it's a valid address (length, chars, maybe checksum?)

This is not using any addresses here. value is just a name of a library (I will change the parameter name). The hash is just used as a placeholder, where linking will replace the placeholder with the actual library address.

I guess linking will do some checks on the supplied addresses, but I haven't checked this yet.

Ah, right, these are the placeholders.

Still, the placeholders need to have a specific length, start and end with __, etc. Linking is done in LinkerObject.cpp and it does not seem to have any validations (or even assertions) against that. AFAIK all the validation is in the CLI and StandardCompiler.

BTW, some other parts of this JSON structure might need some validation too.

libevmasm/Assembly.cpp

cameel · 2021-09-01T17:55:53Z

libevmasm/Assembly.cpp

@@ -227,6 +229,221 @@ string Assembly::toStringInHex(u256 _value)
 	return hexStr.str();
 }

+AssemblyItem Assembly::assemblyItemFromJSON(Json::Value const& _json, Assembly& _assembly)


At first I thought this just returns a new item but then I noticed that the function actually modifies _assembly. I think the name should reflect that.

Actually, maybe a better design would be to have two functions - one that just gives you a list of items (and does not modify the assembly) and another one that takes the list and updates the assembly based on it? If you had to explicitly run the update function, it would be clearer that an update is taking place.

cameel · 2021-09-01T18:01:51Z

libevmasm/Assembly.cpp

 		root[".auxdata"] = toHex(m_auxiliaryData);

 	return root;
 }

+void Assembly::forEachJSONAssemblyItem(Json::Value const& _code, Assembly& _assembly)


With the change I suggested above this function would get a list of assembly items. You would not have to call assemblyItemFromJSON() twice for each item (which is especially bad given that assemblyItemFromJSON has side-effects - it modifies the assembly).

solc/CommandLineInterface.cpp

cameel · 2021-09-01T18:38:59Z

solc/CommandLineInterface.cpp

-		!m_compiler->compilationSuccessful() &&
-		m_options.output.stopAfter == CompilerStack::State::CompilationSuccessful
-	)
+	if (m_assembly)


Can m_assembly and m_compiler both be present at the same time?

No, only m_assembly or m_compiler is possible at the same time. Change this to else if.

cameel · 2021-09-01T18:45:01Z

solc/CommandLineParser.cpp

@@ -1187,7 +1195,7 @@ General Information)").c_str(),
 	if (m_options.input.mode == InputMode::Compiler)
 		m_options.input.errorRecovery = (m_args.count(g_strErrorRecovery) > 0);

-	solAssert(m_options.input.mode == InputMode::Compiler || m_options.input.mode == InputMode::CompilerWithASTImport, "");
+	solAssert(m_options.input.mode == InputMode::AssemblyJson || m_options.input.mode == InputMode::Compiler || m_options.input.mode == InputMode::CompilerWithASTImport, "");


Does this new input mode support all of the arguments that the compiler does? If not, I think it should make the function return at a much earlier point.

Also, which other command-line flags are usable in this mode? There's an issue underway to report errors for flags invalid in other modes (#11629) and I think we should start rejecting invalid flags in this new mode from the very beginning.

Each input mode has a test in test/solc/CommandLineParser.cpp that checks all the options valid in that mode. This new mode should have such a test too.

I added a very simple check in solc/CommandLineParser.cpp. Basically with the new import mode only --asm, --bin and --opcodes are right now supported.

cameel · 2021-09-01T18:50:28Z

solc/CommandLineParser.h

@@ -49,6 +49,7 @@ enum class InputMode
 	StandardJson,
 	Linker,
 	Assembler,
+	AssemblyJson,


Assembly is a bit ambiguous. On the CLI it usually refers to Yul. Maybe we should call it EVMAssemblyJson instead? Or rename Assembler mode to Yul?

What about ImportAssemblyJson?

The problem I see is just that it might not be clear that Assembly refers to EVM assembly rather than Yul here. On the other hand the class is named Assembly, so maybe it's not such a big problem...

I just changed that now to ImportEVMAssemblyJson.

libevmasm/Assembly.cpp

…sed.

…mprovements.

aarlt · 2022-02-28T14:25:04Z

Replaced by #12704.

aarlt marked this pull request as draft August 16, 2021 23:16

This comment has been minimized.

Sign in to view

aarlt commented Aug 16, 2021

View reviewed changes