Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix[codegen]: fix make_setter overlap in dynarray_append #4059

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented May 28, 2024

What I did

fix #4056, which is another make_setter overlap bug

variant of #4037 and #3410

additionally adds an internal assertion to prevent future variants

How I did it

How to verify it

Commit message

`make_setter` can potentially run into overlap when called from
`dynarray_append`; this commit copies to a temporary intermediate buffer
(as is done in `AnnAssign`) before copying to the destination if the
condition is detected.

this is a variant of the bugs fixed in ad9c10b0b98e2d and 1c8349e867b2b.

this commit also adds a util function to detect overlap, and adds an
assertion directly in `make_setter` so that future variants panic
instead of generating bad code.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

make_setter can potentially run into overlap when called from
dynarray_append; this commit copies to a temporary intermediate buffer
before copying to the destination if the condition is detected.

this commit also adds a util function to detect overlap, and adds an
assertion directly in `make_setter` so that future variants panic
instead of generating bad code.
@cyberthirst
Copy link
Collaborator

cyberthirst commented May 29, 2024

currently we don't handle ann_assign well

this will panic

interface Bar:
    def bar() -> Bytes[2]: payable


@external
def foo() -> Bytes[2]:
    x: Bytes[2] = extcall Bar(self).bar()
    return x

@cyberthirst
Copy link
Collaborator

also this due to the internal buffer variable on lhs

BLUEPRINT: immutable(address)

@deploy
def __init__(blueprint_address: address):
    BLUEPRINT = blueprint_address

@external
def test(code_ofst: uint256) -> address:
    return create_from_blueprint(BLUEPRINT, code_offset=code_ofst)

Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 19.04762% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 55.21%. Comparing base (dcec257) to head (74d1b85).

Current head 74d1b85 differs from pull request most recent head 2b93d79

Please upload reports for the commit 2b93d79 to get more accurate results.

Files Patch % Lines
vyper/codegen/core.py 33.33% 4 Missing and 4 partials ⚠️
vyper/codegen/expr.py 0.00% 8 Missing ⚠️
vyper/codegen/stmt.py 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4059       +/-   ##
===========================================
- Coverage   91.23%   55.21%   -36.03%     
===========================================
  Files         108      107        -1     
  Lines       15499    15466       -33     
  Branches     3404     3401        -3     
===========================================
- Hits        14141     8540     -5601     
- Misses        927     6304     +5377     
- Partials      431      622      +191     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@charles-cooper charles-cooper added this to the v0.4.0 milestone May 29, 2024
@charles-cooper charles-cooper changed the title fix[codegen]: fix src/dst overlap in dynarray_append fix[codegen]: fix make_setter overlap in dynarray_append May 30, 2024
@charles-cooper charles-cooper merged commit 9745d44 into vyperlang:master May 30, 2024
160 checks passed
@charles-cooper charles-cooper deleted the fix/dynarray-append-setter branch May 30, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make_setter is incorrect for complex types when RHS references LHS with .append()
2 participants