Add support for instructions and source ranges.

[aarlt]

• to reduce test-size ethdebug data will now be removed from standard-json based cmdline tests
• simple smoke test that is just checking the basic structure of instructions
• ethdebug related command line tests that are not using standard-json got removed - removal of ethdebug data is easier with standard-json - correct behaviour of the command line is still checked in soltest test suit

[clonker]

Why can there only be one?

[aarlt]

only eof can have more

[cameel]

But this should be solUnimplemented not an assert. Or rather, it can be an assert but only if you can assume it will never fail. And for this in all outputs exposed to the user you should have solUnimplemented checks that EOF is not enabled (note that existing outputs already have such checks).

The difference is that the user will get a nice message that this is not supported on EOF rather than an ugly ICE that he will report as a bug. On top of that, with solUnimplemented you can (and should) have a test covering that corner case.

[aarlt]

I just changed that to an solUnimplementedAssert. need to add a test.

[aarlt]

ok.. simple test added now

[clonker]

I am still not convinced having 28k lines of test output is a good idea. There is no real way of assessing whether the test result is correct and in case of a test expectation mismatch I imagine one can only really see that "something" changed. Is there a way to make this more concise?

[veniger]

Are there issues tracking ethdebug support? Could we create an issue for each part of the spec, so we can track what is supported and what is in development (like: source locations, local variables, etc.) @ekpyron

[clonker]

First impressions :) I'll have another go at this tomorrow

[cameel]

I skimmed through the PR and found some things that looks like bugs and discrepancies with the spec. Is this tested properly?

[cameel]

Don't assume you can create random junk files in the test dir. It may not even be writable. Use a proper temporary file or capture it in a variable.

And please don't randomly switch to 2-space indents in the middle of the file.

[cameel]

Also, why is grep needed here? If you're trying to ensure that the .ethdebug key exists, this will have false-positives. You should check that using jq.

[aarlt]

ok, I will use a proper termporary file for this.

however, I did that grepstuff because our pretty-json is sadly creating a different formatting as jq - so I decided to only do this reformatting if ethdebug is found in the output - I know its not very ellegant. however, maybe we should always use jq for json reformatting - but this should be a different PR.

[cameel]

I meant why use grep if you can check that more reliably with jq. See #15368 (comment).

I did that grepstuff because our pretty-json is sadly creating a different formatting as jq

Ah, right, I did not realize it was messing up the formatting. You should really add comments for such things. But ok, I guess, as ugly as this is, it is unavoidable.

We should at some point extract all this logic for cleaning output files into a separate function or even script. That would at least keep the runner script more compact. But not in this PR in any case.

however, maybe we should always use jq for json reformatting - but this should be a different PR.

Well, I used to not like that idea, but considering the awful regexes we need to cut out the other things, maybe this would actually be lesser evil here.

The annoying thing is that this would reformat all our tests and create a massive PR.

[cameel]

Assuming this is from Source range schema, shouldn't this be singular?

Suggested change

	"sources",
	"source",

[cameel]

Also, what's up with the double braces? Doesn't this create a one-element array or something?

You also have a weird double indent under "contract".

[aarlt]

with nlohmann json double braces are needed to ensure that an object is created - if not - depending on the structure - it may become an array

[cameel]

Hmm... is this why you now replaced it all with assignments and manually created objects? :)

[clonker]

A more modern version of manually incrementing the instruction index would be to use ranges::views::enumerate(items) :)

Amounts to something like for (auto const& [instructionIndex, item]: enumerate(items)) { ... }

[aarlt]

At first I changed that to ranges::views::enumerate but then I thought it would be better readable to introduce that addInstructionOffset lambda.

[cameel]

I think it would be more readable if you made it side-effect free and instead explicitly passed ret and instructionIndex into the function. The code is easier to understand when you see explicitly what goes in and out.

[clonker]

Most of that doc can go to the struct

[ekpyron]

I'd actually have a few suggestions for simplifying this in the hot path during assembling, but we can leave that for the second iteration of this. Also a few minor comments in that path, but none crucial.

Other than that, this won't affect regular compilation, so as far as I'm concerned it's good to merge as a first iteration of this.

[cameel]

Since you're still working on this, I decided I'd add my suggestions as comments after all rather than pushing fixups.

The biggest one is really the one about the helper. I'd really prefer to keep the Assembly changes sane, since it's already such a mess.

But overall, nothing critical. In terms of functionality the PR seems fine. Though TBH I only did a light review and I didn't really review the testing part.

[cameel]

Regarding the helper, I had something like this in mind:

Suggested change

	auto assembleInstruction = [&](auto&& _addInstruction) {
	size_t start = ret.bytecode.size();
	_addInstruction();
	size_t end = ret.bytecode.size();
	codeSectionLocation.instructionLocations.emplace_back(
	LinkerObject::InstructionLocation{
	.start = start,
	.end = end,
	.assemblyItemIndex = assemblyItemIndex
	}
	);
	};
	auto assembleInstruction = [](LinkerObject const& _linkerObject, Instruction _opcode, bytes const& _immediates, size_t assemblyItemIndex) {
	_linkerObject.bytecode += static_cast<uint8_t>(_opcode);
	_linkerObject.bytecode += _immediates;
	_linkerObject.codeSectionLocations[0].instructionLocations.push_back(
	LinkerObject::InstructionLocation{
	.start = ret.bytecode.size(),
	.end = ret.bytecode.size() 1 + _immediates.size(),
	.assemblyItemIndex = assemblyItemIndex
	}
	);
	_linkerObject.codeSectionLocations[0].end += 1 + _immediates.size();
	};

I.e. not depending on any external state, just arguments and clearly separating the bytecode from other stuff.

I'd not even make it a lambda - we'll need it in assembleEOF() later, so it should be a normal function, maybe even a class member.

[ekpyron]

I'd even consider turning it into a class doing what it does currently as side-effect :-). But I'd say we can also wait until we inevitably port it to assemblyEOF to nicen it up.

[cameel]

Well, it would be good to at least make it take bytecode, not a callback.

But yeah, it's not like it's a blocker here. It would be very nice to have, but I approved already because at this point these are not things critical to the functionality.

[ekpyron]

But yeah, for the record: this is how I'd have done this: develop...ethdebug_instructions_and_source_ranges_refactor
Which includes doing it for EOF (which is just two lines in that version).
Just to keep anything ethdebug-related that's non-critical as separate from the rest as possible (I'd move the helper classes out of the file).
The only uglyiness there is the AssignImmutable case, which unfortunately is irregular compared to all other cases, but it concentrates that uglyness there. But yeah, we can see if we do something like that or some other solution after the release, maybe there's something even better.

[cameel]

Actually, since all of this is non-critical I'm going to approve already. But it would still be nice if you managed to address these previous comments.

[ekpyron]

Also reapproving regardless of the remaining comments.