From 7a9ee69e986cf58c8b2a6cabec2c59b0eb2fbb57 Mon Sep 17 00:00:00 2001 From: chriseth Date: Sat, 30 Dec 2017 20:13:41 +0100 Subject: [PATCH] Bubble up error messages. --- libsolidity/codegen/CompilerContext.cpp | 18 ++++++++--- libsolidity/codegen/CompilerContext.h | 5 +-- libsolidity/codegen/CompilerUtils.cpp | 1 + libsolidity/codegen/ContractCompiler.cpp | 2 ++ libsolidity/codegen/ExpressionCompiler.cpp | 9 ++++-- test/libsolidity/SolidityEndToEndTest.cpp | 37 ++++++++++++++++++++++ 6 files changed, 62 insertions(+), 10 deletions(-) diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp index 473330466..2cd256f3d 100644 --- a/libsolidity/codegen/CompilerContext.cpp +++ b/libsolidity/codegen/CompilerContext.cpp @@ -262,12 +262,20 @@ CompilerContext& CompilerContext::appendRevert() return *this << u256(0) << u256(0) << Instruction::REVERT; } -CompilerContext& CompilerContext::appendConditionalRevert() +CompilerContext& CompilerContext::appendConditionalRevert(bool _forwardReturnData) { - *this << Instruction::ISZERO; - eth::AssemblyItem afterTag = appendConditionalJump(); - appendRevert(); - *this << afterTag; + if (_forwardReturnData) + appendInlineAssembly(R"({ + if condition { + returndatacopy(0, 0, returndatasize()) + revert(0, returndatasize()) + } + })", {"condition"}); + else + appendInlineAssembly(R"({ + if condition { revert(0, 0) } + })", {"condition"}); + *this << Instruction::POP; return *this; } diff --git a/libsolidity/codegen/CompilerContext.h b/libsolidity/codegen/CompilerContext.h index 7b6632774..c6f2f3be5 100644 --- a/libsolidity/codegen/CompilerContext.h +++ b/libsolidity/codegen/CompilerContext.h @@ -156,8 +156,9 @@ public: CompilerContext& appendConditionalInvalid(); /// Appends a REVERT(0, 0) call CompilerContext& appendRevert(); - /// Appends a conditional REVERT(0, 0) call - CompilerContext& appendConditionalRevert(); + /// Appends a conditional REVERT-call, either forwarding the RETURNDATA or providing the + /// empty string. Consumes the condition. + CompilerContext& appendConditionalRevert(bool _forwardReturnData = false); /// Appends a JUMP to a specific tag CompilerContext& appendJumpTo(eth::AssemblyItem const& _tag) { m_asm->appendJump(_tag); return *this; } /// Appends pushing of a new tag and @returns the new tag. diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index 79aef7b06..34337d7d7 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -691,6 +691,7 @@ void CompilerUtils::convertType( solAssert(enumType.numberOfMembers() > 0, "empty enum should have caused a parser error."); m_context << u256(enumType.numberOfMembers() - 1) << Instruction::DUP2 << Instruction::GT; if (_asPartOfArgumentDecoding) + // TODO: error message? m_context.appendConditionalRevert(); else m_context.appendConditionalInvalid(); diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index 5cb371035..0889ac7ce 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -128,6 +128,7 @@ void ContractCompiler::appendCallValueCheck() { // Throw if function is not payable but call contained ether. m_context << Instruction::CALLVALUE; + // TODO: error message? m_context.appendConditionalRevert(); } @@ -327,6 +328,7 @@ void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contrac m_context << Instruction::STOP; } else + // TODO: error message here? m_context.appendRevert(); for (auto const& it: interfaceFunctions) diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index cb92b030b..3f521f2d6 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -608,7 +608,8 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) m_context << Instruction::CREATE; // Check if zero (out of stack or not enough balance). m_context << Instruction::DUP1 << Instruction::ISZERO; - m_context.appendConditionalRevert(); + // TODO: Can we bubble up here? There might be different reasons for failure, I think. + m_context.appendConditionalRevert(true); if (function.valueSet()) m_context << swapInstruction(1) << Instruction::POP; break; @@ -670,8 +671,9 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) if (function.kind() == FunctionType::Kind::Transfer) { // Check if zero (out of stack or not enough balance). + // TODO: bubble up here, but might also be different error. m_context << Instruction::ISZERO; - m_context.appendConditionalRevert(); + m_context.appendConditionalRevert(true); } break; case FunctionType::Kind::Selfdestruct: @@ -1823,6 +1825,7 @@ void ExpressionCompiler::appendExternalFunctionCall( if (funKind == FunctionType::Kind::External || funKind == FunctionType::Kind::CallCode || funKind == FunctionType::Kind::DelegateCall) { m_context << Instruction::DUP1 << Instruction::EXTCODESIZE << Instruction::ISZERO; + // TODO: error message? m_context.appendConditionalRevert(); existenceChecked = true; } @@ -1865,7 +1868,7 @@ void ExpressionCompiler::appendExternalFunctionCall( { //Propagate error condition (if CALL pushes 0 on stack). m_context << Instruction::ISZERO; - m_context.appendConditionalRevert(); + m_context.appendConditionalRevert(true); } utils().popStackSlots(remainsSize); diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index b6b26f496..41ebe4628 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -10512,6 +10512,43 @@ BOOST_AUTO_TEST_CASE(require_with_message) ABI_CHECK(callContractFunction("h()"), encodeArgs(0, 0x40, 0x80, 0, 0x40, 3, "abc")); } +BOOST_AUTO_TEST_CASE(bubble_up_error_messages) +{ + char const* sourceCode = R"( + contract D { + function f() public { + revert("message"); + } + function g() public { + this.f(); + } + } + contract C { + D d = new D(); + function forward(address target, bytes data) internal returns (bool success, bytes retval) { + uint retsize; + assembly { + success := call(not(0), target, 0, add(data, 0x20), mload(data), 0, 0) + retsize := returndatasize() + } + retval = new bytes(retsize); + assembly { + returndatacopy(add(retval, 0x20), 0, returndatasize()) + } + } + function f() public returns (bool, bytes) { + return forward(address(d), msg.data); + } + function g() public returns (bool, bytes) { + return forward(address(d), msg.data); + } + } + )"; + compileAndRun(sourceCode, 0, "C"); + ABI_CHECK(callContractFunction("f()"), encodeArgs(0, 0x40, 0x80, 0, 0x40, 3, "message")); + ABI_CHECK(callContractFunction("g()"), encodeArgs(0, 0x40, 0x80, 0, 0x40, 3, "message")); +} + BOOST_AUTO_TEST_CASE(negative_stack_height) { // This code was causing negative stack height during code generation