Fix crash when using @use-src while compiling yul -> ewasm

[Marenz]

fixes #12065

I am not really sure this is the correct way to fix this, but at least all tests run..

[leonardoalt]

Tests are failing.

[axic]

Hmm, why do we have Parser with a default value (without the explicit arg) if it causes crashes?

[chriseth]

It only causes a crash because we use it on a source file that is not known to the later asm printer.
The default value means "use the source locations as they are reported by the scanner". What we do here is combining regular source locations with the ones reported from the parser here, which are pointing to an on-the-fly generated source file.

[cameel]

It's a bit more complicated. After #11998 we can discern between the native locations and those grabbed from annotations. In normal yul compilation it does not cause a crash even though the the original source is not available. So why does it crash here?

I think that the problem is actually that the nativeLocations in the generated AST are wrong. EVMToEwasmTranslator copies the whole debug info, when it translates the AST. This means that the new AST gets the native location pointing at the original assembly input file rather than the translated source it's supposed to point at.

The problem is that the translated assembly does not actually exist until we generate it from the AST. At the point where the debug info is being copied, we only have the (incomplete) AST and not source we could point at. I think the only thing we can do is to use empty nativeLocation in the translated AST. Not having native location means that we can't use it in errors reported from the analysis of that AST. But I don't think we even analyze it - it's generated by the compiler after all so it's not supposed to have errors - those should have been caught during parsing of the original assembly input. It's very likely that the only code that actually tries to access the native location is the AsmPrinter (which is where this crash happens).

The location override is another way to get rid of native locations from the AST but a very heavy-handed one - it also discards the origin location. Currently it does not matter much, because I don't think we make use of it when generating ewasm. It will matter once we decide we want to include debug info in ewasm though.

Overall, I think this is an acceptable workaround as long as work on ewasm is stalled. I'd just add a comment explaining that this is just a workaround and that it effectively strips both originLocation and nativeLocation from the AST even though only nativeLocation really has to be discarded.

[ekpyron]

What a monster :-).

Maybe use

/// @use-src 0:"test.sol"
object "C" {
    code { sstore(0,0) }
}

as test case instead and run it with
--strict-assembly --yul-dialect evm --machine ewasm --optimize

:-)?

That also triggered the bug and is fixed with the PR, but it doesn't require 5K of test expectation ;-).

[cameel]

We have #12074 now so you can use output selection to make this smaller. --ir-optimized or --bin should be enough in this case.

[Marenz]

i'll test with the new command parameters and see if they also trigger the problem

[cameel]

I think they should. For the compiler we actually skip generating the stuff for which output is not required but assembler is more straightforward. Output selection just makes it not print some stuff but it still does all the work. Maybe that's actually something we should change though.

[Marenz]

Both parameters (together and on their own) make it not crash.

[Marenz]

Adding sstore(0,0) makes it crash again though.

[cameel]

Then it sounds like it might be because of the optimizer?

Option selection should not disable anything unless some actual work is being done inside functions like print() or toHex(). https://github.com/ethereum/solidity/blob/v0.8.10/solc/CommandLineInterface.cpp#L1036-L1091

[Marenz]

I think I misspelled something, I am currently testing again

[Marenz]

crash:
	with sstore(0,0):
		--strict-assembly --yul-dialect evm --machine ewasm
		--strict-assembly --yul-dialect evm --machine ewasm --optimize
	without sstore(0,0):
		--strict-assembly --yul-dialect evm --machine ewasm
No crash:
	with and without sstore(0,0):
		--strict-assembly --yul-dialect evm --machine ewasm --ir-optimized
		--strict-assembly --yul-dialect evm --machine ewasm --ir-optimized --bin
	with no sstore(0,0):
		--strict-assembly --yul-dialect evm --machine ewasm --optimize

output wise I guess it's best to go with

	with sstore(0,0):

		--strict-assembly --yul-dialect evm --machine ewasm --optimize

[cameel]

--strict-assembly --yul-dialect evm --machine ewasm --ewasm-ir also reproduces the crash for me on 0.8.10 (without sstore if --optimize is not used).

I guess the thing that triggers the crash is called from within AssemblyStack::print() after all.

[cameel]

After looking at this closely, this is not a real fix but just a workaround. It strips origin locations from translated AST so we can't use them in Ewasm. But since Ewasm does not use debug info yet and work on it is stalled, I think it's acceptable. I'd just add some comments explaining the situation. See my comment below for details.

Other than that, just two minor tweaks and it's good to go.

[cameel]

It's a bit more complicated. After #11998 we can discern between the native locations and those grabbed from annotations. In normal yul compilation it does not cause a crash even though the the original source is not available. So why does it crash here?

I think that the problem is actually that the nativeLocations in the generated AST are wrong. EVMToEwasmTranslator copies the whole debug info, when it translates the AST. This means that the new AST gets the native location pointing at the original assembly input file rather than the translated source it's supposed to point at.

The problem is that the translated assembly does not actually exist until we generate it from the AST. At the point where the debug info is being copied, we only have the (incomplete) AST and not source we could point at. I think the only thing we can do is to use empty nativeLocation in the translated AST. Not having native location means that we can't use it in errors reported from the analysis of that AST. But I don't think we even analyze it - it's generated by the compiler after all so it's not supposed to have errors - those should have been caught during parsing of the original assembly input. It's very likely that the only code that actually tries to access the native location is the AsmPrinter (which is where this crash happens).

The location override is another way to get rid of native locations from the AST but a very heavy-handed one - it also discards the origin location. Currently it does not matter much, because I don't think we make use of it when generating ewasm. It will matter once we decide we want to include debug info in ewasm though.

Overall, I think this is an acceptable workaround as long as work on ewasm is stalled. I'd just add a comment explaining that this is just a workaround and that it effectively strips both originLocation and nativeLocation from the AST even though only nativeLocation really has to be discarded.