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

pop() for storage arrays #3743

Merged
merged 5 commits into from May 30, 2018

Conversation

Projects
4 participants
@bit-shift
Contributor

bit-shift commented Mar 15, 2018

Closes #2780

@bit-shift bit-shift requested a review from chriseth Mar 15, 2018

@bit-shift

This comment has been minimized.

Contributor

bit-shift commented Mar 15, 2018

Size is currently not decremented correctly and not implemented for bytearrays at all. I think the length is not stored, so I will find some examples on how to do this.

@@ -1542,6 +1542,13 @@ MemberList::MemberMap ArrayType::nativeMembers(ContractDefinition const*) const
strings{string()},
isByteArray() ? FunctionType::Kind::ByteArrayPush : FunctionType::Kind::ArrayPush
)});
members.push_back({"pop", make_shared<FunctionType>(

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

Beware: This is not inside the body of the if statement.

This comment has been minimized.

@bit-shift

bit-shift Mar 15, 2018

Contributor

Damn. Good catch! That's the reason why I prefer to have braces for if statements if the enclosed block spans over multiple (>2) lines. Enforced defect protection in a some way.

@@ -67,6 +67,11 @@ class ArrayUtils
/// Stack pre: reference (excludes byte offset) new_length
/// Stack post:
void resizeDynamicArray(ArrayType const& _type) const;
/// Decrements the size of a dynamic array by one.
/// Does not touch the new data element. In case of a byte array, this might move the data.

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

Please change to: clears the removed data element.

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

And also: Assumes that the length is nonzero.

solAssert(function.parameterTypes().size() == 0, "");
ArrayType const& arrayType = dynamic_cast<ArrayType const&>(
*dynamic_cast<MemberAccess const&>(_functionCall.expression()).expression().annotation().type);

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

) should be on a line of its own.

case FunctionType::Kind::ArrayPop:
{
_functionCall.expression().accept(*this);
solAssert(function.parameterTypes().size() == 0, "");

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

Could use .empty()

solAssert(arrayType.dataStoredIn(DataLocation::Storage), "");
// stack: ArrayReference

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

I'm wondering whether we should also just move these into ArrayUtils::decrementDynamicArraySize (and then call it popStorageArrayElement or something).

This comment has been minimized.

@bit-shift

bit-shift Mar 15, 2018

Contributor

That's a great idea. We should try to gradually pull code out of this function.

else if (member == "pop")
{
solAssert(
type.isDynamicallySized() && type.location() == DataLocation::Storage,

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

Should probably also check whether type.category() is Array.

This comment has been minimized.

@bit-shift

bit-shift Mar 15, 2018

Contributor

I think it should be also safe without, because we're switching on _memberAccess.expression().annotation().type->category() to reach this branch. But it doesn't hurt to have it checked since one could mess things up in between (function is rather longish :)).

data.push(7);
x = data.push(3);
data.pop();
y = data.push(2);

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

Please also check (return) data.length

x = data.push(3);
data.pop();
data.pop();
y = data.push(2);

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

Please also check (return) data.length

compileAndRun(sourceCode);
ABI_CHECK(callContractFunction("test()"), encodeArgs());
}

This comment has been minimized.

@chriseth

chriseth Mar 15, 2018

Contributor

Please add tests that

  • push and pop across the 32 byte boundary for byte arrays
  • check that storage is properly cleaned (there are other tests here that do that, search for storageEmpty), but for that, you have to reduce the length to zero.

This comment has been minimized.

@bit-shift

bit-shift Mar 19, 2018

Contributor

Done.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Mar 15, 2018

I don't see a problem with the implementation for non-byte arrays, length is decremented and stored.

@gburtini

This comment has been minimized.

gburtini commented Mar 18, 2018

Is it reasonable to add shift and unshift here too?

@chriseth

This comment has been minimized.

Contributor

chriseth commented Mar 19, 2018

@gburtini no, because they require too much copying. You can add that as an internal library function, if you want.

switch and(slot_value, 1)
case 0 {
// short byte array
let length := and(div(slot_value, 2), 0x7f)

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

I spent some thoughts on this and came to the conclusion that we should ignore any higher bits in the length field, but clean up the mess once we write. This means we should use 0x3f here instead of 0x7f.

// Zero-out the suffix of the byte array by masking it
// Do not zero-out the least significant byte
// ((1<<(8 * (31 - length))) - 1) << 8
let mask := mul(0x100, sub(exp(0x100, sub(31, length)), 1))

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

The mask should cover the higher bits of the length and clear them.

let offset := 31
mstore(0, ref)
slot := add(keccak256(0, 0x20), slot)

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

No need to add 0

slot := add(keccak256(0, 0x20), slot)
let data := sload(slot)
sstore(slot, 0)
data := and(data, not(0xFF))

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

The rest of the code uses lowercase hex digits.

let slot := div(sub(length, 1), 32)
let offset := and(sub(length, 1), 0x1F)
mstore(0, ref)

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

This part could be moved to line 806

let data := sload(slot)
// Zero-out the suffix of the byte array by masking it
// Do not zero-out the least significant byte

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

I'm a little confused here. What does this comment refer to?

slot_value := sub(slot_value, 2)
sstore(ref, slot_value)
}
}

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

Indentation is off here.

// Zero-out the suffix of the byte array by masking it
// Do not zero-out the least significant byte
// ((1<<(8 * (31 - offset))) - 1)
let mask := sub(exp(0x100, sub(31, offset)), 1)

This comment has been minimized.

@chriseth

chriseth Mar 19, 2018

Contributor

I'm not sure anymore whether 31 is correct here. I think it should be 32.

This comment has been minimized.

@bit-shift

bit-shift Mar 19, 2018

Contributor

32 is correct :) This was also wrong for "short" byte arrays and led to the issue I was looking into earlier.

@bit-shift

This comment has been minimized.

Contributor

bit-shift commented Mar 19, 2018

@chriseth I've changed the way how the higher bits of the length are being masked. Not sure though if this is enough already or if I'm missing something.

@bit-shift bit-shift changed the title from [WIP] pop() for storage arrays to pop() for storage arrays Mar 26, 2018

@axic

This comment has been minimized.

Member

axic commented Mar 26, 2018

@bit-shift are there any commits which can be squashed (haven't looked yet)?

@bit-shift

This comment has been minimized.

Contributor

bit-shift commented Mar 26, 2018

@axic I've sorted and squashed the commits by value-type / non value-type implementation. The last commit also improves the overall test structure.

"Tried to use .push() on a non-dynamically sized array"
);
}
else if (member == "pop")

