Change translation of implicit throws by wuestholz · Pull Request #1598 · argotorg/solidity

wuestholz · 2017-01-22T19:57:57Z

DO NOT MERGE, depensd on ethereum/EIPs#141

@chriseth @pirapira This adds a new invalid instruction that is used for encoding implicit throws that are emitted by the compiler. This makes it possible to distinguish such runtime errors from user-provided, explicit throws.

For some reason there seem to be three failing tests: SolidityEndToEndTest/function_delete_stack, SolidityEndToEndTest/call_function_returning_function, and SolidityEndToEndTest/conditional_expression_functions. The failures are due to a failing assertion in CompilerUtils::moveToStackVariable. However, it's not really clear to me why these changes should affect that assertion. It would be great if someone could have a look.

chriseth · 2017-01-23T08:35:07Z

libsolidity/codegen/CompilerContext.cpp

+
+CompilerContext& CompilerContext::appendConditionalInvalid()
+{
+	eth::AssemblyItem falseTag = appendConditionalJump();


I wounder whether it might be better to just have a single jump:
ISZERO <continue> JUMPI INVALID continue:

Your code does:
<error> JUMPI <continue> JUMP error: INVALID continue:

I intentionally tried not to be too clever about encoding the jumps. I had considered the same optimization. I have pushed this now.

chriseth · 2017-01-23T08:36:59Z

libsolidity/codegen/ContractCompiler.cpp

 	//Propagate error condition (if DELEGATECALL pushes 0 on stack).
 	a << Instruction::ISZERO;
-	a.appendJumpI(a.errorTag());
+	eth::AssemblyItem falseTag = a.appendJumpI();


Why don't you use appendConditionalInvalid() here?

It's not available for class Assembly and I didn't want to add it since it is only used here. However, I'd be happy to add it if you want.

Oh right, I'm sorry, I didn't see the full context. Is this case this is fine.

chriseth · 2017-01-23T08:38:47Z

Thanks! Please don't forget to change the documentation, too.

wuestholz · 2017-01-23T10:07:05Z

@chriseth Thank you very much for the feedback. I pushed changes that should address it.

Did you have chance to look into why those three tests might be failing now? It it possible that my changes somehow uncovered a separate issue?

chriseth · 2017-01-23T14:08:10Z

libsolidity/codegen/CompilerContext.cpp

+{
+	*this << Instruction::ISZERO;
+	eth::AssemblyItem afterTag = appendConditionalJump();
+	return *this << Instruction::INVALID << afterTag;


The code generator is not too smart about the stack height with regards to control flow, so you have to manually adjust the stack height at the end of this function (it assumes that control flow continues after invalid). This is the reason for the test failure. You can try this snippet to check whether it worked:

contract test { function x() returns (uint) { return 1; } function y() returns (uint) { return 2; } function f(bool cond) returns (uint) { var z = cond ? x : y; return z(); } }

Thanks! I tried the following:

unsigned startStackHeight = stackHeight(); *this << Instruction::ISZERO; eth::AssemblyItem afterTag = appendConditionalJump(); *this << Instruction::INVALID << afterTag; setStackOffset(startStackHeight - 1); return *this;

However, this didn't seem to do the trick. Were you thinking of something else?

chriseth · 2017-01-25T16:13:44Z

Can you allow me access to your pull request branch (this is a new github feature, I think)? Then I will make the required adjustments.

wuestholz · 2017-01-25T17:26:01Z

@chriseth Thanks! That's fine with me. I believe I checked some box about allowing this when I created the PR. I have also added you as a collaborator just in case.

axic · 2017-01-26T14:43:16Z

libevmasm/Instruction.h

 	LOG3,				///< Makes a log entry; 3 topics.
 	LOG4,				///< Makes a log entry; 4 topics.

+	INVALID = 0xef,		///< invalid instruction for expressing runtime errors (e.g., division-by-zero)


Please update to 0xfe as per the final EIP141.

chriseth · 2017-01-26T15:00:45Z

This is still not fixed, but I think we are getting closer.

chriseth · 2017-01-26T15:38:15Z

Ok, found the problem: Zero-initialized variables of internal function type have to be a tag. I found a solution above that is not the best yet, will submit a new version shortly.

This adds a new invalid instruction that is used for encoding implicit throws that are emitted by the compiler. This makes it possible to distinguish such runtime errors from user-provided, explicit throws.

chriseth · 2017-01-26T15:41:46Z

@axic / @pirapira please review.

wuestholz · 2017-01-26T18:14:50Z

@chriseth Great! Thanks for looking into this!

axic · 2017-01-26T23:50:42Z

docs/control-structures.rst


-Internally, Solidity performs an "invalid jump" when an exception is thrown and thus causes the EVM to revert all changes made to the state. The reason for this is that there is no safe way to continue execution, because an expected effect did not occur. Because we want to retain the atomicity of transactions, the safest thing to do is to revert all changes and make the whole transaction (or at least call) without effect.
+Internally, Solidity performs an "invalid jump" when a user-provided exception is thrown. In contrast, it performs an invalid operation
+(code ``0xfe``) if a runtime exception is encountered. In both cases, this causes


Maybe "performs an invalid operation (0xfe instruction)" sounds a bit clearer?

axic · 2017-01-27T00:06:33Z

libsolidity/codegen/ContractCompiler.cpp

 	a << Instruction::ISZERO;
-	a.appendJumpI(a.errorTag());
+	a << Instruction::ISZERO;
+	eth::AssemblyItem afterTag = a.appendJumpI();


Shouldn't this have a .tag()?

I guess it didn't fail because we don't have a test for it?

Yes, clone binaries are an undocumented and also mostly useless feature.

Created an issue for that (#1618) so we can track.

axic · 2017-01-27T00:09:53Z

It is missing an entry in the changelog.

chriseth · 2017-01-27T09:19:28Z

Updated.

axic · 2017-01-27T10:30:10Z

Is the idea for catch to insert tags for the jumps? If so, using invalid for non-user exceptions is the right way.

chriseth · 2017-01-27T10:49:51Z

@axic I don't think catch can really work in a consistent way (it has to roll back changes inside a single call), but if we do something like that, then throw will be changed from jump(nonExistingTag) to jump(mload(mload(0x80))) and catch will push an address onto the stack pointed to at address 0x80.

LianaHus added the needs review label Jan 22, 2017

chriseth added in progress and removed needs review labels Jan 22, 2017

wuestholz mentioned this pull request Jan 22, 2017

Distinguishing explicit and implicit throws #1589

Closed

chriseth suggested changes Jan 23, 2017

View reviewed changes

chriseth reviewed Jan 23, 2017

View reviewed changes

axic changed the title ~~Change translation of implicit throws (issue #1589).~~ Change translation of implicit throws Jan 26, 2017

axic reviewed Jan 26, 2017

View reviewed changes

wuestholz and others added 8 commits January 26, 2017 16:39

Change translation of implicit throws (issue #1589).

9bcbd93

This adds a new invalid instruction that is used for encoding implicit throws that are emitted by the compiler. This makes it possible to distinguish such runtime errors from user-provided, explicit throws.

Address feedback from code review.

5b7cc01

Change code for INVALID opcode to 0xfe.

c2b3d8b

Fix optimizer with regards to INVALID instruction.

ae2b59d

Split line.

390beba

Allow inserting low-level functions without calling them.

d9fbb83

Uninitialized internal function should call INVALID.

a98fa41

Document special case of zero-initialized internal function.

7660736

chriseth added the nextrelease label Jan 26, 2017

axic reviewed Jan 26, 2017

View reviewed changes

axic reviewed Jan 27, 2017

View reviewed changes

Changelog and review suggestions.

bff8fc2

axic approved these changes Jan 27, 2017

View reviewed changes

chriseth approved these changes Jan 27, 2017

View reviewed changes

chriseth merged commit b2c35fb into argotorg:develop Jan 27, 2017

chriseth removed in progress labels Jan 27, 2017

wuestholz mentioned this pull request Feb 16, 2017

Implement assert() #1678

Merged

Conversation

wuestholz commented Jan 22, 2017 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Jan 23, 2017

Uh oh!

wuestholz commented Jan 23, 2017

Uh oh!

chriseth Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Jan 25, 2017

Uh oh!

wuestholz commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Jan 26, 2017

Uh oh!

chriseth commented Jan 26, 2017

Uh oh!

chriseth commented Jan 26, 2017

Uh oh!

wuestholz commented Jan 26, 2017

Uh oh!

axic Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 27, 2017

Uh oh!

chriseth commented Jan 27, 2017

Uh oh!

axic commented Jan 27, 2017

Uh oh!

chriseth commented Jan 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wuestholz commented Jan 22, 2017 •

edited by axic

Loading

chriseth Jan 23, 2017 •

edited

Loading

axic Jan 26, 2017 •

edited

Loading