Breaking: overflow detection for SDIV - minInt / (-1) #1091

pirapira · 2016-09-13T14:05:43Z

Trying to fix #1087.

First I'm trying to catch this in the CI servers.

chriseth · 2016-09-13T17:29:00Z

libsolidity/codegen/ExpressionCompiler.cpp

+		// Test for (minInt) / (-1)
+		if (c_isSigned && _operator == Token::Div)
+		{
+			m_context << Instruction::DUP1 << Instruction::NOT  // ~x


Not sure if this is faster, but you might want to use the new inline assembly here:

m_context.appendInlineAssembly(R"( and( eq(x, exp(2, 255)), // x == 2**256 (min int) iszero(not(y)) // y == -1 ) )", // names of variables already on the stack {"y", "x"}); m_context.appendConditionalJumpTo(m_context.errorTag());

Oh by the way, in all other comments of the code generator, the stack grows to the right, I think yours grows to the left.

Oh, I'll change the stack growing direction.

chriseth · 2016-09-13T17:32:12Z

Please don't miss my comment above.

ethereum#1091 (comment)

pirapira · 2016-09-13T17:38:39Z

No, no, they arrive in my mailbox.

pirapira · 2016-09-13T20:49:29Z

@chriseth The inline assembly caused an exception (possibly because the compiler tried to reduce every constant number, and hit the case?) So I reverted to the old fashioned EVM writing.

pirapira · 2016-09-13T22:04:39Z

This is done on my side.

chriseth

Please add an entry into the changelog (not for the current version, this has to go into 0.5.0) and also add some documentation ("places where solidity throws").

Note that this PR is only one part of the general "throw on arithmetic overflow" task.

chriseth · 2016-10-04T18:31:17Z

libsolidity/codegen/ExpressionCompiler.cpp

+		if (c_isSigned && _operator == Token::Div)
+		{
+			m_context << Instruction::DUP1 // x
+				<< u256(255) << u256(2) << Instruction::EXP // 2 ** 255


Better to use (u256(1) << 255), the optimizer knows how to represent that.

Does the optimizer use EXP, PUSH32 directly, or a much better way? I will do an experiment.

It tries various different ways to represent constants and compares the creation-time costs against the execution-time costs.

OK --optimize does the same thing as I do. I'll change this.

chriseth · 2016-10-04T18:33:38Z

libsolidity/codegen/ExpressionCompiler.cpp

+			m_context << Instruction::AND;
+			m_context.appendConditionalJumpTo(m_context.errorTag());
+		}
+
 		if (_operator == Token::Div)
 			m_context << (c_isSigned ? Instruction::SDIV : Instruction::DIV);
 		else


Is there also a special case for smod?

The SMOD specification is ambiguous. I created a separate issue about this.

My conclusion is that SMOD has no overflows.

pirapira · 2016-10-05T07:41:56Z

Mentioning #796 for cross-referencing.

Following a comment ethereum#1091 (comment)

pirapira · 2016-10-05T15:53:34Z

I've dealt with all comments.

axic · 2016-10-06T11:37:19Z

Can we pull out the "When Solidity Throws" change into a separate PR and merge it?

pirapira · 2016-10-06T12:29:49Z

@axic that's a good idea. I start the separation.

Following a comment ethereum#1091 (comment)

pirapira · 2016-10-06T12:42:29Z

@axic I separated #1167 and rebased this one on top of it.

Following a comment ethereum#1091 (comment)

pirapira · 2017-03-14T12:17:10Z

Tests pass finally. I'll keep rebasing.

Following a comment ethereum#1091 (comment)

axic · 2017-05-05T21:46:29Z

libsolidity/codegen/ExpressionCompiler.cpp

+		if (c_isSigned && _operator == Token::Div)
+		{
+			m_context.appendInlineAssembly(R"({
+				jumpi(invalidJumpLabel, and(eq(y, exp(2,255)), iszero(add(x, 1))))


The invalidJumpLabel is gone. This should use something like:

jumpi(safe, iszero(...)) invalid safe:

Thought not sure if the labels must be unique?

I'll create something like

invalidLabel: invalid

once somewhere.

It seems I can do something within 3 weeks.

Perhaps `main` has a special syntactic treatment

Throw when the result is not within the expressible range of signed 256 bit integer.

that happens when the smallest signed integer is divided by one.

Following a comment ethereum#1091 (comment)

chriseth · 2017-07-12T08:22:24Z

Overflow-detection at runtime is extremely complicated because of the various types we have. I propose to do only compile-time overflow checking and close this pull request.

pirapira · 2017-07-12T08:31:36Z

@chriseth do you have less number of types at compilation time?

chriseth · 2017-07-12T08:33:02Z

No, but it is not as complicated and the effects of getting something wrong are not as bad.

pirapira · 2017-07-12T08:38:36Z

@chriseth really? I think runtime checks are vastly less complicated than compilation time checks.

pirapira · 2017-07-12T08:53:23Z

@chriseth brought up a convincing argument that all different cases of the runtime overflow checks will not make it until the new intermediate language lands. So I'm closing this.

pirapira added the in progress label Sep 13, 2016

pirapira force-pushed the minint_div_negone branch from 5b01e0a to b9d6e4d Compare September 13, 2016 17:07

chriseth reviewed Sep 13, 2016
View reviewed changes

pirapira force-pushed the minint_div_negone branch from a5132bb to 1a6eef6 Compare September 13, 2016 17:37

pirapira added a commit to pirapira/solidity that referenced this pull request Sep 13, 2016

Use inline assembly suggested by @chriseth

1a6eef6

ethereum#1091 (comment)

pirapira force-pushed the minint_div_negone branch 2 times, most recently from b680363 to 5717205 Compare September 13, 2016 20:25

pirapira added the needs review label Sep 13, 2016

pirapira force-pushed the minint_div_negone branch from 5717205 to 4c4d8ab Compare September 23, 2016 09:19

pirapira changed the title ~~[WIP] minInt / (-1)~~ minInt / (-1) Sep 23, 2016

chriseth changed the title ~~minInt / (-1)~~ Breaking: overflow detection for SDIV - minInt / (-1) Oct 4, 2016

chriseth suggested changes Oct 4, 2016

View reviewed changes

pirapira force-pushed the minint_div_negone branch from 4c4d8ab to f78f199 Compare October 5, 2016 13:20

pirapira mentioned this pull request Oct 5, 2016

Can SMOD cause an overflow #1162

Closed

pirapira added a commit to pirapira/solidity that referenced this pull request Oct 5, 2016

Let the optimizer optimize

bd86274

Following a comment ethereum#1091 (comment)

pirapira mentioned this pull request Oct 6, 2016

Adding "When Solidity Throws" section in the documentation #1167

Merged

pirapira force-pushed the minint_div_negone branch from bd86274 to 5c0c3c3 Compare October 6, 2016 12:41

pirapira added a commit to pirapira/solidity that referenced this pull request Oct 6, 2016

Let the optimizer optimize

5c0c3c3

Following a comment ethereum#1091 (comment)

pirapira force-pushed the minint_div_negone branch from 5c0c3c3 to 41e2bc6 Compare October 10, 2016 20:30

pirapira added a commit to pirapira/solidity that referenced this pull request Oct 10, 2016

Let the optimizer optimize

48431a6

Following a comment ethereum#1091 (comment)

pirapira force-pushed the minint_div_negone branch from 41e2bc6 to ae8e1f0 Compare October 10, 2016 20:35

pirapira added a commit to pirapira/solidity that referenced this pull request Mar 10, 2017

Let the optimizer optimize

09803b9

Following a comment ethereum#1091 (comment)

pirapira force-pushed the minint_div_negone branch from 34b9bff to 6dd48f5 Compare March 13, 2017 17:40

pirapira added a commit to pirapira/solidity that referenced this pull request Mar 13, 2017

Let the optimizer optimize

212b8c1

Following a comment ethereum#1091 (comment)

pirapira force-pushed the minint_div_negone branch from 6dd48f5 to 71cfe63 Compare March 14, 2017 11:32

pirapira force-pushed the minint_div_negone branch from 71cfe63 to 041fc84 Compare March 22, 2017 10:17

pirapira added a commit to pirapira/solidity that referenced this pull request Mar 22, 2017

Let the optimizer optimize

3d89829

Following a comment ethereum#1091 (comment)

pirapira force-pushed the minint_div_negone branch from 041fc84 to 87b09e7 Compare April 11, 2017 13:46

pirapira added a commit to pirapira/solidity that referenced this pull request Apr 11, 2017

Let the optimizer optimize

8aa0e52

Following a comment ethereum#1091 (comment)

pirapira force-pushed the minint_div_negone branch from 87b09e7 to 8a8b29b Compare April 21, 2017 10:14

pirapira added a commit to pirapira/solidity that referenced this pull request Apr 21, 2017

Let the optimizer optimize

61bc50c

Following a comment ethereum#1091 (comment)

axic added the breaking change ⚠️ label Apr 26, 2017

axic reviewed May 5, 2017

View reviewed changes

pirapira added 10 commits June 22, 2017 12:14

Add a test case for ethereum#1087

daef8c2

Change a name of a function in the test

89fdd2d

Perhaps `main` has a special syntactic treatment

Oh, when I misspell function as funtction, a syntax error happens.

9eac048

Add a runtime check for (minInt) / (-1)

54840fb

Throw when the result is not within the expressible range of signed 256 bit integer.

Add Changelog entry about the new throw

d40c32e

that happens when the smallest signed integer is divided by one.

Let the optimizer optimize

af574eb

Following a comment ethereum#1091 (comment)

Add documentation about the new throw case

cd96df9

codegen: use 0xfe opcode for minInt/(-1)

253fec0

codegen: replace some instructions with an inline assembly

e685305

Replace C++ expression in inline assembly

2dfdfda

pirapira force-pushed the minint_div_negone branch from 8a8b29b to 2dfdfda Compare June 22, 2017 10:15

pirapira closed this Jul 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking: overflow detection for SDIV - minInt / (-1) #1091

Breaking: overflow detection for SDIV - minInt / (-1) #1091

pirapira commented Sep 13, 2016

chriseth Sep 13, 2016

pirapira Sep 13, 2016

chriseth commented Sep 13, 2016

pirapira commented Sep 13, 2016

pirapira commented Sep 13, 2016

pirapira commented Sep 13, 2016

chriseth left a comment

chriseth Oct 4, 2016

pirapira Oct 5, 2016

chriseth Oct 5, 2016

pirapira Oct 5, 2016

chriseth Oct 4, 2016

pirapira Oct 5, 2016

pirapira Oct 5, 2016

pirapira commented Oct 5, 2016

pirapira commented Oct 5, 2016

axic commented Oct 6, 2016

pirapira commented Oct 6, 2016

pirapira commented Oct 6, 2016

pirapira commented Mar 14, 2017

axic May 5, 2017

pirapira May 7, 2017

chriseth commented Jul 12, 2017

pirapira commented Jul 12, 2017

chriseth commented Jul 12, 2017

pirapira commented Jul 12, 2017

pirapira commented Jul 12, 2017

Breaking: overflow detection for SDIV - minInt / (-1) #1091

Breaking: overflow detection for SDIV - minInt / (-1) #1091

Conversation

pirapira commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Sep 13, 2016

pirapira commented Sep 13, 2016

pirapira commented Sep 13, 2016

pirapira commented Sep 13, 2016

chriseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirapira commented Oct 5, 2016

pirapira commented Oct 5, 2016

axic commented Oct 6, 2016

pirapira commented Oct 6, 2016

pirapira commented Oct 6, 2016

pirapira commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Jul 12, 2017

pirapira commented Jul 12, 2017

chriseth commented Jul 12, 2017

pirapira commented Jul 12, 2017

pirapira commented Jul 12, 2017