ethdebug: Improve current testing infra by djolertrk · Pull Request #16727 · argotorg/solidity

djolertrk · 2026-05-15T14:44:54Z

Handle multiple things ethdebug related:

Add LLVM lit tests (this could be interesting for codegen tests as well)
```
$ lit -sv test/lit --param solc=build/solc/solc
```
Emit schema-compliant compilation resources so we can execute first "round-trip-tests" in producer (here) and in consumer soldb via conformance tests added into the format repo
Validate ETHDebug output against pinned schemas

nikola-matic

As discussed earlier, can you please migrate the llvm-lit test to the new ethdebug isoltest (#16675 - merged this morning)?

I'll be doing some follow up tasks regarding the ethdebug isoltest, so feel free to let me know if you have any requests :)

nikola-matic · 2026-05-21T11:58:36Z

You should be able to get rid of all of these (brink, ens and prb-math), i.e. just drop the commits after a rebase - I fixed these some time ago.

Very nice, I will do it.

djolertrk · 2026-05-21T13:55:25Z

As discussed earlier, can you please migrate the llvm-lit test to the new ethdebug isoltest (#16675 - merged this morning)?

I'll be doing some follow up tasks regarding the ethdebug isoltest, so feel free to let me know if you have any requests :)

Hi @nikola-matic, thanks for the feedback! Yes sure! I will do it, and also include into CI the ethdebug compilation overhead (almost ready), and of course, if something is missing in the ethdebug isoltest, I will let you know.

And just to confirm, we do not want to include llvm lit at all in this PR, right?

nikola-matic · 2026-05-21T14:44:48Z

And just to confirm, we do not want to include llvm lit at all in this PR, right?

Correct - team is very much used to the isoltest way of doing things, and reviewing will go much smoother with isoltest instead of llvm-lit for the time being.

djolertrk · 2026-05-22T12:17:21Z

This PR depends on these changes outside of this repo:

nikola-matic

Looks good - you went a bit overboard with the CI though :)

nikola-matic · 2026-05-23T13:24:34Z

+void appendLengthPrefixed(std::string& _target, std::string_view _value)
+{
+	_target += std::to_string(_value.size());
+	_target += ":";
+	_target += _value;
+}


Suggested change

void appendLengthPrefixed(std::string& _target, std::string_view _value)

{

_target += std::to_string(_value.size());

_target += ":";

_target += _value;

}

std::string lengthPrefixed(std::string_view _value)

{

return fmt::format("{}:{}", _value.size(), _value);

}

nikola-matic · 2026-05-23T13:25:19Z

+	_target += _value;
+}
+
+std::string compilationID(std::vector<Source> const& _sources)


You could also apply fmt::format here, but it's not a dealbreaker.

nikola-matic · 2026-05-23T13:31:23Z

          command: |
            pytest test/ethdebugSchemaTests --solc-binary-path=/tmp/workspace/solc/solc-static-linux -v

+  t_ethdebug_overhead:


I wouldn't add a CI step for benchmarking at this stage - running it on every commit incurs additional cost for us, and the vast majority of PRs will not be touching anything ethdebug related (in terms of the entire repository, ethdebug PRs obviously will).

Also, what would be the failure condition for such a step? I don't see one here - it seems that it only runs the benchmark three times and stores the (average?) result as an artifact.

For the time being, just get rid of all of the CircleCI changes, and instead let's agree to dump the results as a comment in the PR (as a table, or whatever format you deem acceptable).

The extention tot_ethdebug_output_validity is fine.

Also, what would be the failure condition for such a step? I don't see one here - it seems that it only runs the benchmark three times and stores the (average?) result as an artifact.

For the time being, just get rid of all of the CircleCI changes, and instead let's agree to dump the results as a comment in the PR (as a table, or whatever format you deem acceptable).

Well, this tests the compilation overhead, so definitely it needs to be invoked several times, and to report the if the "overhead" is more than a trash-hold (atm I put > 50%). It could be pretty heavy to keep it as default in the CI, so yes, let's remove it for now.

