Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Improvements to assembly sections in BytesLib. #81

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

scnale
Copy link
Collaborator

@scnale scnale commented Feb 6, 2023

This improves codegen in the compiler and reduces the chance of untraceable internal codegen errors when dealing with stack variables that can't be referenced with an instruction.

I've been running into these codegen errors quite a bit while tweaking forge tests but this seems to eliminate them.

In other news, this shaves off 1KB from the contract bytecode so that's a good thing.

The bad news is that marking these sections as memory safe is unsafe to do if they don't actually respect Solidity's memory model. I was actually thinking of creating a reduced version of the BytesLib since we don't use all of it but I prefer to do that in another pull request.

@scnale
Copy link
Collaborator Author

scnale commented Feb 10, 2023

Note that this pull request now includes changes from #82 and #83 too.

Edit: these were merged separately.

@scnale scnale force-pushed the toolchain/mark-memory-safe-asm branch from 738584a to 6b0e267 Compare February 14, 2023 19:06
This improves codegen in the compiler and reduces the chance of
untraceable internal codegen errors when dealing with stack variables
that can't be referenced with an instruction.

Also, it removes unused functions in `BytesLib` to reduce code surface.
@scnale scnale force-pushed the toolchain/mark-memory-safe-asm branch from 6b0e267 to 145a1ce Compare February 14, 2023 19:51
@nonergodic
Copy link

lgtm - all my issues / complaints with the code stem from the original BytesLib (as discussed on Slack, putting it here too for completeness sake):

require(_length + 31 >= _length, "slice_overflow");
this makes no sense since Solidity 0.8 because now all math is checked and so in case of an overflow this will fail with a Panic(0x11) before the require can trigger
similarly, I don't know how smart the optimizer is at this point, but there's a lot of wasted gas in various places:

tempAddress := div(mload(add(add(_bytes, 0x20), _start)), 0x1000000000000000000000000)
this should instead be
tempAddress := shr(12, mload(add(add(_bytes, 0x20), _start)))
which is both cheaper to deploy (saves 12 bytes) and cheaper to execute (shr is 3 gas, div is 5)

also, again not sure how smart the optimizer is at this point, but
require(_bytes.length >= _start + 1 , "toUint8_outOfBounds");
should be implemented using
require(_bytes.length > _start, "toUint8_outOfBounds");
because EVM has only < and == opcodes but no <=, so this is again unnecessarily more gas inefficient...

both the if and the else branch in slice() start by loading the next free memory location and can hence be done once before the if (i.e. switch) instead of in each arm redundantly

inconsistent specification of values in slice() - 31 is specified in dec, everything else in hex

@scnale
Copy link
Collaborator Author

scnale commented Feb 14, 2023

I think I'll address all of these here and make other improvements in a separate PR, if any.

@nonergodic
Copy link

I think I'll address all of these here and make other improvements in a separate PR, if any.

you can also combine the range checking and the offset calculation (right now the add is effectively done twice (once for the check, once for the offset) instead of just once)

@scnale
Copy link
Collaborator Author

scnale commented Feb 15, 2023

I think this is ready to be merged. @nonergodic could you take a peek at the changes?

@nonergodic
Copy link

nonergodic commented Feb 16, 2023

Thanks for catching my error in shr - it should indeed be 96 and not 12!

still looks lgtm, i.e. the nitpicky comments below are very much optional:

Replacing >= with > could be done in all other toUintX functions too. (alternatively, as the very first thing, one could do the addition with the offset and store the result before checking and then eliminate the redundant add in the inline assembly block)

I also think that except for 0x40 (which acts as an address of sorts), I'd in fact actually replace all hex values with decimal values (or better yet introduce constants if they are allowed in inline assembly by now) because specifying a number of bits/bytes in hex seems unnecessarily awkward. Not sure why the author(s) of BytesLib chose to originally default to hex in assembly in the first place (and then break it with the 31 either...).

@scnale scnale changed the title Adds memory-safe tag to assembly sections in BytesLib. Improvements to assembly sections in BytesLib. Feb 22, 2023
@scnale scnale marked this pull request as draft February 22, 2023 22:44
@scnale
Copy link
Collaborator Author

scnale commented Feb 22, 2023

I'm marking this as draft since I'll be adding an improvement here and don't want to merge it separately.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants