ethdebug: Emit type documents and state variable pointers by djolertrk · Pull Request #1 · walnuthq/solidity

djolertrk · 2026-08-31T12:46:56Z

Populate ethdebug/format/info/resources from analysis results: every referenced type as an ethdebug/format/type document keyed by the compiler's type identifier, and the pointer of every state variable in storage and transient storage as a reusable template, derived from the storage layout (packing, mappings, dynamic arrays, bytes/string encodings, structs). Mapping keys become template parameters the consumer binds.

The internal model behind this is the semantic debug info side table specified in docs/internals/ethdebug_internal_debug_info.rst: scope records keyed by (AST ID, instance) with variables carrying identity, declaration range, type reference, phase and an ethdebug pointer, plus a versioned serialization of the table checked strictly against the public pointer schema. Numeric source IDs match the ethdebug.compilation record.

State variables whose pointer is closed, i.e. reads no template parameter and no generated Yul local, are also published in the program-level context of the ethdebug program outputs, with their declaration and type reference.

…debug outputs Selecting an ethdebug output no longer implicitly enables the ethdebug and ast-id components of the debug info selection, just as --ir-optimized does not imply --optimize: requesting a program ethdebug output without ethdebug in --debug-info or settings.debug.debugInfo is now an error, in both the CLI and Standard JSON. Semantic debug info transfer depends on @ast-id comments, so ethdebug now also depends on the ast-id component: selecting ethdebug without ast-id is an error. The ethdebug.resources and ethdebug.compilation outputs are derived from analysis results alone and still need no selection. The optimizer and SSA CFG gates now apply whenever ethdebug is selected, not only when a program output is requested.

…m-level context Populate ethdebug/format/info/resources from analysis results: every referenced type as an ethdebug/format/type document keyed by the compiler's type identifier, and the pointer of every state variable in storage and transient storage as a reusable template, derived from the storage layout (packing, mappings, dynamic arrays, bytes/string encodings, structs). Mapping keys become template parameters the consumer binds. The internal model behind this is the semantic debug info side table specified in docs/internals/ethdebug_internal_debug_info.rst: scope records keyed by (AST ID, instance) with variables carrying identity, declaration range, type reference, phase and an ethdebug pointer, plus a versioned serialization of the table checked strictly against the public pointer schema. Numeric source IDs match the ethdebug.compilation record. State variables whose pointer is closed, i.e. reads no template parameter and no generated Yul local, are also published in the program-level context of the ethdebug program outputs, with their declaration and type reference.

djolertrk · 2026-08-31T12:47:21Z

goes after the argotorg#16961

cameel

Here are some general remarks on the structure of what we have here.

Overall, I think this is still too big. It's doing more than just emitting proper resources schema. It's tangled with the data table and I think that should be separated out. On the other hand it's missing proper test coverage for types and pointers.

cameel · 2026-09-07T21:51:19Z

+	void merge(Resources const& _other);
+};
+
+Resources resources(langutil::SemanticDebugDataTable const& _table);


