Support ethdebug source locations under EOF #15994

clonker · 2025-04-10T09:52:56Z

Adds an EthdebugSchema header with the relevant part of the schema mapped to structs with corresponding to_json methods and validations
Skips the context for instructions with source locations that are not valid (ie, (-1, -1))
Adds source location info in evmasm assembly for assembleEOF
Refactors legacy assemble to use RAII-style instruction location gathering

Fixes the unoptimized part of #15978.
Fixes #15998.

matheusaaguiar

lgtm

cameel

I want to take a closer look at this, especially the Assembly.cpp part, but for now just a few small annoyances I found while doing a quick initial pass.

cameel · 2025-04-22T21:07:21Z

test/cmdlineTests/standard_output_debuginfo_ethdebug_compatible_eof/output.json

The output here is quite long and seems to have little to do with ethdebug itself (the ethdebug JSON gets stripped). Do we need it all? What's the point?

I think I just added it because there is the same test for non-eof. And looking at it now, I agree. There doesn't seem to be much value to keep it (or: them) around, especially with #16009 around the corner. I have removed this one.

test/cmdlineTests/standard_output_debuginfo_ethdebug_compatible_eof/input.json

test/cmdlineTests/ethdebug_eof_container_osaka/output

libevmasm/Assembly.cpp

libevmasm/EthdebugSchema.h

aarlt

LGTM!

aarlt

ok.. at least on the cli I just saw that ethdebug returns null for the interface (e.g. using test/libsolidity/semanticTests/interfaceID/homer.sol) - I guess we want an empty json object there. I think it would be nice to add a small test for that simple interface case.

aarlt · 2025-05-07T05:38:14Z

test/cmdlineTests/standard_output_debuginfo_ethdebug_with_interfaces_and_abstracts/output.json

