stack cleanup uses inconsistent assumptions for different nodes

[cyberthirst]

Version Information

• vyper Version (output of vyper --version): 14a4ca6

What's your issue about?

Per @trocher:

There is a fundamental inconsistency in how stack cleanup is handled between different control flow mechanisms in Vyper's IR compilation. The break statement assumes that stack items can accumulate after evaluation of other IR statements, while cleanup_repeat and exit_to assume no such accumulation occurs, and only repeat can leave items on the stack.

The break statement explicitly handles the case where height - break_height > 0, implicitly meaning that it expects there to be items on the stack that need to be cleaned up:

elif code.value == "break":
    if not break_dest:
        raise CompilerPanic("Invalid break")
    dest, continue_dest, break_height = break_dest

    n_local_vars = height - break_height
    # clean up any stack items declared in the loop body
    cleanup_local_vars = ["POP"] * n_local_vars
    return cleanup_local_vars + [dest, "JUMP"]

In contrast, the cleanup_repeat and exit_to instructions do not account for any stack items that may have been left on the stack by previous operations. They only handle the case where the stack contains items from the repeat loop itself:

elif code.value == "cleanup_repeat":
    if not break_dest:
        raise CompilerPanic("Invalid break")
    # clean up local vars and internal loop vars
    _, _, break_height = break_dest
    # except don’t pop label params
    if "return buffer" in withargs:
        break_height -= 1
    if "return pc" in withargs:
        break_height -= 1
    return ["POP"] * break_height

---

In the current implementation, this inconsistency is not problematic because statements cannot leave items on the stack after their evaluation. The stack is always cleaned up before the next statement, or a child statement is executed (except for repeat), ensuring that the stack state remains consistent.

If in the future, stack items are allowed to accumulate after the evaluation of other IR statements, this inconsistency could lead to several issues:

1. Stack Corruption: cleanup_repeat or exit_to would leave stack items unpopped, corrupting the stack state for outer functions.
2. Runtime Failures: Stack underflow/overflow in complex nested scenarios.
3. Inconsistent Behavior: Different control flow paths would handle stack cleanup differently.