I think that the dependency here goes the wrong way. It's weird to first construct the data table (which represents the codegen's view of the debug data) and then get resources (analysis' view) back from that. I think that the construction of types and pointers should be independent and then be the input for the creation of the table.

The negative effect of this reversed dependency is that it's hard to cleanly split this work into smaller steps.

I think we should change this and then do it this way:

PR 1: Finish --ethdebug-resources output

Serialization of Resources to JSON

Tests based on EthdebugTest covering all Solidity types

Plug these tests into ethdebugSchemaTests (ignoring expectations, just checking that produced JSON structure is ethdebug-compliant)

PR 2: Add data table

SemanticDebugDataTable and the related data structures

Deserialization and validation of Resources from JSON

This point is pretty self-contained and could also be a separate PR

Serialization+deserialization and validation of the full data table

Including semanticDebugPointerHasInternalExpression()

New test case for the sidecar

Covers JSON->data table->JSON roundtrip

Allows testing of validation errors like in our syntax tests (expectations are the error messages)

PR 3: Add program context

.context in the --ethdebug-program output

Including semanticDebugPointerFreeNames()

Tests for program context

cameel · 2026-09-07T22:00:29Z

I don't think this approach to testing will be maintainable. Boost tests just as tedious to read as they are to write. There's also no auto-update mechanism for expectations. We're generally moving away from Boost tests for this reason in favor of isoltest.

In this case I think what we should have is a new flavor of TestCase specifically for testing the sidecar. The test could contain the data table JSON and do a similar roundtrip you're doing here, just without the first step, i.e. JSON->C++ data table->JSON (rather than C++->JSON->C++->JSON). This would also allow us to test validation, which does not seem to have any coverage right now.

cameel · 2026-09-07T22:32:06Z

+void require(bool _condition, std::string const& _message)
+{
+	solRequire(_condition, SemanticDebugDataSerializationError, _message);
+}
+
+void requireObject(Json const& _json, std::string const& _path)
+{
+	require(_json.is_object(), _path + " must be an object.");
+}
+
+void requireArray(Json const& _json, std::string const& _path)
+{
+	require(_json.is_array(), _path + " must be an array.");
+}
+
+/// Rejects members outside @a _allowed: the format is versioned, so an
+/// unknown member is a mistake rather than an extension.
+void requireOnlyMembers(Json const& _json, std::set<std::string_view> const& _allowed, std::string const& _path)
+{
+	requireObject(_json, _path);
+	for (auto const& [name, value]: _json.items())
+		require(_allowed.count(name), _path + " has an unknown member \"" + name + "\".");
+}
+
+Json const& requiredMember(Json const& _json, std::string const& _name, std::string const& _path)
+{
+	requireObject(_json, _path);
+	require(_json.contains(_name), _path + "." + _name + " is required.");
+	return _json.at(_name);
+}
+
+std::string requiredString(Json const& _json, std::string const& _name, std::string const& _path)
+{
+	Json const& value = requiredMember(_json, _name, _path);
+	require(value.is_string(), _path + "." + _name + " must be a string.");
+	return value.get<std::string>();
+}
+
+template<class T>
+T integerValue(Json const& _value, std::string const& _path)
+{
+	require(_value.is_number_integer(), _path + " must be an integer.");
+	if constexpr (std::is_unsigned_v<T>)
+	{
+		require(
+			_value.is_number_unsigned() || _value.get<Json::number_integer_t>() >= 0,
+			_path + " must not be negative."
+		);
+		require(_value.get<Json::number_unsigned_t>() <= std::numeric_limits<T>::max(), _path + " is too large.");
+	}
+	else if (_value.is_number_unsigned())
+		require(
+			_value.get<Json::number_unsigned_t>() <= static_cast<Json::number_unsigned_t>(std::numeric_limits<T>::max()),
+			_path + " is too large."
+		);
+	else
+	{
+		Json::number_integer_t const rawValue = _value.get<Json::number_integer_t>();
+		require(rawValue >= std::numeric_limits<T>::min(), _path + " is too small.");
+		require(rawValue <= std::numeric_limits<T>::max(), _path + " is too large.");
+	}
+	return _value.get<T>();
+}
+
+template<class T>
+T requiredInteger(Json const& _json, std::string const& _name, std::string const& _path)
+{
+	return integerValue<T>(requiredMember(_json, _name, _path), _path + "." + _name);
+}


Helpers like should be defined globally in libsolutil. We have a bunch of places where we parse JSON and we're repeating these things over and over again, differently each time. StandardCompiler has its checkKeys(). ASTJsonImporter its own helpers. Part of it is adapting things to specific types, but there's still a set of basic validations that could be used anyywhere.

With the evmasm import we made some effort not to repeat this so you'll now find some generic validators like isOfType() in JSON.h. See Assembly::createAssemblyItemFromJSON() and Assembly::fromJSON() for how we're using them.

TBH this is still pretty janky and I don't like how each of these places handles JSON, but we should work on improving that. ASTJsonImporter is the oldest and just goes YOLO using asserts for validation (which we haven't cleaned up to this day). Evmasm import does it sloppily (tons of holes in its validations and very little test coverage) but more generally, which is some progress.