This comment has been minimized.

@axic

axic Mar 26, 2018

Member

I think these two could be merged into a single if statement?

ByteArrayPush, ///< .push() to a dynamically sized byte array in storage
ByteArrayPop, ///< .pop() from a dynamically sized byte array in storage

This comment has been minimized.

@chriseth

chriseth Mar 30, 2018

Contributor

Actually, since we check the type of the array anyway, we do not have to introduce two different pop types.

data.push(3);
for (uint j = 0; j < 33; j++)
data.pop();
l = data.length;

This comment has been minimized.

@chriseth

chriseth Mar 30, 2018

Contributor

Please add another test that checks that the data is actually properly copied, i.e. a test that adds 33 elements and thes removes e.g. 4.

This comment has been minimized.

@bit-shift

bit-shift Apr 4, 2018

Contributor

Done.

{
char const* sourceCode = R"(
contract c {
uint[] data;

This comment has been minimized.

@chriseth

chriseth Mar 30, 2018

Contributor

Please add a test that uses uint16, I have a suspicion that this will trigger an assertion.

This comment has been minimized.

@bit-shift

bit-shift Apr 4, 2018

Contributor

Done.

// Do not zero-out the least significant byte, but mask the
// higher bits of the length.
// (((1<<(8 * (32 - length))) - 1) << 8) + 128
let mask := add(mul(0x100, sub(exp(0x100, sub(32, length)), 1)), 0x80)

This comment has been minimized.

@chriseth

chriseth Mar 30, 2018

Contributor

Shouldn't this be 0xc0 at the end (the negation of 0x3f)?

// Zero-out the suffix of the byte array by masking it.
// ((1<<(8 * (32 - offset))) - 1)
let mask := sub(exp(0x100, sub(32, offset)), 1)

This comment has been minimized.

@chriseth

chriseth Mar 30, 2018

Contributor

To really thoroughly test these routines, you could make tiny changes to the constants and see if a test fails. If no test fails, we do not have enough coverage :)

This comment has been minimized.

@bit-shift

bit-shift Apr 4, 2018

Contributor

That revealed more missing tests. Need to put more effort into covering those.

else
{
// stack: ArrayReference

This comment has been minimized.

@axic

axic Apr 3, 2018

Member

While we are at rewriting some stuff here, would it make sense rewriting the below as inline assembly?

This comment has been minimized.

@chriseth

chriseth Apr 3, 2018

Contributor

We considered it, but the code uses too many helper functions that are not translated yet.

@axic

This comment has been minimized.

Member

axic commented Apr 4, 2018

@bit-shift do you want to focus on making these changes today?

@bit-shift bit-shift self-assigned this Apr 4, 2018

@bit-shift bit-shift added this to Under Review in 0.4.22 Apr 5, 2018

@bit-shift bit-shift removed this from Under Review in 0.4.22 Apr 5, 2018

@bit-shift bit-shift added this to To do in 0.5.0 via automation Apr 5, 2018

@bit-shift bit-shift moved this from To do to Under review in 0.5.0 Apr 5, 2018

// Do not zero-out the least significant byte, but mask the
// higher bits of the length.
// (((1<<(8 * (32 - length))) - 1) << 8) + 128
let mask := add(mul(0x100, sub(exp(0x100, sub(32, length)), 1)), 0xc0)

This comment has been minimized.

@axic

axic Apr 5, 2018

Member

It probably would make sense somehow using the shiftLeftFunction from ABIFunctions here or better, why not create the mask in C++ (never mind, didn't see the length comes from sload)?

switch and(slot_value, 1)
case 0 {
// short byte array
let length := and(div(slot_value, 2), 0x3f)

This comment has been minimized.

@chriseth

chriseth Apr 6, 2018

Contributor

Hm, actually this should be 1f shouldn't it?

// higher bits of the length.
// (((1<<(8 * (32 - length))) - 1) << 8) + 192
let mask := add(mul(0x100, sub(exp(0x100, sub(32, length)), 1)), 0xc0)
slot_value := and(not(mask), slot_value)

This comment has been minimized.

@chriseth

chriseth Apr 6, 2018

Contributor

Perhaps the following would be easier to understand:

let mask := mul(0x100, sub(exp(0x100, sub(32, length)), 1))
length := sub(length, 1)
slot_value := or(and(not(mask), slot_value), mul(length, 2))

I.e. just mask away the least significant byte and then or-in the adjusted length.

switch length
case 32
{
let slot := keccak256(0, 0x20)

This comment has been minimized.

@chriseth

chriseth Apr 6, 2018

Contributor

Perhaps it is better to move the keccak call in front of the switch, otherwise it might be hard to see when the memory was written to.

This comment has been minimized.

@axic

axic Apr 23, 2018

Member

This makes sense.

contract c {
uint16[] data;
function test() public {
data.push(7);

This comment has been minimized.

@chriseth

chriseth Apr 6, 2018

Contributor

Please add another test that pushes as many times as is needed to fill the first slot and start storing data in the second.

@axic

Should have syntax tests.

@chriseth

This comment has been minimized.

Contributor

chriseth commented May 16, 2018

Can you please add a syntax test for a statically-sized storage array and a dynamically-sized memory array?

let length := and(div(slot_value, 2), 0x1f)
if iszero(length) { invalid() }
// Zero-out the suffix inlcluding the least significant byte.

This comment has been minimized.

@chriseth

chriseth May 16, 2018

Contributor

Typo: inlcluding

@@ -73,6 +73,11 @@ class ArrayUtils
/// Stack pre: reference (excludes byte offset)
/// Stack post: new_length
void incrementDynamicArraySize(ArrayType const& _type) const;
/// Decrements the size of a dynamic array by one if length is nonzero. Returns otherwise.

This comment has been minimized.

@chriseth

chriseth May 23, 2018

Contributor

Returns otherwise should be Causes an invalid instruction otherwise.

case 1 {
// long byte array
let length := div(slot_value, 2)
let slot := keccak256(0, 0x20)

This comment has been minimized.

@chriseth

chriseth May 23, 2018

Contributor

This and the next line have to be swapped. Why don't we have a test failure for this?

Can you please modify the tests such that the arrays are not the first element in storage, i.e. add some unused storage variables at the beginning of the contract.

This comment has been minimized.

@bit-shift

bit-shift May 30, 2018

Contributor

@chriseth I moved mstore(0, ref) in front of the keccak256 call. But also without this change I wasn't able to provoke a test failure by adding unused storage variables to the test contracts. Even after removing the mstore call all tests passed. Now I'm a bit scared ;) Do you have an idea how we could bisect this? Doesn't keccak expects the array reference to be stored in memory?

This comment has been minimized.

@chriseth

chriseth May 30, 2018

Contributor

Probably the most reliable way to create such a test is to just use assembly { mstore(0, "garbage") } x.pop();

default
{
let slot_offset := div(sub(length, 1), 32)
let length_offset := and(sub(length, 1), 0x1f)

This comment has been minimized.

@chriseth

chriseth May 23, 2018

Contributor

A better name for this variable should be offset_inside_slot

{
let slot_offset := div(sub(length, 1), 32)
let length_offset := and(sub(length, 1), 0x1f)
slot := add(slot, slot_offset)

This comment has been minimized.

@chriseth

chriseth May 23, 2018

Contributor

Perhaps it would be less confusing to remove the slot_offset variable and combine with assignment with line 864.

}
)";
compileAndRun(sourceCode);
ABI_CHECK(callContractFunction("test()"), encodeArgs(32));

This comment has been minimized.

@chriseth

chriseth May 23, 2018

Contributor

Please check for storage empty.

This comment has been minimized.

@bit-shift

bit-shift May 30, 2018

Contributor

Does that make sense in this case? Not all elements are being popped so the storage is not empty. I would remove this test case completely since it's now testing more or less the same than the improved byte_array_pop_storage_empty_long.

This comment has been minimized.

@chriseth

chriseth May 30, 2018

Contributor

ok!

bytes data;
function test() public returns (uint l) {
for (uint i = 0; i < 33; i++)
data.push(3);

This comment has been minimized.

@chriseth

chriseth May 23, 2018

Contributor

Can you change this test to e.g. push(i+1) and then pop all of them again, testing that data[i] == (i+1) and that data.length is correct for each iteration? You can use require for that.

@chriseth chriseth merged commit 5a73044 into develop May 30, 2018

9 of 10 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci: build_emscripten Your tests passed on CircleCI!
Details
ci/circleci: build_x86_linux Your tests passed on CircleCI!
Details
ci/circleci: build_x86_mac Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_external Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_solcjs Your tests passed on CircleCI!
Details
ci/circleci: test_x86_linux Your tests passed on CircleCI!
Details
ci/circleci: test_x86_mac Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

0.5.0 automation moved this from Under review to Done May 30, 2018

@axic axic deleted the popStorageArray branch May 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment