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

⚡️ Memory safe assembly #333

Merged
merged 5 commits into from
Oct 27, 2022
Merged

Conversation

axic
Copy link
Contributor

@axic axic commented Oct 14, 2022

Description

Mark assembly blocks memory-safe, so that they can be used with optimizer stages (for example the stack-to-memory elevation). See https://docs.soliditylang.org/en/v0.8.17/assembly.html?highlight=memory-safe#memory-safety for details.

I did spend quite some time manually reviewing each case in this codebase, but I don't feel confident merging this without at least 2 proper reviews (cc @hrkrshnn).

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Ran forge snapshot?
  • Ran npm run lint?
  • Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

@@ -79,7 +79,7 @@ library SSTORE2 {
uint256 start,
uint256 size
) private view returns (bytes memory data) {
assembly {
assembly ("memory-safe") {
Copy link

@hrkrshnn hrkrshnn Oct 14, 2022

Choose a reason for hiding this comment

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

There can be dirty values at the end of the bytes btw.

Need to clean the last memory chunk here.

Take size to be 31 and have all the memory to be dirty. The last bit of the data would be dirty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct, but shouldn't affect memory-safe?

Choose a reason for hiding this comment

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

Yeah. This is independently of memory safe. I'll separate it into a new issue.

@axic axic changed the title Memory safe Memory safe assembly Oct 17, 2022
@axic axic marked this pull request as ready for review October 23, 2022 21:22
@axic
Copy link
Contributor Author

axic commented Oct 23, 2022

@transmissions11 ready for review, will squash once we get closer to merging.

@transmissions11
Copy link
Owner

isn't this (memory-safe) modifier for the assembly block only available in more recent versions of 0.8? since we want to maintain compatibility with all versions we should probably use the comment syntax, no?

@axic
Copy link
Contributor Author

axic commented Oct 24, 2022

isn't this (memory-safe) modifier for the assembly block only available in more recent versions of 0.8? since we want to maintain compatibility with all versions we should probably use the comment syntax, no?

Good point, can change to that. Hmm, actually wonder how compatible that is given it starts with @solidity as a new natspec tag 😬

Btw, the assembly <string literal> {} syntax was valid since assembly was introduced, but it only accepted evmasm as the sole value plus the parenthesis syntax is new.

@axic
Copy link
Contributor Author

axic commented Oct 24, 2022

@transmissions11 what is the earliest 0.8.x version do you want to support? There should be a CI run for that version, just to be sure.

@transmissions11
Copy link
Owner

@transmissions11 what is the earliest 0.8.x version do you want to support? There should be a CI run for that version, just to be sure.

This is a good point, ideally 0.8.0 itself. I'll open an issue.

@transmissions11 transmissions11 changed the title Memory safe assembly ⚡️ Memory safe assembly Oct 27, 2022
@transmissions11 transmissions11 merged commit f2833c7 into transmissions11:main Oct 27, 2022
@axic axic deleted the memory-safe branch October 28, 2022 13:21
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.

3 participants