Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust Yul CFG export output #15513

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Oct 14, 2024

Depends on #15505

This PR fixes a minor inconsistency in the CLI output of the Yul CFG exporter, which was missing a header when exporting Solidity sources. It also removes the targets field from places where this information is irrelevant and adds some tests for the expected output.

@r0qs r0qs added the has dependencies The PR depends on other PRs that must be merged first label Oct 14, 2024
@r0qs r0qs requested a review from clonker October 14, 2024 16:17
@r0qs r0qs mentioned this pull request Oct 14, 2024
3 tasks
@r0qs r0qs requested a review from ekpyron October 14, 2024 16:22
ekpyron
ekpyron previously approved these changes Oct 14, 2024
@ekpyron ekpyron added the 🟡 PR review label label Oct 14, 2024
@r0qs r0qs force-pushed the add-missing-yul-cfg-output-header-to-cli branch from a4f1d9d to 84313bb Compare October 14, 2024 16:30
@r0qs
Copy link
Member Author

r0qs commented Oct 14, 2024

@ekpyron I did a change adding the Yul CFG Json output to the needsHumanTargetedStdout because I believe this was also expected from Alejandro's comment. However, I noticed that we don't use that for the IR AST Json and IR Optimized AST Json export, not sure if we should keep them consistent as well.

I updated the comment above since --ast-compact-json does contain the header.

@ekpyron
Copy link
Member

ekpyron commented Oct 14, 2024

@ekpyron I did a change adding the yul cfg json output to the needsHumanTargetedStdout because I believe this was also expected from Alejandro's comment. However, I noticed that we don't use that for the ast json export, not sure if we should keep them consistent as well.

We can merge this as is in any case - we just shouldn't forget to check the other case as well :-). I'd need to look into it myself - is there a difference between it being file-based or contract-based or not? The CFG is contract based, an AST isn't, so maybe that's why? But yeah, we should check whether what currently happens makes sense. (But merge this regardless)

@r0qs
Copy link
Member Author

r0qs commented Oct 14, 2024

@ekpyron I did a change adding the yul cfg json output to the needsHumanTargetedStdout because I believe this was also expected from Alejandro's comment. However, I noticed that we don't use that for the ast json export, not sure if we should keep them consistent as well.

We can merge this as is in any case - we just shouldn't forget to check the other case as well :-). I'd need to look into it myself - is there a difference between it being file-based or contract-based or not? The CFG is contract based, an AST isn't, so maybe that's why? But yeah, we should check whether what currently happens makes sense. (But merge this regardless)

I just checked here and it is actually the IR ast exporter (and optimized version) that does not include such file header.

@r0qs r0qs force-pushed the add-missing-yul-cfg-output-header-to-cli branch from 84313bb to 198e2e2 Compare October 14, 2024 16:42
@r0qs r0qs force-pushed the add-missing-yul-cfg-output-header-to-cli branch from 198e2e2 to 9ef1aff Compare October 14, 2024 16:54
@cameel
Copy link
Member

cameel commented Oct 14, 2024

The CFG is contract based, an AST isn't, so maybe that's why?

I'm pretty sure it's not that. The AST export was just done by a contributor, who wasn't aware of such details and we didn't want to spend too much effort reviewing it either not to scare them off. Which resulted in lots of forgotten bits like this or bugs like #14452, or the 25% slowdown due to the outputs being naively always generated rather than using LazyInit which most of the other outputs use.

@r0qs r0qs force-pushed the add-missing-yul-cfg-output-header-to-cli branch 2 times, most recently from 3c1b4fb to 7972770 Compare October 15, 2024 12:58
@r0qs r0qs force-pushed the add-missing-yul-cfg-output-header-to-cli branch from 7972770 to 6dd31bb Compare October 16, 2024 11:14
Base automatically changed from protect_load_generated_ir to develop October 16, 2024 11:22
@clonker clonker dismissed ekpyron’s stale review October 16, 2024 11:22

The base branch was changed.

@ekpyron
Copy link
Member

ekpyron commented Oct 16, 2024

The CFG is contract based, an AST isn't, so maybe that's why?

I'm pretty sure it's not that. The AST export was just done by a contributor, who wasn't aware of such details and we didn't want to spend too much effort reviewing it either not to scare them off. Which resulted in lots of forgotten bits like this or bugs like #14452, or the 25% slowdown due to the outputs being naively always generated rather than using LazyInit which most of the other outputs use.

Yeah, when I commented, I did so quickly without seeing that it's about the Yul ast, for that it's definitely good/safe to add this as well.

@r0qs r0qs force-pushed the add-missing-yul-cfg-output-header-to-cli branch from 6dd31bb to c38580b Compare October 16, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dependencies The PR depends on other PRs that must be merged first 🟡 PR review label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants