Fixes u256 overflow in logical shift optimization rule and adds tests. #6248

bshastry · 2019-03-12T13:24:36Z

Description

(closes #6246 )

This PR fixes incorrect shift optimization due to u256 overflow in the addition of shift sizes.

Checklist

Code compiles correctly
All tests are passing
New tests have been created which fail without the change (if possible)
README / documentation was extended, if necessary
Changelog entry (if change is visible to the user)
Used meaningful commit messages

axic · 2019-03-12T13:30:20Z

libevmasm/RuleList.h

@@ -46,6 +46,11 @@ template <class S> S modWorkaround(S const& _a, S const& _b)
 	return (S)(bigint(_a) % bigint(_b));
 }

+template <class S> S minOfu256Add(S const& _a, S const& _b, S const& _c)


Please move to libdevcore/CommonData.h after bigintShiftLeftWorkaround.

Are you sure that this makes a nice helper function? It's rather cryptic - I don't think it's apparent what it does without looking at the implementation. And it's not like we're working around a bug in boost here or something like that...

I also think it should just be inlined.

axic

Please add a changelog and a buglist entry.

Also include it in the end to end tests (where the other shift combinations are).

chriseth · 2019-03-12T14:05:55Z

It's currently not possible to test this in the end to end tests. I will fix that.

chriseth · 2019-03-12T14:12:08Z

Ah sorry, I confused this with the peephole optimizer. Of course it is possible to test these via the execution framework.

docs/bugs.json

axic · 2019-03-12T15:06:55Z

test/libsolidity/SolidityEndToEndTest.cpp

@@ -15109,11 +15109,21 @@ BOOST_AUTO_TEST_CASE(bitwise_shifting_constantinople_combined)
 					c := shl(0xd0, shl(0x40, a))
 				}
 			}
