Checked arithmetic by default. #9465

chriseth · 2020-07-22T08:29:25Z

Depends on #9479.

TODO:

documentation
document: division by zero is always checked
wrapping arithmetics for Sol -> Yul
disallow -x for unsigned x outside of "unchecked" - or do we want to disallow it in general? 0-x is still OK, I would say.
decide which error to report and change all errors.

libsolidity/codegen/CompilerContext.h

libsolidity/codegen/ContractCompiler.cpp

libsolidity/ast/AST.h

stackenbotten · 2020-07-22T13:53:06Z

There was an error when running chk_coding_style for commit d4651d289c3a790a5a8bb0d9b1818762e9a06788:

Error: Trailing whitespace found:
 libsolidity/ast/AST.h:1341: Statement(_id, _location, _docString),

Please check that your changes are working as intended.

stackenbotten · 2020-07-22T13:53:09Z

There was an error when running chk_coding_style for commit 84ac4987282254e1782946313163dcf202c6d5e7:

Error: Trailing whitespace found:
 libsolidity/ast/AST.h:1341: Statement(_id, _location, _docString),

Please check that your changes are working as intended.

stackenbotten · 2020-07-23T16:45:14Z

There was an error when running chk_coding_style for commit 44e18bc91be7005599bed9f3c05118fb37082fd2:

Error: Trailing whitespace found:
 libsolidity/ast/AST.h:1341: Statement(_id, _location, _docString),

Please check that your changes are working as intended.

leonardoalt · 2020-07-23T17:23:23Z

Rebase problems?

axic · 2020-08-05T21:59:27Z

This still needs to be rebased and perhaps need to merge develop into breaking too. At the same time this seems to depend on #9479, so I'd suggest turning this to draft until that is merged.

stackenbotten · 2020-08-06T14:57:25Z

There was an error when running chk_coding_style for commit b3674152aa040dd440da1143a07ce8522eeda451:

Error: Trailing whitespace found:
 libsolidity/ast/AST.h:1341: Statement(_id, _location, _docString),

Please check that your changes are working as intended.

chriseth · 2020-08-06T14:59:24Z

Rebased and changed target to breaking_8 (@axic requested to have one breaking branch per release at some point)

stackenbotten · 2020-09-14T18:46:54Z

There was an error when running chk_coding_style for commit c24cc01f404d3796f672cdbcd89211dac987f18c:

Error: Trailing whitespace found:
 libsolidity/ast/AST.h:1347: Statement(_id, _location, _docString),

Please check that your changes are working as intended.

leonardoalt · 2020-10-14T16:06:00Z

docs/grammar/Solidity.g4

+block:
+	LBrace ( statement | uncheckedBlock )* RBrace;
+
+uncheckedBlock: Unchecked block;


This allows for nested unchecked blocks.

Yes, it is not a parser but a syntax checker restriction.

Is if (<sth>) unchecked { x; } invalid? (I'll probably know when I'm done reviewing...)

Yeah, apparently it isn't - not sure if it couldn't be valid, but fine!

leonardoalt · 2020-10-14T16:08:51Z

docs/types/value-types.rst

+  There are two modes in which arithmetic is performed on these types: The "wrapping" or "unchecked" mode and the "checked" mode.
+  By default, arithmetic is always "checked", which mean that if the result of an operation falls outside the value range
+  of the type, the call is reverted through a :ref:`failing assertion<assert-and-require>`. You can switch to "unchecked" mode
+  using ``unchecked { ... }``. More details can be found in the section about <unchecked>.


Is <unchecked> at the end an actual link to the section?

nope, it's not. Will fix.

leonardoalt · 2020-10-14T16:09:19Z

docs/types/value-types.rst

+modes in regard to over- and underflow:
+
+By default, all arithmetic is checked for under- or overflow, but this can be disabled
+using the <unchecked> block, resulting in wrapping arithmetic. More details


Suggested change

using the <unchecked> block, resulting in wrapping arithmetic. More details

using the <unchecked> block, resulting in modular arithmetic. More details

Is <unchecked> a link?

Fixed the link. Rust also uses the term "wrapping": https://doc.rust-lang.org/std/primitive.u32.html#method.wrapping_add

Maybe "wrapping" is also less intimidating than "modular"?

leonardoalt · 2020-10-14T16:17:25Z

libsolidity/codegen/CompilerContext.h

@@ -380,6 +384,8 @@ class CompilerContext
 	std::map<Declaration const*, std::vector<unsigned>> m_localVariables;
 	/// The contract currently being compiled. Virtual function lookup starts from this contarct.
 	ContractDefinition const* m_mostDerivedContract = nullptr;
