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.

Sign up for GitHub

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

Jump to bottom

Enable ethdebug debug info and output selection. #15289

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

Enable ethdebug debug info and output selection. #15289

wants to merge 1 commit into from
+773 −26

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Jul 22, 2024 •
edited
Loading

  • 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

ekpyron
ekpyron reviewed Jul 22, 2024
View reviewed changes
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 mentioned this pull request Jul 23, 2024
Closed
@aarlt aarlt self-assigned this Jul 23, 2024
cameel
cameel reviewed Jul 24, 2024
View reviewed changes
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
nikola-matic
nikola-matic reviewed Aug 13, 2024
View reviewed changes
libsolidity/interface/CompilerStack.cpp Outdated
@@ -1091,6 +1091,14 @@ Json CompilerStack::interfaceSymbols(std::string const& _contractName) const
return interfaceSymbols;
}

Json CompilerStack::ethdebug(std::string const& _contractName) const
{
(void)_contractName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

std::ignore = _contractName; or you could even just comment out the argument name in the declaration, i.e.
Json CompilerStack::ethdebug(std::string const& /* _contractName */) const.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh wow, I didn't knew about std::ignore yet 😅

libsolidity/interface/StandardCompiler.cpp Outdated Show resolved Hide resolved
liblangutil/DebugInfoSelection.h
@@ -42,7 +42,9 @@ struct DebugInfoSelection
static DebugInfoSelection const All(bool _value = true) noexcept;
static DebugInfoSelection const None() noexcept { return All(false); }
static DebugInfoSelection const Only(bool DebugInfoSelection::* _member) noexcept;
static DebugInfoSelection const Default() noexcept { return All(); }
static DebugInfoSelection const Default() noexcept { return ExceptExperimental(); }
static DebugInfoSelection const Except(std::vector<bool DebugInfoSelection::*> const& _members) noexcept;
Copy link
Collaborator

Choose a reason for hiding this comment

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

None of this is semantically wrong, so it's really more of a stylistic thing - why are there so many of these functions, and why must we have Except(_memeber), which is only used in ExceptExperimental(), which is then made equivalent to Default(), which again, is not used everywhere, but instead omitted in lieu of the equivalent ExceptExperimental(), even though they're the same thing.

I would simply get rid of (or rather rename) Except() to ExceptExperimental(), and then use Default() everywhere (which can remain as is). Once ethdebug is promoted to non experimental, all you'll need to do is just change Default() to return All().

Copy link
Member Author

Choose a reason for hiding this comment

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

I introduced this because of @cameel's comment here. However, I would agree that this could be simplified more.

solc/CommandLineInterface.cpp Outdated Show resolved Hide resolved
solc/CommandLineParser.cpp Outdated Show resolved Hide resolved
test/cmdlineTests/debug_info_ethdebug_no_ir/args Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

For simply CLI validation, I'd suggest replacing these with test/libsolidity/interface/CommandLineParser.cpp and test/libsolidity/interface/CommandLineInterface.cpp. cmdlineTests are way too verbose for stuff like this.

@ekpyron ekpyron added the 🟡 PR review label label Aug 15, 2024
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch from c4cb445 to dfef226 Compare August 26, 2024 06:18
nikola-matic
nikola-matic reviewed Aug 26, 2024
View reviewed changes
test/solc/CommandLineInterface.cpp Outdated
TemporaryDirectory tempDir(TEST_CASE_NAME);
createFilesWithParentDirs({tempDir.path() / "input.sol"});
// output selection "asm-json" not working with debug-info "ethdebug" or "ethdebug" output selection.
OptionsReaderAndMessages result = runCLI({"solc", "--debug-info", "ethdebug", "--asm-json", tempDir.path().string() + "/input.sol"}, "");
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
OptionsReaderAndMessages result = runCLI({"solc", "--debug-info", "ethdebug", "--asm-json", tempDir.path().string() + "/input.sol"}, "");
OptionsReaderAndMessages result;

Got a duplicate of this case on line 1430.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You know, if Kamil was here, he'd say 'you should put this options into a std::vector and iterate through them in order to make the test more readable'. :D

I will say though - it would make sense here, since we typically also assert the correctness of the error message (which you're not doing here).

test/solc/CommandLineInterface.cpp
BOOST_REQUIRE(!result.success);
}

BOOST_AUTO_TEST_CASE(cli_ethdebug_incompatible_input_modes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in test/CommandLineParser.cpp

test/solc/CommandLineInterface.cpp
BOOST_REQUIRE(result.stdoutContent.find("ethdebug") == std::string::npos);
}

BOOST_AUTO_TEST_CASE(cli_ethdebug_incompatible_outputs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in test/CommandLineParser.cpp

test/solc/CommandLineInterface.cpp
@@ -1412,6 +1412,130 @@ BOOST_AUTO_TEST_CASE(cli_include_paths_ambiguous_import)
BOOST_REQUIRE(!result.success);
}

BOOST_AUTO_TEST_CASE(cli_ethdebug_no_ethdebug_in_help)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in test/CommandLineParser.cpp

test/solc/CommandLineInterface.cpp
BOOST_REQUIRE(result.stdoutContent.find("/// ethdebug: enabled") != std::string::npos);
// --ethdebug only works with --via-ir.
result = runCLI({"solc", "--debug-info", "ethdebug", "--ethdebug", tempDir.path().string() + "/input.sol"}, "");
BOOST_REQUIRE(!result.success);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These negative cases are already tested covered in other tests - they don't need to be here.

test/solc/CommandLineInterface.cpp
BOOST_REQUIRE(result.stdoutContent.find("/// ethdebug: enabled") == std::string::npos);
}

BOOST_AUTO_TEST_CASE(cli_ethdebug_ethdebug_output)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, given that you're covering the existence of /// ethdebug: enabled in generated output for different CLI combinations, it's definitely better that they're all here instead of in a million cmdlineTests with massive outputs - but it would none the less be good to have at least one cmdlineTest that should the full generated output (a smoke test if you will).

@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch from f1a73c3 to 93749ff Compare August 28, 2024 12:05
@aarlt aarlt mentioned this pull request Aug 30, 2024
Open
2 tasks
@aarlt aarlt force-pushed the enable_debug_info_ethdebug branch from 93749ff to 0dd51e5 Compare August 30, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel left review comments

@ekpyron ekpyron ekpyron left review comments

@nikola-matic nikola-matic nikola-matic left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@aarlt aarlt

Labels
ethdebug 🟡 PR review label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aarlt @cameel @ekpyron @nikola-matic