With ethdebug let's try to improve what we have instead of building yet another local set of helpers. Please define them globally and in such a way that these other components will be later able to reuse them too.

cameel · 2026-09-07T22:35:35Z

+/// A literal in its canonical form: `0x` followed by at least one hex digit.
+bool isHexLiteral(std::string_view _text)
+{
+	if (_text.size() < 3 || _text.substr(0, 2) != "0x")
+		return false;
+	for (char const c: _text.substr(2))
+		if (!std::isxdigit(static_cast<unsigned char>(c)))
+			return false;
+	return true;
+}
+
+/// A positive decimal number without leading zeros, as `$sized<N>` requires.
+bool isPositiveDecimal(std::string_view _text)
+{
+	if (_text.empty() || _text.front() == '0')
+		return false;
+	for (char const c: _text)
+		if (!std::isdigit(static_cast<unsigned char>(c)))
+			return false;
+	return true;
+}


We already have helpers like isValidHex() and isValidDecimal().

Check out CommonData.h, StringUtils.h and other files in libsolutil also for other things you're defining helpers for. We have some of them already and if not, they can be extended or new ones added.

cameel · 2026-09-07T22:41:32Z

-		// TODO: type
-		// TODO: pointer according to ethdebug/format/spec/pointer
+		// ethdebug/format/type/specifier: a full type representation or an { "id": ... } reference.
+		std::optional<Json> type;
+		// ethdebug/format/pointer: a region or collection describing where the value lives.
+		std::optional<Json> pointer;


I'd prefer if these weren't just JSON blobs but proper data structures. Once the input is validated, we should be able to rely on the type system to provide some constraints to prevent mistakes.

Not sure why they were defined as Json in Resources below in the first place, but I think this is something we should change.

cameel · 2026-09-07T22:46:42Z

+
+/// Raised for a sidecar that is malformed and for a pointer that cannot be
+/// represented in the ethdebug pointer schema.
+struct SemanticDebugDataSerializationError: virtual util::Exception {};


You use this exception to represent validation errors but you're never catching it. This means it will be treated as an ICE.

The way we do this normally is that you catch it at a place that has access either to an ErrorReporter or at least to an output stream and there you report it to the user.

Cases where it's not supposed to be caught should be asserts. For example the handling of Unknown in various enums seems to me like it should really be an assert and not this.

BTW, the docstrings should mention any non-ICE exceptions a function may throw.

cameel · 2026-09-07T22:51:16Z

+		/// A pointer with locally defined templates: @a templates are available
+		/// by name to template references inside @a scopeTarget.
+		Templates,
+		Unknown


Does it really make sense to have Unknown in all of these enums? Looks like you never serialize this value so it seems optional might be a better choice.

Actually, you already make these enums optional in some cases, like std::optional<Location>. Do nullopt and Unknown really represent different states?

cameel · 2026-09-07T23:01:53Z

+	// Class::Group
+	std::vector<SemanticDebugPointer> group;
+
+	// Class::List
+	std::optional<SemanticDebugPointerExpression> count;
+	std::optional<std::string> indexName;
+	std::shared_ptr<SemanticDebugPointer const> listElement;
+
+	// Class::Conditional
+	std::optional<SemanticDebugPointerExpression> condition;
+	std::shared_ptr<SemanticDebugPointer const> thenPointer;
+	std::shared_ptr<SemanticDebugPointer const> elsePointer;


I don't like how SemanticDebugPointer is designed. It feels like it should be a bunch of smaller encapsulated types, built either with inheritance or variants/templates, with proper checks on what you can put inside.

Also, everything being optional hides the fact that some of these fields are actually mandatory. That should have at the very least been stated as comments.

