From 5216a9bc678597c0076b2e8615cac43c9077a95e Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 31 Mar 2015 14:59:38 +0200 Subject: [PATCH 1/4] Some cleanup concerning byte arrays. --- ArrayUtils.cpp | 118 ++++++++++++++++++---------------------------- Compiler.cpp | 4 -- CompilerUtils.cpp | 2 +- LValue.cpp | 4 +- Types.h | 2 +- 5 files changed, 51 insertions(+), 79 deletions(-) diff --git a/ArrayUtils.cpp b/ArrayUtils.cpp index 58031390f..1a91b0537 100644 --- a/ArrayUtils.cpp +++ b/ArrayUtils.cpp @@ -388,10 +388,7 @@ void ArrayUtils::convertLengthToSize(ArrayType const& _arrayType, bool _pad) con { if (_arrayType.getLocation() == ArrayType::Location::Storage) { - if (_arrayType.isByteArray()) - m_context << u256(31) << eth::Instruction::ADD - << u256(32) << eth::Instruction::SWAP1 << eth::Instruction::DIV; - else if (_arrayType.getBaseType()->getStorageSize() <= 1) + if (_arrayType.getBaseType()->getStorageSize() <= 1) { unsigned baseBytes = _arrayType.getBaseType()->getStorageBytes(); if (baseBytes == 0) @@ -465,82 +462,61 @@ void ArrayUtils::accessIndex(ArrayType const& _arrayType) const m_context << legalAccess; // stack: - if (_arrayType.isByteArray()) - switch (location) - { - case ArrayType::Location::Storage: - // byte array index storage lvalue on stack (goal): - // = - m_context << u256(32) << eth::Instruction::SWAP2; + m_context << eth::Instruction::SWAP1; + if (_arrayType.isDynamicallySized()) + { + if (location == ArrayType::Location::Storage) CompilerUtils(m_context).computeHashStatic(); - // stack: 32 index data_ref + else if (location == ArrayType::Location::Memory) + m_context << u256(32) << eth::Instruction::ADD; + } + // stack: + switch (location) + { + case ArrayType::Location::CallData: + if (!_arrayType.isByteArray()) + m_context + << eth::Instruction::SWAP1 << _arrayType.getBaseType()->getCalldataEncodedSize() + << eth::Instruction::MUL; + m_context << eth::Instruction::ADD; + if (_arrayType.getBaseType()->isValueType()) + CompilerUtils(m_context).loadFromMemoryDynamic( + *_arrayType.getBaseType(), + true, + !_arrayType.isByteArray(), + false + ); + break; + case ArrayType::Location::Storage: + m_context << eth::Instruction::SWAP1; + if (_arrayType.getBaseType()->getStorageBytes() <= 16) + { + // stack: + // goal: + // = <(index % itemsPerSlot) * byteSize> + unsigned byteSize = _arrayType.getBaseType()->getStorageBytes(); + solAssert(byteSize != 0, ""); + unsigned itemsPerSlot = 32 / byteSize; + m_context << u256(itemsPerSlot) << eth::Instruction::SWAP2; + // stack: itemsPerSlot index data_ref m_context << eth::Instruction::DUP3 << eth::Instruction::DUP3 << eth::Instruction::DIV << eth::Instruction::ADD - // stack: 32 index (data_ref + index / 32) + // stack: itemsPerSlot index (data_ref + index / itemsPerSlot) << eth::Instruction::SWAP2 << eth::Instruction::SWAP1 << eth::Instruction::MOD; - break; - case ArrayType::Location::CallData: - // no lvalue, just retrieve the value - m_context - << eth::Instruction::ADD << eth::Instruction::CALLDATALOAD - << ((u256(0xff) << (256 - 8))) << eth::Instruction::AND; - break; - case ArrayType::Location::Memory: - solAssert(false, "Memory lvalues not yet implemented."); + if (byteSize != 1) + m_context << u256(byteSize) << eth::Instruction::MUL; } - else - { - // stack: - m_context << eth::Instruction::SWAP1; - if (_arrayType.isDynamicallySized()) + else { - if (location == ArrayType::Location::Storage) - CompilerUtils(m_context).computeHashStatic(); - else if (location == ArrayType::Location::Memory) - m_context << u256(32) << eth::Instruction::ADD; - } - // stack: - switch (location) - { - case ArrayType::Location::CallData: - m_context - << eth::Instruction::SWAP1 << _arrayType.getBaseType()->getCalldataEncodedSize() - << eth::Instruction::MUL << eth::Instruction::ADD; - if (_arrayType.getBaseType()->isValueType()) - CompilerUtils(m_context).loadFromMemoryDynamic(*_arrayType.getBaseType(), true, true, false); - break; - case ArrayType::Location::Storage: - m_context << eth::Instruction::SWAP1; - if (_arrayType.getBaseType()->getStorageBytes() <= 16) - { - // stack: - // goal: - // = <(index % itemsPerSlot) * byteSize> - unsigned byteSize = _arrayType.getBaseType()->getStorageBytes(); - solAssert(byteSize != 0, ""); - unsigned itemsPerSlot = 32 / byteSize; - m_context << u256(itemsPerSlot) << eth::Instruction::SWAP2; - // stack: itemsPerSlot index data_ref - m_context - << eth::Instruction::DUP3 << eth::Instruction::DUP3 - << eth::Instruction::DIV << eth::Instruction::ADD - // stack: itemsPerSlot index (data_ref + index / itemsPerSlot) - << eth::Instruction::SWAP2 << eth::Instruction::SWAP1 - << eth::Instruction::MOD - << u256(byteSize) << eth::Instruction::MUL; - } - else - { - if (_arrayType.getBaseType()->getStorageSize() != 1) - m_context << _arrayType.getBaseType()->getStorageSize() << eth::Instruction::MUL; - m_context << eth::Instruction::ADD << u256(0); - } - break; - case ArrayType::Location::Memory: - solAssert(false, "Memory lvalues not yet implemented."); + if (_arrayType.getBaseType()->getStorageSize() != 1) + m_context << _arrayType.getBaseType()->getStorageSize() << eth::Instruction::MUL; + m_context << eth::Instruction::ADD << u256(0); } + break; + case ArrayType::Location::Memory: + solAssert(false, "Memory lvalues not yet implemented."); } } diff --git a/Compiler.cpp b/Compiler.cpp index 8e2634499..886565cb4 100644 --- a/Compiler.cpp +++ b/Compiler.cpp @@ -254,7 +254,6 @@ void Compiler::appendCalldataUnpacker(TypePointers const& _typeParameters, bool void Compiler::appendReturnValuePacker(TypePointers const& _typeParameters) { - //@todo this can be also done more efficiently unsigned dataOffset = 0; unsigned stackDepth = 0; for (TypePointer const& type: _typeParameters) @@ -303,9 +302,6 @@ bool Compiler::visit(VariableDeclaration const& _variableDeclaration) bool Compiler::visit(FunctionDefinition const& _function) { CompilerContext::LocationSetter locationSetter(m_context, _function); - //@todo to simplify this, the calling convention could by changed such that - // caller puts: [retarg0] ... [retargm] [return address] [arg0] ... [argn] - // although note that this reduces the size of the visible stack m_context.startFunction(_function); diff --git a/CompilerUtils.cpp b/CompilerUtils.cpp index 454951147..8d3e9d2a2 100644 --- a/CompilerUtils.cpp +++ b/CompilerUtils.cpp @@ -93,7 +93,7 @@ void CompilerUtils::storeInMemoryDynamic(Type const& _type, bool _padToWordBound else { solAssert(type.getLocation() == ArrayType::Location::Storage, "Memory arrays not yet implemented."); - m_context << eth::Instruction::POP; //@todo + m_context << eth::Instruction::POP; // remove offset, arrays always start new slot m_context << eth::Instruction::DUP1 << eth::Instruction::SLOAD; // stack here: memory_offset storage_offset length_bytes // jump to end if length is zero diff --git a/LValue.cpp b/LValue.cpp index 234072bce..02e6cbca5 100644 --- a/LValue.cpp +++ b/LValue.cpp @@ -225,7 +225,8 @@ void StorageItem::setToZero(SourceLocation const&, bool _removeReference) const else if (m_dataType.getCategory() == Type::Category::Struct) { // stack layout: storage_key storage_offset - // @todo this can be improved for packed types + // @todo this can be improved: use StorageItem for non-value types, and just store 0 in + // all slots that contain value types later. auto const& structType = dynamic_cast(m_dataType); for (auto const& member: structType.getMembers()) { @@ -245,7 +246,6 @@ void StorageItem::setToZero(SourceLocation const&, bool _removeReference) const else { solAssert(m_dataType.isValueType(), "Clearing of unsupported type requested: " + m_dataType.toString()); - // @todo actually use offset if (!_removeReference) CompilerUtils(m_context).copyToStackTop(sizeOnStack(), sizeOnStack()); if (m_dataType.getStorageBytes() == 32) diff --git a/Types.h b/Types.h index e1852bc7f..e6d32d175 100644 --- a/Types.h +++ b/Types.h @@ -345,7 +345,7 @@ public: explicit ArrayType(Location _location): m_location(_location), m_isByteArray(true), - m_baseType(std::make_shared(8)) + m_baseType(std::make_shared(1)) {} /// Constructor for a dynamically sized array type ("type[]") ArrayType(Location _location, const TypePointer &_baseType): From 820239a73c08ceb359e5d7dda9162b7e2f0fcaed Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 15 Apr 2015 18:05:14 +0200 Subject: [PATCH 2/4] Line break. --- ArrayUtils.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ArrayUtils.cpp b/ArrayUtils.cpp index 1a91b0537..448e4da2a 100644 --- a/ArrayUtils.cpp +++ b/ArrayUtils.cpp @@ -476,7 +476,8 @@ void ArrayUtils::accessIndex(ArrayType const& _arrayType) const case ArrayType::Location::CallData: if (!_arrayType.isByteArray()) m_context - << eth::Instruction::SWAP1 << _arrayType.getBaseType()->getCalldataEncodedSize() + << eth::Instruction::SWAP1 + << _arrayType.getBaseType()->getCalldataEncodedSize() << eth::Instruction::MUL; m_context << eth::Instruction::ADD; if (_arrayType.getBaseType()->isValueType()) From 6e5de4832da7a3c2fdbc87b71523ac671083d9c0 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 16 Apr 2015 00:06:57 +0200 Subject: [PATCH 3/4] Bugfixes concerning variable declarations. Fixes #1637 --- AST.cpp | 16 +++++++--------- Parser.cpp | 23 ++++++++++++----------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/AST.cpp b/AST.cpp index 095ba7bf1..0abdf7819 100644 --- a/AST.cpp +++ b/AST.cpp @@ -378,20 +378,16 @@ void VariableDeclaration::checkTypeRequirements() if ((m_type && !m_type->isValueType()) || !m_value) BOOST_THROW_EXCEPTION(createTypeError("Unitialized \"constant\" variable.")); } - if (!m_value) - return; if (m_type) { - m_value->expectType(*m_type); - if (m_isStateVariable && !m_type->externalType() && getVisibility() >= Visibility::Public) - BOOST_THROW_EXCEPTION(createTypeError("Internal type is not allowed for state variables.")); - - if (!FunctionType(*this).externalType()) - BOOST_THROW_EXCEPTION(createTypeError("Internal type is not allowed for public state variables.")); + if (m_value) + m_value->expectType(*m_type); } else { - // no type declared and no previous assignment, infer the type + if (!m_value) + // This feature might be extended in the future. + BOOST_THROW_EXCEPTION(createTypeError("Assignment necessary for type detection.")); m_value->checkTypeRequirements(); TypePointer type = m_value->getType(); if (type->getCategory() == Type::Category::IntegerConstant) @@ -405,6 +401,8 @@ void VariableDeclaration::checkTypeRequirements() BOOST_THROW_EXCEPTION(createTypeError("Variable cannot have void type.")); m_type = type; } + if (m_isStateVariable && getVisibility() >= Visibility::Public && !FunctionType(*this).externalType()) + BOOST_THROW_EXCEPTION(createTypeError("Internal type is not allowed for public state variables.")); } bool VariableDeclaration::isExternalFunctionParameter() const diff --git a/Parser.cpp b/Parser.cpp index 5c7676df5..43571314a 100644 --- a/Parser.cpp +++ b/Parser.cpp @@ -472,17 +472,18 @@ ASTPointer Parser::parseTypeName(bool _allowVar) else BOOST_THROW_EXCEPTION(createParserError("Expected type name")); - // Parse "[...]" postfixes for arrays. - while (m_scanner->getCurrentToken() == Token::LBrack) - { - m_scanner->next(); - ASTPointer length; - if (m_scanner->getCurrentToken() != Token::RBrack) - length = parseExpression(); - nodeFactory.markEndPosition(); - expectToken(Token::RBrack); - type = nodeFactory.createNode(type, length); - } + if (type) + // Parse "[...]" postfixes for arrays. + while (m_scanner->getCurrentToken() == Token::LBrack) + { + m_scanner->next(); + ASTPointer length; + if (m_scanner->getCurrentToken() != Token::RBrack) + length = parseExpression(); + nodeFactory.markEndPosition(); + expectToken(Token::RBrack); + type = nodeFactory.createNode(type, length); + } return type; } From 5622364a98102574f04019e2f1041d54d2d04984 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 16 Apr 2015 17:43:40 +0200 Subject: [PATCH 4/4] Fix for signed integers in storage. --- LValue.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/LValue.cpp b/LValue.cpp index 02e6cbca5..68428a0bf 100644 --- a/LValue.cpp +++ b/LValue.cpp @@ -107,6 +107,11 @@ void StorageItem::retrieveValue(SourceLocation const&, bool _remove) const << u256(0x100) << eth::Instruction::EXP << eth::Instruction::SWAP1 << eth::Instruction::DIV; if (m_dataType.getCategory() == Type::Category::FixedBytes) m_context << (u256(0x1) << (256 - 8 * m_dataType.getStorageBytes())) << eth::Instruction::MUL; + else if ( + m_dataType.getCategory() == Type::Category::Integer && + dynamic_cast(m_dataType).isSigned() + ) + m_context << u256(m_dataType.getStorageBytes() - 1) << eth::Instruction::SIGNEXTEND; else m_context << ((u256(0x1) << (8 * m_dataType.getStorageBytes())) - 1) << eth::Instruction::AND; } @@ -148,6 +153,17 @@ void StorageItem::storeValue(Type const& _sourceType, SourceLocation const& _loc m_context << (u256(0x1) << (256 - 8 * dynamic_cast(m_dataType).getNumBytes())) << eth::Instruction::SWAP1 << eth::Instruction::DIV; + else if ( + m_dataType.getCategory() == Type::Category::Integer && + dynamic_cast(m_dataType).isSigned() + ) + // remove the higher order bits + m_context + << (u256(1) << (8 * (32 - m_dataType.getStorageBytes()))) + << eth::Instruction::SWAP1 + << eth::Instruction::DUP2 + << eth::Instruction::MUL + << eth::Instruction::DIV; m_context << eth::Instruction::MUL << eth::Instruction::OR; // stack: value storage_ref updated_value m_context << eth::Instruction::SWAP1 << eth::Instruction::SSTORE;