Fix verbatim control flow side effects

[rodiazet]

• Reasoning: Add ControlFlowSideEffects parameter to createFunction in EVMDialect #15535 (comment)

[github-actions]

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

[matheusaaguiar]

Just a nitpick. And also, @cameel mentioned a changelog entry, but I am not sure about the actual bug he referred to.

[rodiazet]

Just a nitpick. And also, @cameel mentioned a changelog entry, but I am not sure about the actual bug he referred to.

Also not sure. Typo fix pushed.

[cameel]

#15535 is merged so now we can pass these via the parameter we added there.

Might also be a good idea to define a helper called worst(), similarly to the one we already have for SideEffects.

[cameel]

We discussed this bug on Wednesday and we do think that this may cause actual problems in the optimizer. E.g. UnusedStoreEliminator might mistakenly remove a store operation assuming that the code will revert, while a verbatim block actually makes it terminate successfully.

We need a repro test case to confirm it. I think that something like store_before_conditionally_terminating_function_call_revert.yul should do it if you replace the conditionallyStop() call with a verbatim block that uses RETURN.

Still, we decided that we'll not treat it as an important bug (i.e. no blog post and no bug list entry, just a normal changelog entry), because doing something like this actually violates the prescribed usage rules for verbatim:

• Control-flow should not jump into or out of verbatim blocks, but it can jump within the same verbatim block.

Instead, we should add a warning in verbatim docs saying that until 0.8.28 verbatim had this flaw. We could also clarify the point above to specifically say that reverting and returning within verbatim are not allowed.

[rodiazet]

We discussed this bug on Wednesday and we do think that this may cause actual problems in the optimizer. E.g. UnusedStoreEliminator might mistakenly remove a store operation assuming that the code will revert, while a verbatim block actually makes it terminate successfully.

We need a repro test case to confirm it. I think that something like store_before_conditionally_terminating_function_call_revert.yul should do it if you replace the conditionallyStop() call with a verbatim block that uses RETURN.

Still, we decided that we'll not treat it as an important bug (i.e. no blog post and no bug list entry, just a normal changelog entry), because doing something like this actually violates the prescribed usage rules for verbatim:

• Control-flow should not jump into or out of verbatim blocks, but it can jump within the same verbatim block.

Instead, we should add a warning in verbatim docs saying that until 0.8.28 verbatim had this flaw. We could also clarify the point above to specifically say that reverting and returning within verbatim are not allowed.

I added such a test but even without this fix it passes. After debugging of UnusedStoreEliminator I guessing that it works because in UnusedStoreEliminator::applyOperation the store is considered as read from by the verbatim after it.

{
    let x := 0
    let y := 1
    sstore(x, y)
    verbatim_0i_0o(hex"600080F3")
    revert(0,0)
}

[cameel]

Nice. I looked at the implementation and I think you're right. So we're safe here. In that case we do not need the changelog entry or the warning.

I'd still add the test case though, even if it does not fail. Best to have this corner case covered.

And we should till clarify the docs to say that reverting and returning within verbatim are not allowed.

[rodiazet]

Nice. I looked at the implementation and I think you're right. So we're safe here. In that case we do not need the changelog entry or the warning.

I'd still add the test case though, even if it does not fail. Best to have this corner case covered.

And we should till clarify the docs to say that reverting and returning within verbatim are not allowed.

Test added. Doc edited.

[cameel]

Looks good, but I have two small suggestions.