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.

Sign up for GitHub

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

Jump to bottom

eof: Implement stack height calculation #15555

Merged
merged 1 commit into from
Dec 6, 2024
Merged

eof: Implement stack height calculation #15555

merged 1 commit into from
Dec 6, 2024

Conversation

rodiazet
Copy link
Contributor

@rodiazet rodiazet commented Oct 30, 2024 •
edited
Loading

Depends on: #15550 Merged

@github-actions GitHub Actions
Copy link

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.

@cameel cameel added has dependencies The PR depends on other PRs that must be merged first EOF labels Oct 30, 2024
@rodiazet rodiazet force-pushed the stack-height branch 3 times, most recently from 2ae7088 to 692ec4d Compare October 30, 2024 21:49
cameel
cameel reviewed Oct 31, 2024
View reviewed changes
libevmasm/Assembly.cpp Outdated Show resolved Hide resolved
@rodiazet rodiazet changed the title eof: Stack height calculation eof: Implement stack height calculation Oct 31, 2024
@rodiazet rodiazet mentioned this pull request Oct 31, 2024
Merged
@shemnon shemnon mentioned this pull request Nov 27, 2024
Closed
@cameel

This comment was marked as resolved.

Sign in to view
@rodiazet

This comment was marked as resolved.

Sign in to view
@rodiazet rodiazet force-pushed the stack-height branch 7 times, most recently from 2b3c2b0 to ff30ef9 Compare December 3, 2024 14:26
cameel
cameel previously requested changes Dec 3, 2024
View reviewed changes
Copy link
Member

@cameel cameel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this is not handling ConditionalRelativeJump correctly and also one of the asserts should probably be a validation. Those should be trivial to fix though.

EDIT: My bad, posted this too quickly. No problem with ConditionalRelativeJump after all.

Other than that, a few suggestions for sanity checks and tweaks to make the algorithm easier to understand.

libevmasm/Assembly.cpp Outdated Show resolved Hide resolved
libevmasm/Assembly.cpp Outdated Show resolved Hide resolved
libevmasm/Assembly.cpp Outdated Show resolved Hide resolved
libevmasm/Assembly.cpp Outdated Show resolved Hide resolved
libevmasm/Assembly.cpp Outdated Show resolved Hide resolved
libevmasm/Assembly.cpp Show resolved Hide resolved
@cameel cameel dismissed their stale review December 3, 2024 19:04

I was wrong about RJUMPI and all the other suggestions are non-critical so removing the change request

@cameel cameel added 🟡 PR review label and removed has dependencies The PR depends on other PRs that must be merged first labels Dec 3, 2024
@rodiazet rodiazet force-pushed the stack-height branch 2 times, most recently from a4b0b28 to 6187d93 Compare December 4, 2024 10:53
cameel
cameel previously approved these changes Dec 5, 2024
View reviewed changes
@cameel cameel merged commit 9ec58fa into ethereum:develop Dec 6, 2024
73 checks passed
@rodiazet rodiazet deleted the stack-height branch December 9, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel approved these changes

Assignees
No one assigned
Labels
EOF external contribution ⭐ 🟡 PR review label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rodiazet @cameel