From f33dc92cbd908a6d852368fa30144bda9e8da439 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Mon, 7 May 2018 19:09:52 +0200 Subject: [PATCH 1/2] Use proper SAR for signed right shifts and emulate on pre-constantinople. --- Changelog.md | 1 + docs/types.rst | 13 +- libsolidity/ast/Types.cpp | 11 +- libsolidity/codegen/ExpressionCompiler.cpp | 25 ++- test/libsolidity/SolidityEndToEndTest.cpp | 172 ++++++++++++++------- 5 files changed, 152 insertions(+), 70 deletions(-) diff --git a/Changelog.md b/Changelog.md index 0050f9a67..d5b440b2e 100644 --- a/Changelog.md +++ b/Changelog.md @@ -8,6 +8,7 @@ Breaking Changes: * Commandline interface: Require ``-`` if standard input is used as source. * General: New keywords: ``calldata`` * General: ``continue`` in a ``do...while`` loop jumps to the condition (it used to jump to the loop body). Warning: this may silently change the semantics of existing code. + * General: Signed right shift uses proper arithmetic shift, i.e. rounding towards negative infinity. Warning: this may silently change the semantics of existing code! * Introduce ``emit`` as a keyword instead of parsing it as identifier. * Type Checker: Disallow arithmetic operations for Boolean variables. * Disallow trailing dots that are not followed by a number. diff --git a/docs/types.rst b/docs/types.rst index 08b74241b..009896d5b 100644 --- a/docs/types.rst +++ b/docs/types.rst @@ -60,15 +60,14 @@ operators are :ref:`literals` (or literal expressions). Division by zero and modulus with zero throws a runtime exception. The result of a shift operation is the type of the left operand. The -expression ``x << y`` is equivalent to ``x * 2**y``, and ``x >> y`` is -equivalent to ``x / 2**y``. This means that shifting negative numbers -sign extends. Shifting by a negative amount throws a runtime exception. +expression ``x << y`` is equivalent to ``x * 2**y``, and, for positive integers, +``x >> y`` is equivalent to ``x / 2**y``. For negative ``x``, ``x >> y`` +is equivalent to dividing by a power of ``2`` while rounding down (towards negative infinity). +Shifting by a negative amount throws a runtime exception. .. warning:: - The results produced by shift right of negative values of signed integer types is different from those produced - by other programming languages. In Solidity, shift right maps to division so the shifted negative values - are going to be rounded towards zero (truncated). In other programming languages the shift right of negative values - works like division with rounding down (towards negative infinity). + Before version ``0.5.0`` a right shift ``x >> y`` for negative ``x`` was equivalent to ``x / 2**y``, + i.e. right shifts used rounding towards zero instead of rounding towards negative infinity. .. index:: ! ufixed, ! fixed, ! fixed point number diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp index 1e0565c08..94e04b6a3 100644 --- a/libsolidity/ast/Types.cpp +++ b/libsolidity/ast/Types.cpp @@ -1084,9 +1084,16 @@ TypePointer RationalNumberType::binaryOperatorResult(Token::Value _operator, Typ { uint32_t exponent = other.m_value.numerator().convert_to(); if (exponent > mostSignificantBit(boost::multiprecision::abs(m_value.numerator()))) - value = 0; + value = m_value.numerator() < 0 ? -1 : 0; else - value = rational(m_value.numerator() / boost::multiprecision::pow(bigint(2), exponent), 1); + { + if (m_value.numerator() < 0) + // add 1 to the negative value before dividing to get a result that is strictly too large + // subtract 1 afterwards to round towards negative infinity + value = rational((m_value.numerator() + 1) / boost::multiprecision::pow(bigint(2), exponent) - bigint(1), 1); + else + value = rational(m_value.numerator() / boost::multiprecision::pow(bigint(2), exponent), 1); + } } break; } diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index 93d440c85..908c703ab 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -1737,11 +1737,28 @@ void ExpressionCompiler::appendShiftOperatorCode(Token::Value _operator, Type co m_context << u256(2) << Instruction::EXP << Instruction::MUL; break; case Token::SAR: - // NOTE: SAR rounds differently than SDIV - if (m_context.evmVersion().hasBitwiseShifting() && !c_valueSigned) - m_context << Instruction::SHR; + if (m_context.evmVersion().hasBitwiseShifting()) + m_context << (c_valueSigned ? Instruction::SAR : Instruction::SHR); else - m_context << u256(2) << Instruction::EXP << Instruction::SWAP1 << (c_valueSigned ? Instruction::SDIV : Instruction::DIV); + { + if (c_valueSigned) + // For negative values xor_mask has all bits set and xor(value_to_shift, xor_mask) will be + // the bitwise complement of value_to_shift, i.e. abs(value_to_shift) - 1. Dividing this by + // exp(2, shift_amount) results in a value that is positive and strictly smaller than the + // absolute value of the desired result. Taking the complement again changes the sign + // back to negative and subtracts one, resulting in rounding towards negative infinity. + // For positive values xor_mask is zero and xor(value_to_shift, xor_mask) is again value_to_shift. + m_context.appendInlineAssembly(R"({ + let xor_mask := sub(0, slt(value_to_shift, 0)) + value_to_shift := xor(div(xor(value_to_shift, xor_mask), exp(2, shift_amount)), xor_mask) + })", {"value_to_shift", "shift_amount"}); + else + m_context.appendInlineAssembly(R"({ + value_to_shift := div(value_to_shift, exp(2, shift_amount)) + })", {"value_to_shift", "shift_amount"}); + m_context << Instruction::POP; + + } break; case Token::SHR: default: diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index b53a92947..3b3cc4f79 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -10487,6 +10487,7 @@ BOOST_AUTO_TEST_CASE(shift_right) ABI_CHECK(callContractFunction("f(uint256,uint256)", u256(0x4266), u256(8)), encodeArgs(u256(0x42))); ABI_CHECK(callContractFunction("f(uint256,uint256)", u256(0x4266), u256(16)), encodeArgs(u256(0))); ABI_CHECK(callContractFunction("f(uint256,uint256)", u256(0x4266), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(uint256,uint256)", u256(1)<<255, u256(5)), encodeArgs(u256(1)<<250)); } BOOST_AUTO_TEST_CASE(shift_right_garbled) @@ -10583,16 +10584,73 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue) compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(0)), encodeArgs(u256(-4266))); ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(17)), encodeArgs(u256(-1))); ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(0)), encodeArgs(u256(-4267))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(1)), encodeArgs(u256(-2134))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(17)), encodeArgs(u256(-1))); +} + +BOOST_AUTO_TEST_CASE(shift_right_negative_literal) +{ + char const* sourceCode = R"( + contract C { + function f1() pure returns (bool) { + return (-4266 >> 0) == -4266; + } + function f2() pure returns (bool) { + return (-4266 >> 1) == -2133; + } + function f3() pure returns (bool) { + return (-4266 >> 4) == -267; + } + function f4() pure returns (bool) { + return (-4266 >> 8) == -17; + } + function f5() pure returns (bool) { + return (-4266 >> 16) == -1; + } + function f6() pure returns (bool) { + return (-4266 >> 17) == -1; + } + function g1() pure returns (bool) { + return (-4267 >> 0) == -4267; + } + function g2() pure returns (bool) { + return (-4267 >> 1) == -2134; + } + function g3() pure returns (bool) { + return (-4267 >> 4) == -267; + } + function g4() pure returns (bool) { + return (-4267 >> 8) == -17; + } + function g5() pure returns (bool) { + return (-4267 >> 16) == -1; + } + function g6() pure returns (bool) { + return (-4267 >> 17) == -1; + } + } + )"; + compileAndRun(sourceCode, 0, "C"); + ABI_CHECK(callContractFunction("f1()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("f2()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("f3()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("f4()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("f5()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("f6()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("g1()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("g2()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("g3()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("g4()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("g5()"), encodeArgs(true)); + ABI_CHECK(callContractFunction("g6()"), encodeArgs(true)); } BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_int8) @@ -10607,16 +10665,16 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_int8) compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(0)), encodeArgs(u256(-66))); ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(1)), encodeArgs(u256(-33))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(4)), encodeArgs(u256(-4))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(8)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(4)), encodeArgs(u256(-5))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(8)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-66), u256(17)), encodeArgs(u256(-1))); ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(0)), encodeArgs(u256(-67))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(1)), encodeArgs(u256(-33))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(4)), encodeArgs(u256(-4))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(8)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(1)), encodeArgs(u256(-34))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(4)), encodeArgs(u256(-5))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(8)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(-67), u256(17)), encodeArgs(u256(-1))); } BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_signextend_int8) @@ -10630,10 +10688,10 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_signextend_int8) )"; compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(0)), encodeArgs(u256(-103))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(1)), encodeArgs(u256(-51))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(2)), encodeArgs(u256(-25))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(4)), encodeArgs(u256(-6))); - ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(8)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(1)), encodeArgs(u256(-52))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(2)), encodeArgs(u256(-26))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(4)), encodeArgs(u256(-7))); + ABI_CHECK(callContractFunction("f(int8,int8)", u256(0x99u), u256(8)), encodeArgs(u256(-1))); } BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_signextend_int16) @@ -10647,10 +10705,10 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_signextend_int16) )"; compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(0)), encodeArgs(u256(-103))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(1)), encodeArgs(u256(-51))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(2)), encodeArgs(u256(-25))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(4)), encodeArgs(u256(-6))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(8)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(1)), encodeArgs(u256(-52))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(2)), encodeArgs(u256(-26))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(4)), encodeArgs(u256(-7))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(0xFF99u), u256(8)), encodeArgs(u256(-1))); } BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_signextend_int32) @@ -10664,10 +10722,10 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_signextend_int32) )"; compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(0)), encodeArgs(u256(-103))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(1)), encodeArgs(u256(-51))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(2)), encodeArgs(u256(-25))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(4)), encodeArgs(u256(-6))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(8)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(1)), encodeArgs(u256(-52))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(2)), encodeArgs(u256(-26))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(4)), encodeArgs(u256(-7))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(0xFFFFFF99u), u256(8)), encodeArgs(u256(-1))); } @@ -10683,16 +10741,16 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_int16) compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(0)), encodeArgs(u256(-4266))); ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(17)), encodeArgs(u256(-1))); ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(0)), encodeArgs(u256(-4267))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(1)), encodeArgs(u256(-2134))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4267), u256(17)), encodeArgs(u256(-1))); } BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_int32) @@ -10707,16 +10765,16 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_int32) compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(0)), encodeArgs(u256(-4266))); ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4266), u256(17)), encodeArgs(u256(-1))); ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(0)), encodeArgs(u256(-4267))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(1)), encodeArgs(u256(-2134))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int32,int32)", u256(-4267), u256(17)), encodeArgs(u256(-1))); } BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_assignment) @@ -10732,16 +10790,16 @@ BOOST_AUTO_TEST_CASE(shift_right_negative_lvalue_assignment) compileAndRun(sourceCode, 0, "C"); ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(0)), encodeArgs(u256(-4266))); ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4266), u256(17)), encodeArgs(u256(-1))); ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(0)), encodeArgs(u256(-4267))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(1)), encodeArgs(u256(-2133))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(4)), encodeArgs(u256(-266))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(8)), encodeArgs(u256(-16))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(16)), encodeArgs(u256(0))); - ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(17)), encodeArgs(u256(0))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(1)), encodeArgs(u256(-2134))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(4)), encodeArgs(u256(-267))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(8)), encodeArgs(u256(-17))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(16)), encodeArgs(u256(-1))); + ABI_CHECK(callContractFunction("f(int256,int256)", u256(-4267), u256(17)), encodeArgs(u256(-1))); } BOOST_AUTO_TEST_CASE(shift_negative_rvalue) From e84b55bd6feded46789d2d398cd1b4092ef7a1c0 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Thu, 7 Jun 2018 17:08:42 +0200 Subject: [PATCH 2/2] Extend explanatory remark and argue using bitwise operations instead of rounding. --- libsolidity/codegen/ExpressionCompiler.cpp | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index 908c703ab..0470c3ec1 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -1742,12 +1742,20 @@ void ExpressionCompiler::appendShiftOperatorCode(Token::Value _operator, Type co else { if (c_valueSigned) - // For negative values xor_mask has all bits set and xor(value_to_shift, xor_mask) will be - // the bitwise complement of value_to_shift, i.e. abs(value_to_shift) - 1. Dividing this by - // exp(2, shift_amount) results in a value that is positive and strictly smaller than the - // absolute value of the desired result. Taking the complement again changes the sign - // back to negative and subtracts one, resulting in rounding towards negative infinity. - // For positive values xor_mask is zero and xor(value_to_shift, xor_mask) is again value_to_shift. + // In the following assembly snippet, xor_mask will be zero, if value_to_shift is positive. + // Therefor xor'ing with xor_mask is the identity and the computation reduces to + // div(value_to_shift, exp(2, shift_amount)), which is correct, since for positive values + // arithmetic right shift is dividing by a power of two (which, as a bitwise operation, results + // in discarding bits on the right and filling with zeros from the left). + // For negative values arithmetic right shift, viewed as a bitwise operation, discards bits to the + // right and fills in ones from the left. This is achieved as follows: + // If value_to_shift is negative, then xor_mask will have all bits set, so xor'ing with xor_mask + // will flip all bits. First all bits in value_to_shift are flipped. As for the positive case, + // dividing by a power of two using integer arithmetic results in discarding bits to the right + // and filling with zeros from the left. Flipping all bits in the result again, turns all zeros + // on the left to ones and restores the non-discarded, shifted bits to their original value (they + // have now been flipped twice). In summary we now have discarded bits to the right and filled with + // ones from the left, i.e. we have performed an arithmetic right shift. m_context.appendInlineAssembly(R"({ let xor_mask := sub(0, slt(value_to_shift, 0)) value_to_shift := xor(div(xor(value_to_shift, xor_mask), exp(2, shift_amount)), xor_mask)