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]: panic on potential eval order issue for some builtins #4157

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jun 18, 2024

What I did

alternative to #4156; panic instead of changing codegen

How I did it

How to verify it

Commit message

`extract32()` and `slice()` have an evaluation order issue when
the arguments touch the same data. specifically, the length and data
evaluation are interleaved with the index/start/length evaluations. in
unusual situations (such as those in the included test cases), this
can result in "invalid" reads where the data and length reads appear
out of order. this commit conservatively blocks compilation if the
preconditions for the interleaved evaluation are detected.

---------

Co-authored-by: trocher <trooocher@proton.me>
Co-authored-by: cyberthirst <cyberthirst.eth@gmail.com>

Description for the changelog

Cute Animal Picture

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

charles-cooper and others added 6 commits June 18, 2024 10:24
---------

Co-authored-by: trocher <trooocher@proton.me>
---------

Co-authored-by: cyberthirst <cyberthirst.eth@gmail.com>
---------

Co-authored-by: trocher <trooocher@proton.me>
they are currently blocked with compiler panic; codegen fix should come
later
@charles-cooper charles-cooper changed the title fix[codegen]: panic on eval order for some builtins fix[codegen]: panic on potential eval order issue for some builtins Jun 18, 2024
@charles-cooper charles-cooper marked this pull request as ready for review June 18, 2024 17:15
@cyberthirst
Copy link
Collaborator

should we add a hint on how to avoid this issue? or link to the issue tracker?

self.var.pop()
return 3

s:bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray variable

self.var.pop()
return 3

s:bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray variable

self.var.pop()
return 32

s:bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray variable

self.var.pop()
return 3

s:bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray variable

@charles-cooper
Copy link
Member Author

should we add a hint on how to avoid this issue? or link to the issue tracker?

no, i think it's pretty unlikely anybody actually runs into this. if they do, they should be encouraged to create an issue (as prompted) so we can get some real world usage data

@charles-cooper charles-cooper enabled auto-merge (squash) June 18, 2024 19:33
@charles-cooper charles-cooper enabled auto-merge (squash) June 18, 2024 19:33
@charles-cooper charles-cooper changed the title fix[codegen]: panic on potential eval order issue for some builtins fix[codegen]: panic on potential eval order issue Jun 18, 2024
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (d92cd34) to head (a90f50d).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4157      +/-   ##
==========================================
- Coverage   91.32%   90.08%   -1.24%     
==========================================
  Files         109      109              
  Lines       15569    15573       +4     
  Branches     3418     3420       +2     
==========================================
- Hits        14218    14029     -189     
- Misses        923     1070     +147     
- Partials      428      474      +46     

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

@charles-cooper charles-cooper enabled auto-merge (squash) June 18, 2024 19:41
@charles-cooper charles-cooper changed the title fix[codegen]: panic on potential eval order issue fix[codegen]: panic on potential eval order issue for builtins Jun 18, 2024
@charles-cooper charles-cooper changed the title fix[codegen]: panic on potential eval order issue for builtins fix[codegen]: panic on eval order issue for some builtins Jun 18, 2024
@charles-cooper charles-cooper changed the title fix[codegen]: panic on eval order issue for some builtins fix[codegen]: panic on potential eval order issue for some builtins Jun 18, 2024
@charles-cooper charles-cooper enabled auto-merge (squash) June 18, 2024 19:42
@charles-cooper charles-cooper merged commit 3d9c537 into vyperlang:master Jun 18, 2024
162 of 163 checks passed
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.

2 participants