[BREAKING] Use proper SAR for signed right shifts and emulate on pre-constantinople. by ekpyron · Pull Request #4084 · argotorg/solidity

ekpyron · 2018-05-07T17:12:54Z

Closes #3847.

chriseth · 2018-05-08T06:04:50Z

Changelog.md

 ### 0.5.0 (unreleased)

 Features:
+ * General: Signed right shift uses proper arithmetic shift, i.e. rounding towards negative infinity.


For 0.4.0 we had a separate section with breaking changes, I think we should do that again.

chriseth · 2018-05-08T06:07:16Z

libsolidity/codegen/ExpressionCompiler.cpp

+			if (c_valueSigned)
+			{
+				// stack: value_to_shift shift_amount
+				m_context << u256(0) << Instruction::DUP3 << Instruction::SLT;


Can you use inline assembly for this?

Actually, this can be done as a pragma 050, should we do that?

Ok - I didn't use inline assembly so far, because this way it's clearer to me that the amount of overhead introduced is not too excessive. But I can change it now.

Regarding a pragma 050, I'm not sure - this change does not only change some warnings to errors and it isn't merely a breaking syntactic change, but it will actually semantically change existing code. Did we do that before with a pragma 050? If so I can do it here as well - otherwise it may make sense to treat this one differently.

The overhead in generated code should not be too large and the optimizer should reduce it to exactly the same code, but please test that. The main reason why I want to push inline assembly is because it makes the transition to webassembly easier.

We can discuss on wednesday whether we want 050 for that.

Not sure whether people are more likely to read Changelogs for 0.5.0 than for earlier releases - the problem with this change is that it changes semantics, but is absolutely silent. And I could actually imagine people using signed right shifts, while expecting to end up at zero at some point... An option would be to instead introduce a temporary warning about all signed right shifts before 0.5.0 (and maybe even for pragma 050 and only remove it with actual 0.5.0) - but a warning might be too strong for that... What do you think?

Ah OK - sorry, I missed your last comment.

chriseth

Please change it to using inline assembly.

ekpyron · 2018-05-08T07:59:20Z

@chriseth
Uses inline assembly now.
I had a look at the optimized EVM assembly output and at the gas estimates. The gas estimates are identical for the assembly and the non-assembly versions and the generated code is slightly different (as one would expect), but looks fine.

ekpyron · 2018-05-08T08:00:32Z

Note: still to be discussed: should this be a pragma 050 change or not?

chriseth · 2018-05-08T08:02:52Z

libsolidity/codegen/ExpressionCompiler.cpp

+		{
+			if (c_valueSigned)
+				m_context.appendInlineAssembly(R"({
+					let xor_mask := sub(0,slt(value_to_shift,0))


Please add a space after each comma :)

Can we assume the value to be clean, or do we have to call signextend before the check? Since we previously only used SHR, it should be fine, I think.

I think it should be fine, but I can try to find some test cases for that and/or read through the code to see whether the assumption is valid. I think I'll also add a few more test cases for more odd and even, signed and unsigned values of different bit widths in general.

Added some test cases that verify that signextend is not needed.

chriseth · 2018-05-08T08:07:29Z

test/libsolidity/SolidityEndToEndTest.cpp

+			}
+		)";
+	compileAndRun(sourceCode, 0, "C");
+	ABI_CHECK(callContractFunction("f(int16,int16)", u256(-4266), u256(0)), encodeArgs(u256(-4266)));


Would it be better to use hex here?

boost::multiprecision's doc states signed to unsigned conversions extract the last MaxBits bits of the 2's complement representation of the input value, so negative decimals are fine. To me hex values with a negative sign look weird and using the 2's complement might be harder to understand (especially with rounding towards negative infinity).

The existing tests also already used a mixture of hex and dec values, so if we were to agree that hex is better, then we should probably change it everywhere.

Updated

axic · 2018-05-08T21:35:02Z

@ekpyron do you want to pull out the test cases? Those can be merged now (with the current values).

ekpyron · 2018-05-09T11:19:59Z

Note: Needs to be rebased once the tests in #4102 are merged into develop and develop is merged into 050.

ekpyron · 2018-05-14T12:21:08Z

Rebased.