We have something similar with AssemblyItem in libevmasm, even to the point of having factory methods for different kinds of items like you did here. I always wanted to refactor it into something more sensible. The fact that it's an open struct that anyone can modify and put in a weird state makes reasoning about what can happen to it pretty hard.

cameel · 2026-09-07T23:02:52Z

+	/// passes that clone code give each copy its own instance, so the copies
+	/// do not overwrite each other. In Yul text the instance travels in the
+	/// `@ast-id-instance` annotation next to `@ast-id`.
+	using Key = std::pair<int64_t, int64_t>;


I'd make it a struct with properly named fields. Also, ScopeID would be a better name.

djolertrk · 2026-09-08T12:33:49Z

Here are some general remarks on the structure of what we have here.

Overall, I think this is still too big. It's doing more than just emitting proper resources schema. It's tangled with the data table and I think that should be separated out. On the other hand it's missing proper test coverage for types and pointers.

Thanks a lot for your comments, but it is hard to keep the whole stack up to date, so my focus will be one (small) PR only at time, so, yeah, you can consider the rest of PRs as not ready yet. :)

cameel · 2026-09-08T13:08:28Z

That's fine. I did not expect this to be changed here and right away. Please treat it more as a forward-looking bit of feedback for when you're creating next PRs. I'm always behind on it so wanted to get a bit ahead here :)

For example, instead of you submitting this whole PR next and me requesting to split it then, I wanted to let you know that it would be better to take only the subset that what I described as PR 1 in #1 (comment).

djolertrk · 2026-09-08T13:14:29Z

That's fine. I did not expect this to be changed here and right away. Please treat it more as a forward-looking bit of feedback for when you're creating next PRs. I'm always behind on it so wanted to get a bit ahead here :)

For example, instead of you submitting this whole PR next and me requesting to split it then, I wanted to let you know that it would be better to take only the subset that what I described as PR 1 in #1 (comment).

yesyes, that was my plan as well, so we are on the same page! :) Thank you!!

cameel · 2026-09-09T04:38:34Z

but it is hard to keep the whole stack up to date, so my focus will be one (small) PR only at time

Actually, does the part adding full support for --ethdebug-resources even depend on argotorg#16961? There are definitely dependencies between it and other PRs later in the stack, but argotorg#16961 specifically seems to me like it should be mostly orthogonal to what we have here. You could try rebasing it without that commit and if it works, you could already start working on this bit.

djolertrk · 2026-09-09T10:46:02Z

but it is hard to keep the whole stack up to date, so my focus will be one (small) PR only at time

Actually, does the part adding full support for --ethdebug-resources even depend on argotorg#16961? There are definitely dependencies between it and other PRs later in the stack, but argotorg#16961 specifically seems to me like it should be mostly orthogonal to what we have here. You could try rebasing it without that commit and if it works, you could already start working on this bit.

yeah, lets start with argotorg#16990

It is marked as draft since I reached the PR limit in the original repo.

djolertrk added 2 commits August 31, 2026 14:07

djolertrk mentioned this pull request Aug 31, 2026

ethdebug: Add the semantic debug info sidecar for the Yul pipeline argotorg/solidity#16954

Draft

cameel suggested changes Sep 7, 2026

View reviewed changes

cameel mentioned this pull request Sep 15, 2026

ethdebug: Require an explicit debug info selection and ast-id argotorg/solidity#16961

Merged

djolertrk closed this Oct 1, 2026

Conversation

djolertrk commented Aug 31, 2026

Uh oh!

djolertrk commented Aug 31, 2026

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

djolertrk commented Sep 8, 2026

Uh oh!

cameel commented Sep 8, 2026

Uh oh!

djolertrk commented Sep 8, 2026

Uh oh!

cameel commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djolertrk commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cameel Sep 7, 2026 •

edited

Loading

cameel Sep 7, 2026 •

edited

Loading

cameel Sep 7, 2026 •

edited

Loading

cameel Sep 7, 2026 •

edited

Loading

cameel Sep 7, 2026 •

edited

Loading

cameel commented Sep 9, 2026 •

edited

Loading