Add --asm-json output in assembler mode #14612

r0qs · 2023-10-13T16:44:04Z

Extracted from #13673 based on this comment #13673 (comment)

libevmasm/Assembly.cpp

nikola-matic · 2023-10-16T15:46:38Z

Only took a cursory look, but I think you're missing tests in test/solc/CommandLineParser.cpp and test/solc/CommandLineInterface.cpp.

edit: Actually, perhaps not CommandLineParser since the option is already there.

r0qs · 2023-10-17T11:02:29Z

Only took a cursory look, but I think you're missing tests in test/solc/CommandLineParser.cpp and test/solc/CommandLineInterface.cpp.

edit: Actually, perhaps not CommandLineParser since the option is already there.

Good point. The option is there in the CommandLineParser tests, but not for the assembler mode. I will add it ;)

About the CommandLineInterface tests, I guess it is not needed, is it? There is already command-line tests to check the output. Or do you mean to add a unit test for the collectYulSourceIndices?

matheusaaguiar

I think there are some refinements that can be made to the code, but it is looking good.

libevmasm/Assembly.cpp

solc/CommandLineInterface.cpp

Changelog.md

libevmasm/Assembly.cpp

nikola-matic · 2023-12-14T08:31:06Z

@r0qs fix the changelog, and I'll approve.

solc/CommandLineInterface.cpp

libevmasm/Assembly.cpp

cameel · 2023-12-15T17:47:08Z

solc/CommandLineInterface.cpp

+					std::map<std::string, unsigned> sourceIndices = collectYulSourceIndices(*stack.parserResult());
+					// If sourceIndices are empty, there were no source locations annotated in the yul source.
+					// In this case, we just add the filename of the yul file itself.
+					if (sourceIndices.empty())
+						sourceIndices[src.first] = 0;
+
+					sout() << util::jsonPrint(
+						removeNullMembers(assembly->assemblyJSON(sourceIndices)),
+						m_options.formatting.json
+					) << std::endl;
+					return;


I'd put this logic (without actual printing and removal of nulls of course) in YulStack, similar to how CompilerStack currently has sourceIndices() and assemblyJSON() methods. This will also make it easier to later reuse it in StandardJSON.

It's not a lot of logic, but at least the decision to stick the Yul source name in there is something that you may not even realize you should do when you need to write this again, so it should be encapsulated in a single place.

Good point. I moved the logic to YulStack. But I was wondering if placing collectYulSourceIndices (now named sourceIndices) in yul::Object might be more appropriate than in YulStack. Also, maybe this piece of code:

if (sourceIndices.empty()) sourceIndices[src.first] = 0;

Could be moved to collectYulSourceIndices as well, I haven't made this adjustment yet, as I am uncertain whether this behavior aligns with the expectations of sourceIndices in CompilerStack (i.e. if there is no source locations annotated in the yul source we should have the filename of the yul file pointing to index 0).
.

Maybe. It would make sense as something like Object::collectSourceIndices() since it does not really depend on anything in the stack.

But then one might expect it to keep the strings shared via shared_ptr. Probably still fine not to, because we can always change it if needed, but the function right now really makes copies like crazy, while the sharing was meant to prevent that so it's a bit against the design of Object :) In the current form might be better to just leave it in the stack.

test/cmdlineTests/asm_json_export_yul_more_subobjects/stdin

test/cmdlineTests/asm_json_export_yul_arbitrary_source_index/output

libyul/YulStack.h

libevmasm/Assembly.cpp

cameel

This is almost ready to merge. Please rebase on top of develop, squash the commits a bit and fix the typo in the function name I found. Then we can merge.

cameel · 2024-07-31T11:11:31Z

libyul/YulStack.h

@@ -163,6 +166,8 @@ class YulStack: public langutil::CharStreamProvider
 	std::shared_ptr<yul::Object> m_parserResult;
 	langutil::ErrorList m_errors;
 	langutil::ErrorReporter m_errorReporter;
+
+	std::unique_ptr<std::string> m_sourceMappings;


Ah, wait. Why did this come back? I remember removing it in one of my PRs because it was unused. Looks like rebasing error.

Co-authored-by: r0qs <[email protected]> Co-authored-by: Kamil Śliwak <[email protected]>

r0qs self-assigned this Oct 13, 2023

r0qs commented Oct 13, 2023

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

r0qs added the feature label Oct 13, 2023

r0qs force-pushed the add-asm-json-output-assembler-mode branch from e766721 to bb5a4eb Compare October 16, 2023 13:29

ekpyron added this to the 0.8.22 milestone Oct 16, 2023

r0qs force-pushed the add-asm-json-output-assembler-mode branch from bb5a4eb to 60c808d Compare October 17, 2023 10:30

r0qs force-pushed the add-asm-json-output-assembler-mode branch 3 times, most recently from c1d82f8 to e1a7045 Compare October 18, 2023 12:53

matheusaaguiar reviewed Oct 19, 2023

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

solc/CommandLineInterface.cpp Outdated Show resolved Hide resolved

Changelog.md Outdated Show resolved Hide resolved

r0qs force-pushed the add-asm-json-output-assembler-mode branch 2 times, most recently from 4ab1b97 to 0eaf85f Compare October 26, 2023 21:24

r0qs removed this from the 0.8.22 milestone Oct 27, 2023