+            "C": {
+                "evm": {
+                    "bytecode": {
+                        "ethdebug": null


ok, I thought those would get automatically removed.

it's even better if they don't. if you request ethdebug output there should be an ethdebug output artifact there that indicates that there is no ethdebug output :)

aarlt · 2025-05-07T05:39:17Z

ok.. at least on the cli I just saw that ethdebug returns null for the interface (e.g. using test/libsolidity/semanticTests/interfaceID/homer.sol) - I guess we want an empty json object there. I think it would be nice to add a small test for that simple interface case.

regarding the null stuff I changed my mind - lets fix this later if its really needed.

nikola-matic · 2025-05-14T13:06:06Z

libevmasm/Assembly.cpp

+		m_instructionLocations.emplace_back(LinkerObject::InstructionLocation{
+			.start = m_instructionLocationStart,
+			.end = end,
+			.assemblyItemIndex = m_assemblyItemIndex
+		});


Shouldn't this be as follows:

Suggested change

m_instructionLocations.emplace_back(LinkerObject::InstructionLocation{

.start = m_instructionLocationStart,

.end = end,

.assemblyItemIndex = m_assemblyItemIndex

});

m_instructionLocations.emplace_back(

m_instructionLocationStart,

end,

m_assemblyItemIndex

);

Otherwise it's equivalent to a push_back, i.e. copied instead of constructed in place.

yup but clang (in the version we use it at least) doesn't like that :( in the end it's POD though so the effect should be marginal at best

I'd suggest using push_back() in that situation though. The whole shtick of emplace_back() is that it will perform an implicit conversion. When you know the exact type, you don't really want that.

Generally I'd rather say the whole shtick is that stuff is created in-place (if possible) and we avoid an additional move. This is still the case if I know the precise type.
In our specific case there is exactly zero performance (or copy/move semantic) difference between push_back and emplace_back - both perform a move into an allocated memory region and that is it. For the sake of clarity I have changed it to push_back, though.

But the creation in place happens via an implicit conversion (basically, though choosing a constructor, though it could be one with multiple args; which is why sometimes use explicit to prevent such conversions).

I'm just pointing this out because for some time I've seen a push to use the emplace-style methods everywhere and wanted to push back against it a little :P. push_back() actually seems to me like a better default, because it's stricter about what it accepts. I'd only use emplace_back() when I know I want such a conversion or construction in-place.

I don't want to derail this PR and I think the push-back is great! Although I do have to push back a bit on the push-back. You absolutely can have an explicit constructor and still use emplace_back by virtue of its args. It is simply forwarded into std::construct_at / placement new. So this can perform implicit conversion but doesn't have to :)

For me personally it's more of a performance thing. For small objects in a tight loop or objects that are expensive to move it can make quite a bit of difference. Implicit conversions are (sometimes) mean.

nikola-matic · 2025-05-14T13:14:13Z

libevmasm/Assembly.cpp

+/// instruction locations vector.
+/// If the instruction decomposes into multiple individual evm instructions, `emit` can be
+/// called for all but the last one (which will be emitted by the destructor).
+class InstructionLocationEmitter


Was this refactor strictly necessary in this PR?

no. it does however disentangle the ethdebug metadata stuff a bit from the assembly items themselves and unifies the style.

Well, it would have been better off as a separate PR. Actually wanted to suggest that at some point.

Doesn't feel like it is necessary in this case. It is a cosmetic change and not a ton of lines of change either. Not that it matters now.

Well, the main reason is that it touches a critical part of the compiler and is completely independent of the rest of the PR. I was only really interested in this part, and it looks fine so would be best if we could merge it right away (not sure what the state of the rest is, but no reason for it to hold it back).

That makes sense. I'll extract it!

libevmasm/Assembly.cpp

Changelog.md

nikola-matic · 2025-05-14T13:58:10Z

libevmasm/Ethdebug.cpp

+	schema::program::Instruction::Operation operation;
+	operation.mnemonic = instructionInfo(static_cast<Instruction>(_linkerObject.bytecode[_start]), _assembly.evmVersion()).name;
+	static size_t constexpr instructionSize = 1;
+	if (_start + instructionSize < _end)


Might even make sense to replace this with an assert as this could end up with an empty operation, which is not desired?

there is an assert that start < end. which leaves start + 1== end as possibility, ie, an op with no argument data - which is desired and possible I'd think

nikola-matic · 2025-05-14T14:05:49Z

libevmasm/Ethdebug.cpp

+schema::materials::SourceRange::Range locationRange(langutil::SourceLocation const& _location)
+{
+	return {
+		.length = schema::data::Unsigned{_location.end - _location.start},


The assert already is in Unsigned.

cameel

I only looked at the Assembly changes so far, but they look good. We could merge that if you extract it into a separate PR.

cameel · 2025-05-14T18:20:35Z

Changelog.md

@@ -4,7 +4,7 @@ Language Features:


 Compiler Features:
-
+* ethdebug: Experimental support for instructions and source locations under EOF.


Suggested change

* ethdebug: Experimental support for instructions and source locations under EOF.

* ethdebug: Experimental support for instructions and source locations under EOF.

Uhh. Happy to fix this - but why? It's not like there's a blank line between bullet point list and headline anywhere else.

Ah, my bad, should have been the other way around - an extra empty line below :)

but why?

Two empty lines between sections.

Maybe we should just change the convention to a single line? No one seems to be able to keep it straight or see it in the diff and it gets changed by PRs back and forth. I usually ignore it but I got annoyed enough seeing it again to suggest a correction :)

Haha yeah, that explains it. Might've also been my pre-coffee confusion that I didn't realize you meant the blank line below. I am absolutely fine with changing it to one line in any case! :)

libevmasm/Assembly.cpp

clonker force-pushed the eof_source_locations_unoptimized branch 2 times, most recently from 20d6a64 to fb559fb Compare April 11, 2025 09:12

clonker marked this pull request as ready for review April 11, 2025 09:38

clonker force-pushed the eof_source_locations_unoptimized branch 9 times, most recently from e6a1055 to 2a91773 Compare April 12, 2025 06:49

clonker requested review from cameel and aarlt April 12, 2025 09:12

clonker force-pushed the eof_source_locations_unoptimized branch 5 times, most recently from d82b70c to 0fd1232 Compare April 15, 2025 07:16

clonker mentioned this pull request Apr 17, 2025

Test ethdebug program output against corresponding schema #16009

Open

clonker force-pushed the eof_source_locations_unoptimized branch from 0fd1232 to cf6aa71 Compare April 17, 2025 08:18

clonker requested a review from matheusaaguiar April 17, 2025 09:11

clonker added the ethdebug label Apr 17, 2025

matheusaaguiar previously approved these changes Apr 22, 2025

View reviewed changes

cameel reviewed Apr 22, 2025

View reviewed changes

clonker dismissed matheusaaguiar’s stale review via 19a6b42 April 23, 2025 06:16

clonker force-pushed the eof_source_locations_unoptimized branch 3 times, most recently from 3c02df0 to f9a0985 Compare April 23, 2025 06:51