And this solc-bench-change is the PR that introduces the new benchmark there, so we can discuss the format of the output there, right?

Please check this: argotorg/solc-bench#12 (comment)

Given that it would be a bit much to run this in the CI for every push, let's agree to just paste the benchmarking results from solc-bench to solc PRs when they're opened, so we can see the effects.

nikola-matic · 2026-05-23T13:36:37Z

+{
+	schema::materials::Source result;
+	result.id = schema::materials::ID{_source.id};
+	result.path = _source.path;
+	result.contents = _source.contents;
+	result.encoding = std::nullopt;
+	result.language = _source.language;
+	return result;


Suggested change

{

schema::materials::Source result;

result.id = schema::materials::ID{_source.id};

result.path = _source.path;

result.contents = _source.contents;

result.encoding = std::nullopt;

result.language = _source.language;

return result;

{

return {

.id = schema::materials::ID{_source.id},

.path = _source.path,

.contents = _source.contents,

.encoding = std::nullopt,

.language = _source.language,

};

c++20 very nice 👍 You can also drop .encoding = std::nullopt, since it'll be default initialized as such.

okay, I agree, c++20 it is :)

It seems that CI Windows job rejects C++20

Bleeeh, this .id = schema::materials::ID{_source.id} is the offender that triggers an ICE in MSVC, and is only fixed in MSVC 19.33 (family 17.3). Unfortunately, this would mean we'd have to bump MSVC from 2019 to 2022, and we're not doing that now.

Revert it back to how it was then.

nikola-matic · 2026-05-23T13:37:25Z

+	for (auto const& source: _sources)
+		sources.emplace_back(materialSource(source));
+
+	schema::materials::Compilation result;


Named struct initialization here as well.

nikola-matic · 2026-05-25T08:19:44Z

+// .compilation.id: <IGNORE>
+// .compilation.sources | length: 1
+// .compilation.sources[0].id: 0
+// .compilation.sources[0].path: <IGNORE>


You should test this in test/libsolidity/ethdebugTests/multi_source_name_collision.sol and test/libsolidity/ethdebugTests/multi_source.sol as it's multisource with paths.

You can also create a new test if you feel like something is undertested, or the current tests don't allow for the best representation.

at least for these basic ETHDebug things, no, but in future, once we start adding more complex things into it (like variable values), it will definitely need to evolve a bit.

github-actions Bot added the external contribution ⭐ label May 15, 2026

djolertrk marked this pull request as draft May 15, 2026 14:53

nikola-matic self-assigned this May 18, 2026

djolertrk force-pushed the feature/ethdebug branch 2 times, most recently from 4c86c71 to 5186b7e Compare May 19, 2026 14:07

nikola-matic reviewed May 21, 2026

View reviewed changes

djolertrk added 2 commits May 21, 2026 17:55

ethdebug: Emit schema-compliant compilation resources

78aa6de

test: Validate ETHDebug output against pinned schemas

cc430fb

djolertrk force-pushed the feature/ethdebug branch from 5186b7e to 7f45821 Compare May 22, 2026 09:35

ci: Check ETHDebug compilation overhead

efaef8d

djolertrk force-pushed the feature/ethdebug branch from 7f45821 to efaef8d Compare May 22, 2026 10:05

nikola-matic reviewed May 25, 2026

View reviewed changes

ethdebug: Address resource review feedback

a5686ef

djolertrk force-pushed the feature/ethdebug branch from dcd8d7b to a5686ef Compare May 25, 2026 14:41

djolertrk marked this pull request as ready for review May 25, 2026 14:50

ethdebug: Avoid MSVC ICE in schema material builders

b90b7c0

nikola-matic approved these changes May 28, 2026

View reviewed changes

nikola-matic merged commit c28af75 into argotorg:develop May 28, 2026
83 checks passed

Conversation

djolertrk commented May 15, 2026

Uh oh!

nikola-matic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djolertrk commented May 21, 2026

Uh oh!

nikola-matic commented May 21, 2026

Uh oh!

djolertrk commented May 22, 2026

Uh oh!

nikola-matic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants