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

Enable ethdebug debug info and output selection. #15289

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Jul 22, 2024

  • if debug-info was set to ethdebug
    • it will only work if also ir, irOptimized and/or ethdebug was selected as output.
      standard_debug_info_in_yul_ethdebug_output_ir_optimized, standard_debug_info_in_yul_ethdebug_output_no_ir
    • it will always work with strict-assembly e.g. solc --strict-assembly <yul> --debug-info ethdebug
    • debug-info ethdebug is excluded from the help on cli
    • debug-info ethdebug is excluded from all on cli and wildcard selection * in standard-json
  • if ethdebug was selected as output
    • if no debug-info was selected, it implicitly set debug-info to ethdebug. solc <contract> --ethdebug
    • if via-ir was not specified, it will error with a message stating that ethdebug can only be selected as output, if via-ir was defined. solc <contract> --ethdebug only works with --via-ir
    • if debug-info was selected and did not contain ethdebug, an error will be generated stating that ethdebug need to be set in debug-info solc <contract> --ethdebug --debug-info location
    • strict-assembly will always work e.g. solc --strict-assembly <yul> --ethdebug
    • output selection ethdebug is not shown in cli help
    • ethdebug output selection is excluded from wildcard selection * in standard-json

UPDATE
After some discussion with @gnidan and @ekpyron it turned out that we need something slightly different:

  • we need to be able to distinguish ethdebug debug data from deploy-time and run-time.
  • standard-json:  ethdebug output will now be enabled with evm.bytecode.ethdebug(deploytime part) and evm.deployedBytecode.ethdebug (runtime part)
    • special case: output selection of evm.bytecodeand evm.deployedBytecodebehave like a wildcard, so the ethdebug stuff is excluded here.
  • cli: like binary selection --bin and --bin-runtime ethdebug selection will now work similar with --ethdebug and --ethdebug-runtime

solc/CommandLineParser.cpp Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch 3 times, most recently from ccbe426 to 71bf655 Compare July 23, 2024 14:08
@ethereum ethereum deleted a comment from stackenbotten Jul 23, 2024
@aarlt aarlt self-assigned this Jul 23, 2024
libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved
libsolidity/interface/StandardCompiler.cpp Outdated Show resolved Hide resolved
libsolidity/interface/StandardCompiler.cpp Outdated Show resolved Hide resolved
solc/CommandLineParser.cpp Outdated Show resolved Hide resolved
liblangutil/DebugInfoSelection.cpp Outdated Show resolved Hide resolved
@cameel cameel changed the title [ethdebug] Enable ethdebug debug info selection. Enable ethdebug debug info selection Jul 24, 2024
@aarlt aarlt marked this pull request as draft July 24, 2024 11:14
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch 5 times, most recently from ec729b3 to ee2bc12 Compare July 24, 2024 23:05
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch from ee2bc12 to 9772d20 Compare August 1, 2024 18:51
@aarlt aarlt changed the title Enable ethdebug debug info selection Enable ethdebug debug info and output selection. Aug 1, 2024
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch 2 times, most recently from 09fd459 to 53e12ef Compare August 5, 2024 11:12
@ethereum ethereum deleted a comment from stackenbotten Aug 6, 2024
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch 4 times, most recently from 4b5c548 to a262eef Compare August 7, 2024 16:00
@aarlt aarlt marked this pull request as ready for review August 7, 2024 16:00
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch from a262eef to c4cb445 Compare August 12, 2024 10:30
@nikola-matic nikola-matic self-requested a review August 12, 2024 13:30
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch 6 times, most recently from 32b21f6 to fe6a4ca Compare September 24, 2024 15:51
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch from 7917ca3 to 13c0683 Compare October 2, 2024 09:06
libyul/YulStack.h Show resolved Hide resolved
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch 6 times, most recently from c4faae4 to c9417ed Compare October 8, 2024 10:32
[](const Json& result)
{
return result["contracts"]["fileA"]["contractA"]["evm"]["deployedBytecode"].contains("ethdebug") &&
result["contracts"]["fileB"]["contractB"]["evm"]["bytecode"].contains("ethdebug") ;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result["contracts"]["fileB"]["contractB"]["evm"]["bytecode"].contains("ethdebug") ;
result["contracts"]["fileB"]["contractB"]["evm"]["bytecode"].contains("ethdebug");

for (auto const& selectedArtifactJson: _outputSelection)
{
std::string const& selectedArtifact = selectedArtifactJson.get<std::string>();
if (
_artifact == selectedArtifact ||
boost::algorithm::starts_with(_artifact, selectedArtifact + ".")
)
{
if (_artifact.find("ethdebug") != std::string::npos)
// only accept exact matches for ethdebug, e.g. evm.bytecode.ethdebug
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have this covered by a test, e.g. requesting ethdebugs instead of ethdebug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah good point! I guess ethdebugs will be detected too ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.. this is not an issue because of the next line return selectedArtifact == _artifact;

Copy link
Member Author

@aarlt aarlt Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that the error handling of the output selection is horrible.. there are no error messages at all, if someone selects a non-existing selection option.. thats why I will also not do anything about this.. I just checked, whether evm.bytecode.ethdebugs/evm.deployedBytecode.ethdebugsis accidentally selecting ethdebug - but this is not the case

return false;

for (auto const& fileRequests: _outputSelection)
for (auto const& requests: fileRequests)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requests is just a list of contracts (wildcard included) right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep exactly

std::nullopt,
},
{
generateStandardJson(false, Json::array({"ethdebug"}), Json::array({"ir"})),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I still think you should have one or two cmdline tests for ethdebug - one for the CLI and one for StandardJSON. These should just be happy case smoke tests. I know they're covered here, but this is significantly more difficult to read than a cmdline test :D

Copy link
Member Author

@aarlt aarlt Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah true! thanks for the reminder, I wanted to add some basic smoke tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was definitely worth to add those tests, because it revealed a small bug in cli parameter handling in the context of strict assembly :) also with yul in standard json the top-level ethdebug object was missing in the output

@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch 4 times, most recently from 4d01075 to ada3616 Compare October 15, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ethdebug 🟡 PR review label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants