Make ethdebug inputs/outputs consistent#16565
Conversation
2e64751 to
a1c530b
Compare
| m_options.compiler.outputs.ethdebug || | ||
| m_options.compiler.outputs.ethdebugRuntime || | ||
| m_options.compiler.outputs.ethdebugProgram || | ||
| m_options.compiler.outputs.ethdebugProgramRuntime || |
There was a problem hiding this comment.
@nikola-matic What was the thing you wanted to do with --stop-after? The way I see it, you only need to touch CompilerStack::PipelineConfig. It defines which parts of the pipeline should be enabled. Perhaps we just need separate entry for analysis in it.
TBH these two things are kinda redundant and we should probably just integrate stopAfter into PipelineConfig. We could do that just fine without changing compiler's interface. Both tell CompilerStack which stages to execute, they were just created at different times for different purposes.
But not sure if you want to get into that now. For the purpose of this PR I think it will be enough to just add relevant fields to PipelineConfig and if they happen to overlap with stopAfter, just use asserts and validations to ensure they do not contradict each other.
There was a problem hiding this comment.
Yup, I saw it after the call. I'll ping you for review once the PR is in better shape.
6eb6ffd to
1986be6
Compare
clonker
left a comment
There was a problem hiding this comment.
Nice cleanup! Mainly two things though:
- the
requiresFullCompilation/ skiplink()thing seems orthogonal and should probably be extracted into its own pr unless i'm missing the point here - global ethdebug output assumes analysis succeeded. this should be tested and guarded in the implementation
the rest is nits and a design question about the standard json interface (the * selection for global settings seems a bit awkward to me) as well as if we want to add a --ethdebug-full or just --ethdebug output as short-hand for the common case.
| // evm.gasEstimates - Function gas estimates | ||
| // yulCFGJson - Control Flow Graph (CFG) of the Single Static Assignment (SSA) form of the contract (experimental) | ||
| // | ||
| // Global level (needs "*" as file name and "*" as contract name): |
There was a problem hiding this comment.
not sure why it needs "*" for file and contract name, can this be solved differently? wouldn't it be better as flat top-level key if it's actually global?
| +-----------------------+--------------------------+------------------+-----------------------------------------------------------------------------------------------------------------------------------------+ | ||
| | Non-mainnet EVMs | ``evm`` | yes | ``--evm-version <version name>`` | | ||
| +-----------------------+--------------------------+------------------+-----------------------------------------------------------------------------------------------------------------------------------------+ | ||
| | Ethdebug | ``ethdebug`` | no | ``--ethdebug-resources``, ``--ethdebug-compilation``, ``--ethdebug-program``, ``--ethdebug-program-runtime``, ``--debug-info ethdebug`` | |
There was a problem hiding this comment.
what's the default now, actually? is that somehow reflected in the docs?
There was a problem hiding this comment.
Default in what way?
There was a problem hiding this comment.
Sorry I confused myself here. What I meant were the default settings for emitting debug info. Which, obviously, is to not emit anything unless debug info output is requested. So nothing to change or even discuss in that regard.
Tangentially (and where my confusion came from): I've always found it a bit funny that the default behavior of IR output is to attach source location info.
| if (requiresFullCompilation) | ||
| this->link(); |
There was a problem hiding this comment.
how is this (as well as the perf optimization above) related to ethdebug? isn't this orthogonal?
There was a problem hiding this comment.
Sure, I guess you could say so, although it was a requirement in the issue. E.g. requiresting ethdebug.compilation, which just gives the compiler details shouldn't really trigger full compilation.
| if (m_options.output.viaSSACFG) | ||
| solUnimplemented("ethdebug is not yet supported with --" + g_strViaSSACFG + "."); |
There was a problem hiding this comment.
out of curiosity, what happens if you do request it? at least some information should propagate, i think
There was a problem hiding this comment.
I'll check and report back :)
There was a problem hiding this comment.
test.sol:
contract C
{
uint256 x = 1;
function foo() public view returns(uint256) {
return x;
}
}Running solc --experimental --ethdebug-program --via-ssa-cfg test.sol outputs the following:
======= test.sol:C =======
Debug Data (ethdebug/format/program):
{"contract":{"definition":{"source":{"id":0}},"name":"C"},"environment":"create","instructions":[{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":0,"operation":{"arguments":["0x80"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":2,"operation":{"arguments":["0x40"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":4,"operation":{"mnemonic":"MSTORE"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":5,"operation":{"mnemonic":"CALLVALUE"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":6,"operation":{"arguments":["0x22"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":8,"operation":{"mnemonic":"JUMPI"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":9,"operation":{"arguments":["0x0e"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":11,"operation":{"arguments":["0x8b"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":13,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":14,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":15,"operation":{"arguments":["0x14"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":17,"operation":{"arguments":["0x26"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":19,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":20,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":21,"operation":{"arguments":["0x0142"],"mnemonic":"PUSH2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":24,"operation":{"arguments":["0x0097"],"mnemonic":"PUSH2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":27,"operation":{"mnemonic":"DUP3"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":28,"operation":{"mnemonic":"CODECOPY"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":29,"operation":{"arguments":["0x0142"],"mnemonic":"PUSH2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":32,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":33,"operation":{"mnemonic":"RETURN"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":34,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":35,"operation":{"arguments":["0x2c"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":37,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":38,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":39,"operation":{"arguments":["0x40"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":41,"operation":{"mnemonic":"MLOAD"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":42,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":43,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":44,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":45,"operation":{
"mnemonic":"PUSH0"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":46,"operation":{"mnemonic":"DUP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":47,"operation":{"mnemonic":"REVERT"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":48,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":49,"operation":{"mnemonic":"PUSH0"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":50,"operation":{"mnemonic":"SHL"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":51,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":52,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":53,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":54,"operation":{"mnemonic":"PUSH0"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":55,"operation":{"mnemonic":"NOT"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":56,"operation":{"mnemonic":"SWAP2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":57,"operation":{"arguments":["0x3f"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":59,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":60,"operation":{"arguments":["0x30"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":62,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":63,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":64,"operation":{"mnemonic":"DUP3"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":65,"operation":{"mnemonic":"NOT"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":66,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":67,"operation":{"mnemonic":"SWAP2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":68,"operation":{"mnemonic":"AND"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":69,"operation":{"mnemonic":"SWAP2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":70,"operation":{"mnemonic":"AND"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":71,"operation":{"mnemonic":"OR"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":72,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":73,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":74,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":75,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":76,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":77,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":78,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":79,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":80,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":81,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":82,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":83,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":84,"operation":{"arguments":["0x5a"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":86,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":87,"operation":{"arguments":["0x4a"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":89,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":90,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":91,"operation":{"arguments":["0x61"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":93,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":94,"operation":{"arguments":["0x50"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":96,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":97,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":98,"operation":{"arguments":["0x68"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":100,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":101,"operation":{"arguments":["0x4d"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":103,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":104,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":105,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":106,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":107,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":108,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":109,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":110,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":111,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":112,"operation":{"arguments":["0x76"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":114,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":115,"operation":{"arguments":["0x53"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":117,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":118,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":119,"operation":{"arguments":["0x7d"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":121,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":122,"operation":{"arguments":["0x6b"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":124,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":125,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":126,"operation":{"mnemonic":"DUP2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":127,"operation":{"mnemonic":"SLOAD"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":128,"operation":{"arguments":["0x87"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":130,"operation":{"mnemonic":"SWAP2"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":131,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":132,"operation":{"arguments":["0x35"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":134,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":135,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":136,"operation":{"mnemonic":"SWAP1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":137,"operation":{"mnemonic":"SSTORE"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":138,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":139,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":140,"operation":{"arguments":["0x94"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":142,"operation":{"arguments":["0x01"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":94,"offset":26},"source":{"id":0}}},"offset":144,"operation":{"mnemonic":"PUSH0"}},{"context":{"code":{"range":{"length":1,"offset":53},"source":{"id":0}}},"offset":145,"operation":{"arguments":["0x6e"],"mnemonic":"PUSH1"}},{"context":{"code":{"range":{"length":1,"offset":53},"source":{"id":0}}},"offset":147,"operation":{"mnemonic":"JUMP"}},{"context":{"code":{"range":{"length":1,"offset":53},"source":{"id":0}}},"offset":148,"operation":{"mnemonic":"JUMPDEST"}},{"context":{"code":{"range":{"length":1,"offset":53},"source":{"id":0}}},"offset":149,"operation":{"mnemonic":"JUMP"}}]}
Didn't wanna pretty print so as not to take up too much vertical comment space :)
There was a problem hiding this comment.
Ah sweet, so it does emit ethdebug! Perhaps not the full granularity but yeah, something. :D
| @@ -1 +1 @@ | |||
| --experimental --ethdebug --via-ir --pretty-json --json-indent 4 | |||
| --experimental --ethdebug-compilation --ethdebug-program --ethdebug-program-runtime --via-ir --pretty-json --json-indent 4 | |||
There was a problem hiding this comment.
i get that this is to make things uniform but from a UX point of view.. ugh.. :) what do you think about --ethdebug-full or something? i guess that's the target use-case?
There was a problem hiding this comment.
Current decision is to not have a top level (full) ethdebug output - it's all experimental, and we'll see down the road if it makes sense.
There was a problem hiding this comment.
yeah i also don't want to waste your time with this. just seemed a bit annoying to have to provide four cli flags for the full ethdebug output is all
There was a problem hiding this comment.
In practice, most everyone (and especially tooling) rely on standard JSON, where this granular approach makes significantly more sense - the purpose of this PR is to introduce equivalence between the two, which I do agree is quite inconvenient given that we're mapping each stdjson option into a flag :)
There was a problem hiding this comment.
Right. Well let's see. Maybe we can stomach the small divergence of introducing an additional flag in the cli that groups together standard json settings. If things become too annoying down the line. :)
| } | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(cli_ethdebug_ethdebug_output) |
There was a problem hiding this comment.
ethdebug_cli_flag_combinations?
anyways for the whole thing i think such a structure would be nice:
BOOST_AUTO_TEST_SUITE(CommandLineInterfaceTest)
BOOST_AUTO_TEST_SUITE(Ethdebug)
BOOST_AUTO_TEST_CASE(rejects_optimize_with_program) { ... }
BOOST_AUTO_TEST_CASE(rejects_program_without_via_ir) { ... }
BOOST_AUTO_TEST_CASE(accepts_resources_standalone) { ... }
BOOST_AUTO_TEST_CASE(accepts_compilation_standalone) { ... }
BOOST_AUTO_TEST_SUITE_END()
BOOST_AUTO_TEST_SUITE_END()There was a problem hiding this comment.
All of the test cases start with cli_* (except for standard json onses), so cli_ethdebug_ethdebug_output is more consistent.
As far as adding a separate suite for ethdebug and separating each combination into a separate test case - I do agree that would be significantly better in almost every way, and failures would also be significantly easier to find, instead of trying to figure out which combination in a vector of combinations caused a failure.
I don't necessarily think it's worth a refactor at the moment, since the long term plan is to migrate all cmdLineTests to isoltest, which would then make all of these boost tests redundant (i.e. we would move all of them to the new isoltest as well).
There was a problem hiding this comment.
OK, that makes sense. I don't think I'd mind an enclosing Ethdebug suite around the ethdebug tests in the current boost test setup and it's like a 2 lines of code. But also completely optional and a bit cosmetic. Restructuring the tests is a lot more work. Seems like there is no free lunch after all :)
All of the requirements are in the issue, including the full/partial compilation requirements as well as the |
1986be6 to
74f8ebf
Compare
clonker
left a comment
There was a problem hiding this comment.
I think it's good code-wise now, the remaining points are more general design questions and/or refactorings and improvements that can be done as follow-ups. I think it'd be good to do the templated areArtifactsRequested + static constexpr arrays when possible but I'll leave that up to you.
Other than that I think it would be good to have test coverage for stopAfter: parsing with ethdebug.compilation and/or ethdebug.resources requested.
Leaves the final point: changelog? :)
db47406 to
b006193
Compare
b006193 to
32a5a84
Compare
Closes #16460