chriseth · 2018-05-14T17:13:54Z

Changelog.md

 ### 0.5.0 (unreleased)

+Breaking Changes:
+ * General: Signed right shift uses proper arithmetic shift, i.e. rounding towards negative infinity.


Can you please better highlight that this is a semantics change that might go unnoticed?

chriseth · 2018-05-14T17:17:18Z

docs/types.rst

-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).


We should probably say something about literals. -1>>2 will result in -1/2 and thus int(-1>>1) will be different from int(-1) >> 1, right?

There is also a comment in Types.cpp:1057 (BinaryOperatorResult of RationalNumberType).

I hadn't thought about literals actually - I think they need to be reworked as well.
Looking at Types.cpp:1060-1077 it looks like -1>>1 will actually perform logical shift and not arithmetic shift and result in 0 right now. Similarly -1>>2, etc.

The question is: should -1>>1 be -1/2 or should it be -1?

It should be consistent with int(-1) >> 1 I would say. @axic what do you think?

I would agree that -1>>1 should be consistent with int(-1) >> 1 and therefore result in -1. Fractional operands are disallowed anyways and one can always use -1/(2^1) (resp. -1/(2^c) instead of -1>>c, where c is a positive literal) instead, if one wants fractional results.

In any case I will add test cases for right shifting negative literals - they were apparently missing or haven't been updated.

ekpyron · 2018-05-14T18:03:17Z

libsolidity/ast/Types.cpp

+					value = m_value.numerator() < 0 ? -1 : 0;
 				else
-					value = rational(m_value.numerator() / mp::pow(bigint(2), exponent), 1);
+					value = rational(m_value.numerator() >> exponent, 1);


It looks like boost can actually perform arithmetic right shift on bigint, but I'm still looking for a mention of that in the boost docs and haven't found anything so far...

Ok - I think this is undefined in boost, so I will have to change away from this.

chriseth · 2018-05-28T16:04:46Z

test/libsolidity/SolidityEndToEndTest.cpp