r0qs force-pushed the add-asm-json-output-assembler-mode branch from 0eaf85f to 25977ef Compare November 9, 2023 13:12

r0qs force-pushed the add-asm-json-output-assembler-mode branch from 25977ef to 3e18fdc Compare November 19, 2023 10:50

r0qs force-pushed the add-asm-json-output-assembler-mode branch from 3e18fdc to 477eb60 Compare November 29, 2023 19:16

r0qs force-pushed the add-asm-json-output-assembler-mode branch from 477eb60 to 0955cd0 Compare December 11, 2023 12:57

nikola-matic force-pushed the add-asm-json-output-assembler-mode branch from 0955cd0 to 81083a7 Compare December 12, 2023 18:17

nikola-matic reviewed Dec 12, 2023

View reviewed changes

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

r0qs force-pushed the add-asm-json-output-assembler-mode branch from 5a2a274 to 180aaf4 Compare December 13, 2023 12:39

cameel reviewed Dec 15, 2023

View reviewed changes

test/cmdlineTests/asm_json_export_yul_arbitrary_source_index/output Outdated Show resolved Hide resolved

r0qs force-pushed the add-asm-json-output-assembler-mode branch 2 times, most recently from cf927e7 to 59ed68a Compare December 18, 2023 20:24

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jan 3, 2024

nikola-matic removed the stale The issue/PR was marked as stale because it has been open for too long. label Jan 3, 2024

ethereum deleted a comment from github-actions bot Jan 3, 2024

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jun 6, 2024

cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Jun 7, 2024

ethereum deleted a comment from github-actions bot Jun 7, 2024

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jun 21, 2024

cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Jun 23, 2024

ethereum deleted a comment from github-actions bot Jun 25, 2024

aarlt force-pushed the add-asm-json-output-assembler-mode branch from de114b7 to 6ba226c Compare June 25, 2024 16:43

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jul 10, 2024

ethereum deleted a comment from github-actions bot Jul 11, 2024

aarlt removed the stale The issue/PR was marked as stale because it has been open for too long. label Jul 11, 2024

aarlt force-pushed the add-asm-json-output-assembler-mode branch from 6ba226c to 0cc249d Compare July 11, 2024 05:35

cameel mentioned this pull request Jul 24, 2024

The --asm-json output suppresses --asm #14628

Open

cameel reviewed Jul 24, 2024

View reviewed changes

libyul/YulStack.h Outdated Show resolved Hide resolved

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved

cameel reviewed Jul 24, 2024

View reviewed changes

aarlt force-pushed the add-asm-json-output-assembler-mode branch 3 times, most recently from 9d27078 to 905ac49 Compare July 30, 2024 19:00

cameel previously approved these changes Jul 31, 2024

View reviewed changes

cameel force-pushed the add-asm-json-output-assembler-mode branch from 905ac49 to 15e1bbc Compare July 31, 2024 11:10

cameel reviewed Jul 31, 2024

View reviewed changes

Add --asm-json option in assembler mode.

9a1d740

Co-authored-by: r0qs <[email protected]> Co-authored-by: Kamil Śliwak <[email protected]>

cameel dismissed their stale review via 9a1d740 July 31, 2024 11:13

cameel force-pushed the add-asm-json-output-assembler-mode branch from 15e1bbc to 9a1d740 Compare July 31, 2024 11:13

cameel approved these changes Jul 31, 2024

View reviewed changes

cameel enabled auto-merge July 31, 2024 11:16

cameel disabled auto-merge July 31, 2024 11:16

cameel changed the title ~~Add --asm-json option in assembler mode~~ Add --asm-json output in assembler mode Jul 31, 2024

cameel enabled auto-merge July 31, 2024 11:16

cameel merged commit 4c7e490 into develop Jul 31, 2024
72 checks passed

cameel deleted the add-asm-json-output-assembler-mode branch July 31, 2024 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --asm-json output in assembler mode #14612

Add --asm-json output in assembler mode #14612

r0qs commented Oct 13, 2023

nikola-matic commented Oct 16, 2023 •

edited

Loading

r0qs commented Oct 17, 2023 •

edited

Loading

matheusaaguiar left a comment

nikola-matic commented Dec 14, 2023

cameel Dec 15, 2023

r0qs Dec 18, 2023 •

edited

Loading

cameel Feb 8, 2024

cameel left a comment •

edited

Loading

cameel Jul 31, 2024

Add --asm-json output in assembler mode #14612

Add --asm-json output in assembler mode #14612

Conversation

r0qs commented Oct 13, 2023

nikola-matic commented Oct 16, 2023 • edited Loading

r0qs commented Oct 17, 2023 • edited Loading

matheusaaguiar left a comment

Choose a reason for hiding this comment

nikola-matic commented Dec 14, 2023

cameel Dec 15, 2023

Choose a reason for hiding this comment

r0qs Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

cameel Feb 8, 2024

Choose a reason for hiding this comment

cameel left a comment • edited Loading

Choose a reason for hiding this comment

cameel Jul 31, 2024

Choose a reason for hiding this comment

nikola-matic commented Oct 16, 2023 •

edited

Loading

r0qs commented Oct 17, 2023 •

edited

Loading

r0qs Dec 18, 2023 •

edited

Loading

cameel left a comment •

edited

Loading