+			function shl_combined_overflow(uint a) public returns (uint c) {
+				assembly {
+					c := shl(0x01, shl(not(0x00), a))


I don't think there is a need for these, just supply a large value in callContractFunction below.

I don't understand - it only works for constants. Supply a large value where?

I thought it has two inputs, sorry.

codecov · 2019-03-12T15:08:57Z

Codecov Report

Merging #6248 into develop will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #6248      +/-   ##
===========================================
- Coverage    26.76%   26.76%   -0.01%     
===========================================
  Files          381      381              
  Lines        36573    36577       +4     
  Branches      4336     4336              
===========================================
  Hits          9789     9789              
- Misses       26115    26119       +4     
  Partials       669      669

Flag	Coverage Δ
#syntax	`26.76% <0%> (-0.01%)`	⬇️

codecov · 2019-03-12T15:43:14Z

Codecov Report

Merging #6248 into develop will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #6248      +/-   ##
===========================================
- Coverage    26.76%   26.76%   -0.01%     
===========================================
  Files          381      381              
  Lines        36573    36577       +4     
  Branches      4336     4336              
===========================================
  Hits          9789     9789              
- Misses       26115    26119       +4     
  Partials       669      669

Flag	Coverage Δ
#syntax	`26.76% <0%> (-0.01%)`	⬇️

axic · 2019-03-12T15:47:58Z

docs/bugs.json

+        "name": "DoubleShiftSizeOverflow",
+        "summary": "Double bitwise shifts by large constants whose sum overflows 256 bits can result in unexpected values.",
+        "description": "Nested logical shift operations whose total shift size is 2**256 or more are incorrectly optimized. This only applies to shifts by numbers of bits that are compile-time constant expressions.",
+	    "introduced": "0.5.5",


Indentation?

Weird, I thought I fixed it...

Changelog.md

bshastry

I think the yulOptimizer tests may fail on homestead

test/libyul/yulOptimizerTests/expressionSimplifier/shift_left_overflow.yul

test/libyul/yulOptimizerTests/expressionSimplifier/shift_right_overflow.yul

codecov · 2019-03-12T16:13:24Z

Codecov Report

Merging #6248 into develop will increase coverage by <.01%.
The diff coverage is 91.66%.

@@             Coverage Diff             @@
##           develop    #6248      +/-   ##
===========================================
+ Coverage    87.96%   87.96%   +<.01%     
===========================================
  Files          381      381              
  Lines        36779    36801      +22     
  Branches      4336     4340       +4     
===========================================
+ Hits         32353    32373      +20     
  Misses        2952     2952              
- Partials      1474     1476       +2

Flag	Coverage Δ
#all	`87.96% <91.66%> (ø)`	⬆️
#syntax	`26.75% <8.33%> (-0.02%)`	⬇️

chriseth · 2019-03-12T21:01:40Z

Changed severity to "low". The situation is comparable to ConstantOptimizerSubtraction.

libevmasm/RuleList.h

chriseth · 2019-03-13T10:19:46Z

Updated.

ekpyron

I hope I'm not missing something else, but I think it's fine now :-).

bshastry requested a review from axic March 12, 2019 13:24

axic reviewed Mar 12, 2019

View reviewed changes

axic requested changes Mar 12, 2019

View reviewed changes

axic reviewed Mar 12, 2019

View reviewed changes

docs/bugs.json Show resolved Hide resolved

axic reviewed Mar 12, 2019

View reviewed changes

bshastry force-pushed the shiftopt-fix-overflow branch from e0000f9 to 3b55767 Compare March 12, 2019 15:07

chriseth force-pushed the shiftopt-fix-overflow branch 3 times, most recently from 21b7a96 to fa693e2 Compare March 12, 2019 15:43

axic reviewed Mar 12, 2019

View reviewed changes

chriseth reviewed Mar 12, 2019

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

chriseth force-pushed the shiftopt-fix-overflow branch from fa693e2 to 2e46df7 Compare March 12, 2019 15:52

bshastry commented Mar 12, 2019

View reviewed changes

test/libyul/yulOptimizerTests/expressionSimplifier/shift_left_overflow.yul Outdated Show resolved Hide resolved

test/libyul/yulOptimizerTests/expressionSimplifier/shift_right_overflow.yul Outdated Show resolved Hide resolved

chriseth force-pushed the shiftopt-fix-overflow branch from 2e46df7 to 34c6cbe Compare March 12, 2019 16:13

chriseth force-pushed the shiftopt-fix-overflow branch from 34c6cbe to 7b5f784 Compare March 12, 2019 21:01

chriseth force-pushed the shiftopt-fix-overflow branch from 7b5f784 to d725b0f Compare March 12, 2019 21:04

ekpyron requested changes Mar 12, 2019

View reviewed changes

libevmasm/RuleList.h Outdated Show resolved Hide resolved

Fixes u256 overflow in logical shift optimization rule and adds tests.

515fa87

chriseth force-pushed the shiftopt-fix-overflow branch from d725b0f to 515fa87 Compare March 13, 2019 10:19

ekpyron approved these changes Mar 13, 2019

View reviewed changes

chriseth merged commit 58a3f3c into develop Mar 13, 2019

axic deleted the shiftopt-fix-overflow branch March 13, 2019 11:23

Fixes u256 overflow in logical shift optimization rule and adds tests. #6248

Fixes u256 overflow in logical shift optimization rule and adds tests. #6248

Uh oh!

Conversation

bshastry commented Mar 12, 2019

Description

Checklist

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 left a comment

Choose a reason for hiding this comment

Uh oh!

chriseth commented Mar 12, 2019

Uh oh!

chriseth commented Mar 12, 2019

Uh oh!

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!

codecov bot commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bshastry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chriseth commented Mar 12, 2019

Uh oh!

Uh oh!

chriseth commented Mar 13, 2019

Uh oh!

ekpyron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 12, 2019 •

edited

Loading

codecov bot commented Mar 12, 2019 •

edited

Loading

codecov bot commented Mar 12, 2019 •

edited

Loading