clonker requested a review from cameel April 28, 2025 15:44

aarlt approved these changes Apr 29, 2025

View reviewed changes

aarlt requested changes Apr 29, 2025

View reviewed changes

clonker force-pushed the eof_source_locations_unoptimized branch 4 times, most recently from 42d5d8f to 7eecf23 Compare May 6, 2025 12:35

aarlt reviewed May 7, 2025

View reviewed changes

clonker force-pushed the eof_source_locations_unoptimized branch from 7eecf23 to e696889 Compare May 7, 2025 09:27

aarlt previously approved these changes May 9, 2025

View reviewed changes

clonker dismissed aarlt’s stale review via 667d2e3 May 9, 2025 10:37

clonker force-pushed the eof_source_locations_unoptimized branch from e696889 to 667d2e3 Compare May 9, 2025 10:37

aarlt previously approved these changes May 9, 2025

View reviewed changes

nikola-matic reviewed May 14, 2025

View reviewed changes

clonker dismissed aarlt’s stale review via bcf1c34 May 14, 2025 15:16

clonker force-pushed the eof_source_locations_unoptimized branch from 667d2e3 to bcf1c34 Compare May 14, 2025 15:16

cameel reviewed May 14, 2025

View reviewed changes

clonker force-pushed the eof_source_locations_unoptimized branch 3 times, most recently from af84571 to aad6fca Compare May 15, 2025 09:08

clonker mentioned this pull request May 15, 2025

Collect instruction location info for EOF assembly #16052

Merged

clonker added 7 commits May 15, 2025 16:03

Ethdebug requires assembly instance

78dd8ad

Ethdebug instructions output over multiple code sections

2b0a71f

Add ethdebug schema

6f5c010

Ethdebug uses schema for serialization

3c99146

Update test expectations with eof-enabled ethdebug

a58e70b

Test ethdebug output when requested in standard json under EOF

4863544

Add tests for ethdebug output on abstract contracts and interfaces

739761a

clonker force-pushed the eof_source_locations_unoptimized branch from aad6fca to 739761a Compare May 15, 2025 14:04

Changelog uses single blank line between headlines

2f4f25b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ethdebug source locations under EOF #15994

Support ethdebug source locations under EOF #15994

clonker commented Apr 10, 2025 •

edited

Loading

matheusaaguiar left a comment

cameel left a comment

cameel Apr 22, 2025

clonker Apr 23, 2025 •

edited

Loading

aarlt left a comment

aarlt left a comment •

edited

Loading

aarlt May 7, 2025

clonker May 7, 2025

aarlt commented May 7, 2025

nikola-matic May 14, 2025

clonker May 14, 2025 •

edited

Loading

cameel May 14, 2025

clonker May 15, 2025

cameel May 15, 2025 •

edited

Loading

clonker May 15, 2025 •

edited

Loading

nikola-matic May 14, 2025

clonker May 14, 2025

cameel May 14, 2025

clonker May 15, 2025

cameel May 15, 2025

clonker May 15, 2025

clonker May 15, 2025

nikola-matic May 14, 2025

clonker May 14, 2025

nikola-matic May 14, 2025

clonker May 14, 2025 •

edited

Loading

cameel left a comment

cameel May 14, 2025

clonker May 15, 2025

cameel May 15, 2025 •

edited

Loading

clonker May 15, 2025

		@@ -4,7 +4,7 @@ Language Features:


		Compiler Features:

		* ethdebug: Experimental support for instructions and source locations under EOF.

Support ethdebug source locations under EOF #15994

Are you sure you want to change the base?

Support ethdebug source locations under EOF #15994

Conversation

clonker commented Apr 10, 2025 • edited Loading

matheusaaguiar left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

aarlt left a comment

Choose a reason for hiding this comment

aarlt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarlt commented May 7, 2025

Choose a reason for hiding this comment

clonker May 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel May 15, 2025 • edited Loading

Choose a reason for hiding this comment

clonker May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker May 14, 2025 • edited Loading

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker commented Apr 10, 2025 •

edited

Loading

clonker Apr 23, 2025 •

edited

Loading

aarlt left a comment •

edited

Loading

clonker May 14, 2025 •

edited

Loading

cameel May 15, 2025 •

edited

Loading

clonker May 15, 2025 •

edited

Loading

clonker May 14, 2025 •

edited

Loading

cameel May 15, 2025 •

edited

Loading