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

Signextend and friends #575

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Oct 19, 2021

What was wrong?

There were a few cases where we needed to call adjust_numeric_size to "clean" a 256 bit stack value that is holding a numeric value <= 128 bits (See #524)

How was it fixed?

Changed move_expression to call adjust_numeric_size when loading integer values from memory or storage.

@cburgdorf cburgdorf force-pushed the christoph/fix/loading_numerics_from_storage_or_mem branch from 42d36bc to 914a0c9 Compare October 19, 2021 16:11
@cburgdorf cburgdorf marked this pull request as ready for review October 19, 2021 16:16
@cburgdorf
Copy link
Collaborator Author

@g-r-a-n-t I can't really tell if that is all that is needed in this regard. I know that Solidity performs way more of these cleanup operations at a bunch of places (e.g. inside checked_add_*, checked_sub_* etc) but I can't convince myself that they are actually needed there and I don't just want to put in checks (that cost gas) without having tests that prove we need them.

@g-r-a-n-t
Copy link
Member

I know that Solidity performs way more of these cleanup operations

I've noticed that too. It could be precautionary, but I'm not sure.

The PR looks good, but I'll spend some more time reviewing and approve it when I feel comfortable with it.

@cburgdorf
Copy link
Collaborator Author

In this issue it is noted that they could perform less checks and it also sounds that some of these checks are just in place to avoid users messing things up with inline assembly which we don't have.

but I'll spend some more time reviewing

Thanks, I'll sleep better if you check this over thoroughly. 😅

@cburgdorf
Copy link
Collaborator Author

Btw, here's a blog post on a recently found Solidity bug related to these cleanups. Haven't read it yet but felt like leaving it here for later: https://blog.soliditylang.org/2021/09/29/signed-immutables-bug/

Copy link
Member

@g-r-a-n-t g-r-a-n-t left a comment

Choose a reason for hiding this comment

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

LGTM

@cburgdorf cburgdorf merged commit 4f574f4 into ethereum:master Oct 22, 2021
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