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

New ABI encoder #2704

Merged
merged 8 commits into from Aug 14, 2017

Conversation

Projects
None yet
2 participants
@chriseth
Contributor

chriseth commented Aug 7, 2017

This is disabled for now (waiting for "experimental pragma").

Depends on: #2701 , #2700 , #2690

part of #2433

@chriseth chriseth added the in progress label Aug 7, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 8, 2017

Member

What experimental pragma to use?

abiEncoderV2 ?

Member

axic commented Aug 8, 2017

What experimental pragma to use?

abiEncoderV2 ?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 10, 2017

Member

Can you please rebase?

Member

axic commented Aug 10, 2017

Can you please rebase?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Aug 10, 2017

Contributor

This allows multi-dimensional arrays in interfaces unconditionally, i.e. now only if the experimental mode is turned on. This is more or less required because it is a type feature.

I don't think this is harmful, because it will only cause an "unimplemented feature" exception in the code generator. I don't think it changes any interfaces, but I'm not 100% positive on that.

Contributor

chriseth commented Aug 10, 2017

This allows multi-dimensional arrays in interfaces unconditionally, i.e. now only if the experimental mode is turned on. This is more or less required because it is a type feature.

I don't think this is harmful, because it will only cause an "unimplemented feature" exception in the code generator. I don't think it changes any interfaces, but I'm not 100% positive on that.

@@ -1528,8 +1528,6 @@ TypePointer ArrayType::interfaceType(bool _inLibrary) const
TypePointer baseExt = m_baseType->interfaceType(_inLibrary);
if (!baseExt)
return TypePointer();
if (m_baseType->category() == Category::Array && m_baseType->isDynamicallySized())

This comment has been minimized.

@axic

axic Aug 10, 2017

Member

Can you fold this change into another commit or rename the commit message?

@axic

axic Aug 10, 2017

Member

Can you fold this change into another commit or rename the commit message?

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

Done.

@axic

axic Aug 14, 2017

Member

Done.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Aug 11, 2017

Contributor

Ok, hopefully no failures anymore now.

Contributor

chriseth commented Aug 11, 2017

Ok, hopefully no failures anymore now.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 11, 2017

Member

Fails on:

/home/travis/build/ethereum/solidity/libsolidity/codegen/ArrayUtils.cpp(290): fatal error in "void dev::solidity::ArrayUtils::copyArrayToMemory(const dev::solidity::ArrayType &, bool) const": std::exception: Nested dynamic arrays not implemented here.
/home/travis/build/ethereum/solidity/test/RPCSession.cpp(324): last checkpoint
Member

axic commented Aug 11, 2017

Fails on:

/home/travis/build/ethereum/solidity/libsolidity/codegen/ArrayUtils.cpp(290): fatal error in "void dev::solidity::ArrayUtils::copyArrayToMemory(const dev::solidity::ArrayType &, bool) const": std::exception: Nested dynamic arrays not implemented here.
/home/travis/build/ethereum/solidity/test/RPCSession.cpp(324): last checkpoint
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 11, 2017

Member

I haven't reviewed the actual encoder yet, but the rest (bar the above) seems ok.

Member

axic commented Aug 11, 2017

I haven't reviewed the actual encoder yet, but the rest (bar the above) seems ok.

@chriseth chriseth referenced this pull request Aug 11, 2017

Closed

[WIP] Struct encoder #2433

2 of 3 tasks complete

@axic axic added the nextrelease label Aug 11, 2017

toCategory == Type::Category::Integer ||
toCategory == Type::Category::Contract,
"");
IntegerType const addressType(0, IntegerType::Modifier::Address);

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

Same as above, 160, Address I guess.

@axic

axic Aug 14, 2017

Member

Same as above, 160, Address I guess.

<#word>
mstore(add(pos, <offset>), <wordValue>)
</word>
end := add(pos, <overallSize>)

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

You could increment the pos in-place in the loop and use end := add(pos, 64).

@axic

axic Aug 14, 2017

Member

You could increment the pos in-place in the loop and use end := add(pos, 64).

This comment has been minimized.

@chriseth

chriseth Aug 14, 2017

Contributor

I think this way it is better for the optimizer.

@chriseth

chriseth Aug 14, 2017

Contributor

I think this way it is better for the optimizer.

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

How so?

@axic

axic Aug 14, 2017

Member

How so?

if (_to.isDynamicallySized())
{
Whiskers templ(R"(
function <functionName>(pos) -> end {

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

Hm, this one returns the occupied memory size but the other encoding functions below don't?

@axic

axic Aug 14, 2017

Member

Hm, this one returns the occupied memory size but the other encoding functions below don't?

This comment has been minimized.

@chriseth

chriseth Aug 14, 2017

Contributor

It returns the "new memory pointer" if and only if it is dynamically sized. You can take a look at how the functions are called, it is consistent and should be checked by the julia analyzer.

@chriseth

chriseth Aug 14, 2017

Contributor

It returns the "new memory pointer" if and only if it is dynamically sized. You can take a look at how the functions are called, it is consistent and should be checked by the julia analyzer.

function <functionName>(src, dst, length) {
calldatacopy(dst, src, length)
// clear end
mstore(add(dst, length), 0)

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

Why is there an extra slot at the end?

@axic

axic Aug 14, 2017

Member

Why is there an extra slot at the end?

This comment has been minimized.

@chriseth

chriseth Aug 14, 2017

Contributor

It just removes potential garbage. The routine might write to more memory than it actually "returns" in the end. This is documented in the header.

@chriseth

chriseth Aug 14, 2017

Contributor

It just removes potential garbage. The routine might write to more memory than it actually "returns" in the end. This is documented in the header.

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

It just feels weird this is not handled in the caller, why would it ask for more if the entire slot is cleared?

@axic

axic Aug 14, 2017

Member

It just feels weird this is not handled in the caller, why would it ask for more if the entire slot is cleared?

This comment has been minimized.

@axic

axic Aug 21, 2017

Member

@chriseth this question wasn't sorted out

@axic

axic Aug 21, 2017

Member

@chriseth this question wasn't sorted out

This comment has been minimized.

@chriseth

chriseth Aug 23, 2017

Contributor

The ABI specification requires everything to be padded with zeros to multiples of 32 bytes. The calldatacopy only copies the actual length. This line performs the padding, especially in the case when the memory was pre-occupied by something else.

@chriseth

chriseth Aug 23, 2017

Contributor

The ABI specification requires everything to be padded with zeros to multiples of 32 bytes. The calldatacopy only copies the actual length. This line performs the padding, especially in the case when the memory was pre-occupied by something else.

switch (_type.location())
{
case DataLocation::CallData:
solAssert(false, "called regular array length function on calldata array");

This comment has been minimized.

@axic

axic Aug 14, 2017

Member

Shouldn't this be possible?

@axic

axic Aug 14, 2017

Member

Shouldn't this be possible?

This comment has been minimized.

@chriseth

chriseth Aug 14, 2017

Contributor

No, because calldata arrays are stored as (pos, length), so you cannot retrieve the length from pos alone.

@chriseth

chriseth Aug 14, 2017

Contributor

No, because calldata arrays are stored as (pos, length), so you cannot retrieve the length from pos alone.

@axic

axic approved these changes Aug 14, 2017

@chriseth chriseth merged commit 2411f5d into develop Aug 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@axic axic deleted the newEncoder branch Aug 15, 2017

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