+	/// Whether to use checked arithmetic.
+	std::stack<Arithmetic> m_arithmetic;


It needs to be a stack because you can call a function inside an unchecked block and then open another unchecked block inside that function, but the rest of that function is checked. Correct?

It actually does not need to be a stack anymore, I think.

ekpyron

I haven't seen anything that really bothered me in the first pass, but I wasn't extremely careful about it, so I'm a bit hesitant approving just yet...

ekpyron · 2020-10-15T15:04:34Z

docs/control-structures.rst

+An overflow or underflow is the situation where the resulting value of an arithmetic operation,
+when executed on an unrestricted integer, falls outside the range of the result type.


This seems a bit out of place to me here... this should probably be the second paragraph of the section at the very top?

ekpyron · 2020-10-15T15:07:25Z

docs/grammar/Solidity.g4

+block:
+	LBrace ( statement | uncheckedBlock )* RBrace;
+
+uncheckedBlock: Unchecked block;


Is if (<sth>) unchecked { x; } invalid? (I'll probably know when I'm done reviewing...)

ekpyron · 2020-10-15T15:20:44Z

docs/grammar/Solidity.g4

+block:
+	LBrace ( statement | uncheckedBlock )* RBrace;
+
+uncheckedBlock: Unchecked block;


Yeah, apparently it isn't - not sure if it couldn't be valid, but fine!

ekpyron · 2020-10-15T16:07:42Z

libsolidity/codegen/ExpressionCompiler.cpp

@@ -460,7 +479,7 @@ bool ExpressionCompiler::visit(BinaryOperation const& _binaryOperation)
 		m_context << commonType->literalValue(nullptr);
 	else
 	{
-		bool cleanupNeeded = cleanupNeededForOp(commonType->category(), c_op);
+		bool cleanupNeeded = m_context.arithmetic() == Arithmetic::Checked || cleanupNeededForOp(commonType->category(), c_op);


Do we actually need cleanup here or aren't we doing it twice then? The checked arithmetic yul functions do the cleanup themselves, don't they?

chriseth · 2020-10-15T17:00:29Z

if (<sth>) unchecked { x; } is invalid currently. It was just looking weird to me. We anyway need a special case unless we also want
function f() public unchecked { x; } to be valid.

ekpyron · 2020-10-15T17:01:34Z

if (<sth>) unchecked { x; } is invalid currently. It was just looking weird to me. We anyway need a special case unless we also want
function f() public unchecked { x; } to be valid.

It's fine, I was just wondering when I saw the grammar - disallowing it now is definitely good - we can still allow it later anyways, while not the other way!

leonardoalt

A few small comments.

It would be nice to also have a few more corner case tests:

unchecked function called by checked function called by unchecked function
checked function called by unchecked function
is it possible to have contract C { uint x = unchecked { ... }; } ?

leonardoalt · 2020-10-16T09:32:45Z

libsolidity/codegen/ExpressionCompiler.cpp

@@ -275,7 +275,7 @@ bool ExpressionCompiler::visit(Assignment const& _assignment)
 		solAssert(*_assignment.annotation().type == leftType, "");
 	bool cleanupNeeded = false;
 	if (op != Token::Assign)
-		cleanupNeeded = cleanupNeededForOp(leftType.category(), binOp);
+		cleanupNeeded = m_context.arithmetic() == Arithmetic::Checked || cleanupNeededForOp(leftType.category(), binOp);


Is this just extra safety?

Same as #9465 (comment) - since the arithmetic functions in yul already do the cleanup, we should probably not clean up before them.

leonardoalt · 2020-10-16T09:38:27Z

libsolidity/codegen/YulUtilFunctions.cpp

+	});
+}
+
+


Extra blank

leonardoalt · 2020-10-16T09:42:57Z

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

 				break;
 			case Token::Mod:
-				fun = m_utils.checkedIntModFunction(*type);
+				fun = m_utils.intModFunction(*type);


What about exp?

It branches out earlier in visit(BinaryOperation const& _binOp)

leonardoalt · 2020-10-16T09:44:17Z

test/libsolidity/SolidityParser.cpp

 	BOOST_CHECK(TokenTraits::isReservedKeyword(Token::Var));
+	BOOST_CHECK(TokenTraits::isReservedKeyword(Token::Reference));


Where did Reference come from?

IIUC the test verifies that isReservedKeyword is correctly true for the first and the last reserved keyword - the last one used to be unchecked, but then we made var reserved again and added it as new last one here, but just kept unchecked, so since then the idea is "check first and last reserved keyword and a random one in between" - the random one in between can't be unchecked anymore, so now it's reference...

