From cedf7087e68e67c7bfbd47ae95dcb16b81ad2e02 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Sun, 24 Mar 2024 09:28:47 +0800 Subject: [PATCH] fix[codegen]: fix non-memory reason strings (#3877) fix code generation for non-memory reason strings. previously, the byte array copier would fail trying to produce code for the buf, which was an int rather than an IRnode. this commit cleans up the revert generation code, allocates a fresh buffer to avoid memory cleanliness issues, and refactors the code to use abi_encoder code instead of handrolling the ABI encoding routine. --------- Co-authored-by: Charles Cooper --- .github/workflows/pull-request.yaml | 2 + .../codegen/features/test_assert.py | 22 +++++++ vyper/codegen/stmt.py | 58 +++++++------------ 3 files changed, 44 insertions(+), 38 deletions(-) diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index f36b465bb5..5f93b613ee 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -34,6 +34,7 @@ jobs: # ux: language changes (UX) # tool: integration # ir: (old) IR/codegen changes + # codegen: lowering from vyper AST to codegen # venom: venom changes scopes: | ci @@ -43,6 +44,7 @@ jobs: ux tool ir + codegen venom requireScope: true subjectPattern: '^(?![A-Z]).+$' diff --git a/tests/functional/codegen/features/test_assert.py b/tests/functional/codegen/features/test_assert.py index efde44fbad..406bade926 100644 --- a/tests/functional/codegen/features/test_assert.py +++ b/tests/functional/codegen/features/test_assert.py @@ -28,6 +28,8 @@ def foo(): def test_assert_reason(w3, get_contract_with_gas_estimation, tx_failed, memory_mocker): code = """ +err: String[32] + @external def test(a: int128) -> int128: assert a > 1, "larger than one please" @@ -43,6 +45,17 @@ def test2(a: int128, b: int128, extra_reason: String[32]) -> int128: @external def test3(reason_str: String[32]): raise reason_str + +@external +def test4(a: int128, reason_str: String[32]) -> int128: + self.err = reason_str + assert a > 1, self.err + return 1 + a + +@external +def test5(reason_str: String[32]): + self.err = reason_str + raise self.err """ c = get_contract_with_gas_estimation(code) @@ -66,6 +79,15 @@ def test3(reason_str: String[32]): c.test3("An exception") assert _fixup_err_str(e_info.value.args[0]) == "An exception" + assert c.test4(2, "msg") == 3 + with pytest.raises(TransactionFailed) as e_info: + c.test4(0, "larger than one again please") + assert _fixup_err_str(e_info.value.args[0]) == "larger than one again please" + + with pytest.raises(TransactionFailed) as e_info: + c.test5("A storage exception") + assert _fixup_err_str(e_info.value.args[0]) == "A storage exception" + invalid_code = [ """ diff --git a/vyper/codegen/stmt.py b/vyper/codegen/stmt.py index 1da31d3bda..9c5720d61c 100644 --- a/vyper/codegen/stmt.py +++ b/vyper/codegen/stmt.py @@ -1,6 +1,7 @@ import vyper.codegen.events as events import vyper.utils as util from vyper import ast as vy_ast +from vyper.codegen.abi_encoder import abi_encode from vyper.codegen.context import Constancy, Context from vyper.codegen.core import ( LOAD, @@ -9,20 +10,14 @@ clamp_le, get_dyn_array_count, get_element_ptr, - make_byte_array_copier, + get_type_for_exact_size, make_setter, - zero_pad, + wrap_value_for_external_return, ) from vyper.codegen.expr import Expr from vyper.codegen.return_ import make_return_stmt from vyper.evm.address_space import MEMORY, STORAGE -from vyper.exceptions import ( - CodegenPanic, - CompilerPanic, - StructureException, - TypeCheckFailure, - tag_exceptions, -) +from vyper.exceptions import CodegenPanic, StructureException, TypeCheckFailure, tag_exceptions from vyper.semantics.types import DArrayT from vyper.semantics.types.shortcuts import UINT256_T @@ -132,39 +127,26 @@ def _assert_reason(self, test_expr, msg): finally: self.context.constancy = tmp - # TODO this is probably useful in codegen.core - # compare with eval_seq. - def _get_last(ir): - if len(ir.args) == 0: - return ir.value - return _get_last(ir.args[-1]) - - # TODO maybe use ensure_in_memory - if msg_ir.location != MEMORY: - buf = self.context.new_internal_variable(msg_ir.typ) - instantiate_msg = make_byte_array_copier(buf, msg_ir) - else: - buf = _get_last(msg_ir) - if not isinstance(buf, int): # pragma: nocover - raise CompilerPanic(f"invalid bytestring {buf}\n{self}") - instantiate_msg = msg_ir + msg_ir = wrap_value_for_external_return(msg_ir) + bufsz = 64 + msg_ir.typ.memory_bytes_required + buf = self.context.new_internal_variable(get_type_for_exact_size(bufsz)) # offset of bytes in (bytes,) method_id = util.method_id_int("Error(string)") - # abi encode method_id + bytestring - assert buf >= 36, "invalid buffer" - # we don't mind overwriting other memory because we are - # getting out of here anyway. - _runtime_length = ["mload", buf] - revert_seq = [ - "seq", - instantiate_msg, - zero_pad(buf), - ["mstore", buf - 64, method_id], - ["mstore", buf - 32, 0x20], - ["revert", buf - 36, ["add", 4 + 32 + 32, ["ceil32", _runtime_length]]], - ] + # abi encode method_id + bytestring to `buf+32`, then + # write method_id to `buf` and get out of here + payload_buf = buf + 32 + bufsz -= 32 # reduce buffer by size of `method_id` slot + encoded_length = abi_encode(payload_buf, msg_ir, self.context, bufsz, returns_len=True) + with encoded_length.cache_when_complex("encoded_len") as (b1, encoded_length): + revert_seq = [ + "seq", + ["mstore", buf, method_id], + ["revert", buf + 28, ["add", 4, encoded_length]], + ] + revert_seq = b1.resolve(revert_seq) + if is_raise: ir_node = revert_seq else: