eof: Fix stack height calculation for non-returning function.

[rodiazet]

OptimizedEVMCodeTransform requires Assembly stack height for non-returning function call to be equal to rets-args. For EVM Assembly for EOF it's calculated based on CodeSection inputs and outputs. It means that we need to store number of rets for a code section equal to number of return values based in function declaration. Additional flag canContinue is needed to properly generate code section type in EOF container and to decide if we should use JumpF or CallF when creating AssemblyItem for the function call.

Depends on #15635. Merged.

[cameel]

Approving since this seems correct and none of the comments below are critical.

Still, reading this made me realize that we probably have another bug in stack height calculation, this time in calculateMaxStackHeight() because it does not take into account terminating instructions.

We could also use some tweaks to asserts and naming.

[cameel]

This made me realize something: in Assembly::calculateMaxStackHeight() we're applying the stack height change even for terminating instructions, especially JUMPF, but the spec say:

terminating instructions do not need to update stack heights.

This must be making the calculation wrong in some cases and I'm also not confident that our semantic tests would catch that, so it would be good to cover that with some extra tests. For example a non-returning JUMPF with tons of inputs should affect the max height in a way that cause a mismatch with EVM's validation.

Fixing that might be a separate PR though. It's related to this one since it's also stack height calculation but I guess we could consider it a separate bug.

[cameel]

Just to make sure I understand what we're fixing here. inline_assembly_in_modifiers.sol triggers this assert:

solidity/libyul/backends/evm/OptimizedEVMCodeTransform.cpp

Line 135 in 87b76ad

yulAssert(m_assembly.stackHeight() == static_cast<int>(m_stack.size()), "");

The assert failing means that Assembly::stackHeight() (which is calculated by adding deposit(), i.e. returnValue() - arguments(), for each appended assembly item) does not match the stack height tracked internally by the Yul->EVM transform.

And the mismatch comes from the fact that for non-returning calls the transform uses the number of returns declared by the function while the assembly uses 0. The fix here is to change the calculation in assembly to use the number from function definition as well.

This means that in AssemblyItem::FunctionSignature the number of returns will never be 0x80, but the actual number of returns. Since AssemblyItem::returnValues() was the only place that checked for that special value, canContinue() is now unused and can be removed.

On the other hand in Assembly::CodeSection we never need to know the number of returns of non-returning functions so we could have left it as is. But changing it to match FunctionSignature and replacing the special value with a canContinue flag makes things simpler (e.g. we don't need to special-case it when comparing them).

Am I missing anything?

[rodiazet]

You get it right. Potentially we could leave CodeSection definition unchanged because the flag canContinue (nonReturning) is needed only to decide on CallF or JumpF when adding function call but I would leave it as it is now. It's clearer IMO.

[cameel]

Just one more assert to get rid of. Then we can merge.