leonardoalt · 2020-10-19T13:47:56Z

Actually some commits need to be squashed.

leonardoalt · 2020-10-19T15:38:42Z

Looks good. What about decide which error to report and change all errors.?

chriseth · 2020-10-19T15:39:35Z

Will be done in #10013

chriseth force-pushed the unchecked branch from ed72489 to fc31877 Compare July 22, 2020 08:37

leonardoalt reviewed Jul 22, 2020

View reviewed changes

libsolidity/codegen/CompilerContext.h Outdated Show resolved Hide resolved

libsolidity/codegen/ContractCompiler.cpp Outdated Show resolved Hide resolved

libsolidity/codegen/ContractCompiler.cpp Outdated Show resolved Hide resolved

mijovic reviewed Jul 22, 2020

View reviewed changes

libsolidity/ast/AST.h Outdated Show resolved Hide resolved

chriseth force-pushed the unchecked branch from d4651d2 to 44e18bc Compare July 23, 2020 16:44

axic marked this pull request as draft August 5, 2020 22:00

chriseth force-pushed the unchecked branch from 44e18bc to b367415 Compare August 6, 2020 14:56

chriseth changed the base branch from breaking to breaking_8 August 6, 2020 14:58

chriseth force-pushed the unchecked branch from b367415 to c24cc01 Compare September 14, 2020 18:46

chriseth changed the base branch from breaking_8 to breaking September 14, 2020 18:46

chriseth force-pushed the unchecked branch 8 times, most recently from 38be2ad to e7e6212 Compare September 16, 2020 15:27

chriseth force-pushed the unchecked branch 5 times, most recently from 8450cc3 to 2b3900d Compare September 24, 2020 14:11

chriseth force-pushed the unchecked branch 3 times, most recently from 709edb0 to a991b2f Compare October 14, 2020 15:52

leonardoalt reviewed Oct 14, 2020

View reviewed changes

chriseth force-pushed the unchecked branch from 4d1b441 to 71723aa Compare October 15, 2020 14:40

ekpyron reviewed Oct 15, 2020

View reviewed changes

chriseth force-pushed the unchecked branch from 324e097 to 1f6b173 Compare October 15, 2020 17:06

leonardoalt reviewed Oct 16, 2020

View reviewed changes

leonardoalt approved these changes Oct 19, 2020

View reviewed changes

chriseth added 5 commits October 19, 2020 16:58

Checked arithmetic by default.

527c073

Grammar for unchecked.

33d8b62

Update existing tests.

e61fa59

Documentation.

e262f47

New tests.

c9ef727

chriseth force-pushed the unchecked branch from 37fc84b to c9ef727 Compare October 19, 2020 14:59

leonardoalt approved these changes Oct 19, 2020

View reviewed changes

chriseth merged commit 33a5fbf into breaking Oct 19, 2020

chriseth deleted the unchecked branch October 19, 2020 15:58

chriseth mentioned this pull request Oct 20, 2020

Exception on overflow #796

Closed

axic mentioned this pull request Oct 30, 2020

Investigate feasibility of overflow checks #8467

Closed

code423n4 mentioned this pull request Jun 24, 2022

Gas Optimizations code-423n4/2022-06-nibbl-findings#221

Open

	using the <unchecked> block, resulting in wrapping arithmetic. More details
	using the <unchecked> block, resulting in modular arithmetic. More details

		An overflow or underflow is the situation where the resulting value of an arithmetic operation,
		when executed on an unrestricted integer, falls outside the range of the result type.

		BOOST_CHECK(TokenTraits::isReservedKeyword(Token::Var));
		BOOST_CHECK(TokenTraits::isReservedKeyword(Token::Reference));

Checked arithmetic by default. #9465

Checked arithmetic by default. #9465

Conversation

chriseth commented Jul 22, 2020 • edited Loading

stackenbotten commented Jul 22, 2020

stackenbotten commented Jul 22, 2020

stackenbotten commented Jul 23, 2020

leonardoalt commented Jul 23, 2020

axic commented Aug 5, 2020

stackenbotten commented Aug 6, 2020

chriseth commented Aug 6, 2020

stackenbotten commented Sep 14, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron 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

chriseth commented Oct 15, 2020

ekpyron commented Oct 15, 2020

leonardoalt 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

leonardoalt commented Oct 19, 2020

leonardoalt commented Oct 19, 2020

chriseth commented Oct 19, 2020

chriseth commented Jul 22, 2020 •

edited

Loading