search

vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.81k stars 788 forks source link

Double Eval in experimental codegen without optimization #4072

Closed ritzdorf closed 2 weeks ago

ritzdorf commented 1 month ago

Version Information

Issue description

In version 0.4.0rc6 it seems that https://github.com/vyperlang/vyper/pull/4030 fixed the issue https://github.com/vyperlang/vyper/issues/4071 for all tested compilation settings EXCEPT when compiling with experimental_codegen and no optimization. For that compilation setting incorrect bytecode with double evaluation is still being generated.

PoC

m: HashMap[uint256, String[33]]

@external
def foo() -> uint256:
    x: DynArray[uint256, 32] = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
    self.m[x.pop()] = "Hello world"
    return len(x)

And compile with

vyper --experimental-codegen -O none example.vy

Returns 30, but should return 31.

harkal commented 3 weeks ago

It appears that the latest master (commit 21f7172) fails to compile the above example. I have created a test case for it in #4130 to investigate for venom, but it seems to be a general issue.

trocher commented 3 weeks ago

It appears that the latest master (commit 21f7172) fails to compile the above example. I have created a test case for it in #4130 to investigate for venom, but it seems to be a general issue.

I think master failing to compile is expected given that #3514/#4071 have not been fixed yet, only a fence was added (#4030) to prevent incorrect bytecode generation by having the compiler fail, but the underlying issue is not fixed

harkal commented 3 weeks ago

so #4030 is not actually fixing the issue, instead it asserts to abort from producing erroneous bytecode?

trocher commented 3 weeks ago

Exact, #4030, together with #3835 it extend the usage of unique_symbol to more expressions that are known to have side effects. They make the compiler fail if the tagged node gets evaluated more than once by using unique_symbol:

Concerned (IR) nodes are:

This means that for now, the compiler relies on that list to be exhaustive not to perform any double evaluation of the dst of an assignment that would lead to visible side effects

charles-cooper commented 3 weeks ago

yea we can probably demote this to "type 0" bug (panic instead of producing bad code). but we should still investigate since it indicates some kind of deeper issue

charles-cooper commented 3 weeks ago

ah never mind, i see what's happening -- -Onone --experimental-codegen bypasses the unique symbols check