+	char const* sourceCode = R"(
+			contract C {
+				function f1() pure returns (int) {
+					return -4266 >> 0;


Can you change these tests to move the expectations closer to the test case, perhaps using return (-4266 >> 0 == -4266)?

chriseth · 2018-05-28T16:05:14Z

@axic @leonardoalt can you please review the shifting routines?

axic

This in general looks good to me.

chriseth · 2018-06-07T13:09:22Z

libsolidity/codegen/ExpressionCompiler.cpp

+				// 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)


By the way, something that might also work is (we could check if it is cheaper):

mstore(0x20, value) signextend(mload(sub(0x20, shift_amount)), shift_amount)

signextend uses byte offsets, not bit offsets, doesn't it?

Not sure how I could get a shift that's not a multiple of 8 this way...

Oh you are right! I forgot about that!

ekpyron · 2018-06-07T15:10:02Z

@chriseth Do you think the comment in ExpressionCompiler.cpp works now?

axic · 2018-06-11T10:12:38Z

I am still fine merging this. The code can be improved/changed, if needed, at a later time.

axic · 2018-06-11T20:38:06Z

This is in conflict now. Will rebase.

axic · 2018-06-11T20:40:13Z

Rebased. @chriseth are we ok to merge this?

axic · 2018-06-11T23:11:21Z

libsolidity/codegen/ExpressionCompiler.cpp

-		if (m_context.evmVersion().hasBitwiseShifting() && !c_valueSigned)
-			m_context << Instruction::SHR;
+		if (m_context.evmVersion().hasBitwiseShifting())
+			m_context << Instruction::SAR;


Hm, I think this is broken. This should check for c_valueSigned, otherwise right shifting an unsigned value with the top bit set will be wrong.

Oh, wow - you're right! I was so focused on the emulation part that I missed this entirely! I fixed it and added another test case that would have caught this mistake.

Fixed

…ple.

…of rounding.

ekpyron self-assigned this May 7, 2018

ekpyron changed the title ~~Use proper SAR for signed right shifts; emulate on pre-constantinopel.~~ [BREAKING] Use proper SAR for signed right shifts; emulate on pre-constantinopel. May 7, 2018

chriseth reviewed May 8, 2018

View reviewed changes

chriseth previously requested changes May 8, 2018

View reviewed changes

ekpyron force-pushed the signedRightShift branch 2 times, most recently from f874af2 to dd0205a Compare May 8, 2018 07:55

chriseth reviewed May 8, 2018

View reviewed changes

ekpyron force-pushed the signedRightShift branch from dd0205a to 0dac962 Compare May 8, 2018 08:04

chriseth reviewed May 8, 2018

View reviewed changes

ekpyron force-pushed the signedRightShift branch 3 times, most recently from d747897 to ea37ddc Compare May 8, 2018 09:41

ekpyron mentioned this pull request May 9, 2018

Add more tests for right shifts. #4102

Merged

chriseth force-pushed the 050 branch from dc857fe to 40cc63c Compare May 14, 2018 12:04

ekpyron force-pushed the signedRightShift branch from ea37ddc to f4a0a96 Compare May 14, 2018 12:20

chriseth reviewed May 14, 2018

View reviewed changes

ekpyron force-pushed the signedRightShift branch from f4a0a96 to 934dbdb Compare May 14, 2018 17:56

ekpyron commented May 14, 2018

View reviewed changes

ekpyron force-pushed the signedRightShift branch 2 times, most recently from 65a39d6 to fb4ec8d Compare May 14, 2018 18:42

chriseth force-pushed the 050 branch 2 times, most recently from 2d1ffcf to f98c8fa Compare May 15, 2018 13:08

ekpyron force-pushed the signedRightShift branch from a86ccd1 to 00bc38c Compare May 23, 2018 17:59

chriseth reviewed May 28, 2018

View reviewed changes

ekpyron force-pushed the signedRightShift branch 2 times, most recently from af01d6b to 9080a13 Compare May 28, 2018 18:32

axic changed the title ~~[BREAKING] Use proper SAR for signed right shifts; emulate on pre-constantinopel.~~ [BREAKING] Use proper SAR for signed right shifts; emulate on pre-constantinople. May 30, 2018

axic previously approved these changes May 30, 2018

View reviewed changes

ekpyron dismissed axic’s stale review via a31386d June 5, 2018 18:17

ekpyron force-pushed the signedRightShift branch from 9080a13 to a31386d Compare June 5, 2018 18:17

ekpyron requested review from erak and leonardoalt June 7, 2018 08:09

chriseth reviewed Jun 7, 2018

View reviewed changes

ekpyron force-pushed the signedRightShift branch from a31386d to 782f2b2 Compare June 7, 2018 14:46

axic previously approved these changes Jun 11, 2018

View reviewed changes

axic dismissed their stale review via 69bbf2f June 11, 2018 20:40

axic force-pushed the signedRightShift branch from a249e07 to 69bbf2f Compare June 11, 2018 20:40

ekpyron force-pushed the signedRightShift branch from 69bbf2f to a9cb89c Compare June 11, 2018 23:02

axic previously requested changes Jun 11, 2018

View reviewed changes

ekpyron added 2 commits June 12, 2018 09:32

Use proper SAR for signed right shifts and emulate on pre-constantino…

f33dc92

…ple.

Extend explanatory remark and argue using bitwise operations instead …

e84b55b

…of rounding.

axic force-pushed the signedRightShift branch from a2e479e to e84b55b Compare June 12, 2018 08:32

axic approved these changes Jun 12, 2018

View reviewed changes

axic changed the title ~~[BREAKING] Use proper SAR for signed right shifts; emulate on pre-constantinople.~~ [BREAKING] Use proper SAR for signed right shifts and emulate on pre-constantinople. Jun 12, 2018

axic merged commit ae2b589 into develop Jun 12, 2018

axic deleted the signedRightShift branch June 12, 2018 09:01

Conversation

ekpyron commented May 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

ekpyron commented May 8, 2018

Uh oh!

ekpyron commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented May 8, 2018

Uh oh!

ekpyron commented May 9, 2018

Uh oh!

ekpyron commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented May 28, 2018

Uh oh!

axic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron commented Jun 7, 2018

Uh oh!

axic commented Jun 11, 2018

Uh oh!

axic commented Jun 11, 2018

Uh oh!

axic commented Jun 11, 2018