Add via SSA CFG in CLI and Standard Json Interface#16503
Conversation
blishko
left a comment
There was a problem hiding this comment.
Looks reasonable to me, left some questions.
I'll leave approval to someone who has worked with CLI/JSON arguments before.
cameel
left a comment
There was a problem hiding this comment.
Not going to do a proper review now, but since the PR was up already I decided to take a peek and I see that you're not storing the state of the flag in metadata. This is very important for source verification. All settings necessary to reproduce the bytecode must be stored in metadata.
This aspect also needs test coverage.
There was a problem hiding this comment.
There are a few more test cases here where the new option is relevant: invalid_options_input_modes_combinations, assembly_mode_options, cli_mode_options.
There was a problem hiding this comment.
I have added tests to these test cases. It is a bit unfortunate for experimental features in the invalid_options_input_modes_combinations because I can't, e.g., test that --via-ssa-cfg --standard-json is an invalid combination inside of the test definition, it will error out differently:
solc --via-ssa-cfg --standard-json input.json
> Error: The following options are only available in experimental mode: --via-ssa-cfg. To enable experimental mode, use the --experimental flag.
solc --experimental --via-ssa-cfg --standard-json input.json
> Error: Standard JSON input mode is incompatible with the --experimental flag. Instead, please use the 'settings.experimental' setting in your Standard JSON input file to enable experimental mode.The only thing I could effectively test it against (without changing the test itself) was --link).
There was a problem hiding this comment.
I think it would be fine to change the test and accept this alternative error message too.
BTW, I see that the evmasm import mode is not in that table and it really should be. Especially for --via-ssa-cfg, which does not make sense in this mode.
There was a problem hiding this comment.
Yep you are right of course. I have added --import-asm-json to the list. Let's do the asm json for the rest in a follow up.
260ecd6 to
e08dba6
Compare
b1caac3 to
79612c5
Compare
There was a problem hiding this comment.
The only thing that's missing in the implementation are validations that SSA CFG codegen is not requested in evmasm import mode. Since that mode is experimental, I'd be fine with addressing that in a separate PR, especially given that this validation is missing for many other options too.
Other than that, mostly wording and test tweaks.
There was a problem hiding this comment.
viaSSACFG: true should be disallowed in EVMAssembly mode (which goes through importEVMAssembly() and just ignores the setting).
I see that we're not disallowing viaIR there either. That perhaps should be fixed in a separate PR covering all the stray options we're allowing there but shouldn't.
There was a problem hiding this comment.
I think it would be fine to change the test and accept this alternative error message too.
BTW, I see that the evmasm import mode is not in that table and it really should be. Especially for --via-ssa-cfg, which does not make sense in this mode.
There are still minor things to improve, but overall the PR is no longer missing anything vital.
3444570 to
a73b2a9
Compare
cameel
left a comment
There was a problem hiding this comment.
It's 99.9% good, so approving already, but please remember to remove the Standard JSON changelog entry (#16503 (comment)) before you merge.
537bccc to
0144a43
Compare
0144a43 to
11beecc
Compare
Expose the SSA CFG pipeline through the CLI (
--via-ssa-cfg) and Standard JSON (settings.viaSSACFG) as an experimental feature. Both flags implyviaIRand require experimental mode.