From 33719560a5d13089fcd527681d170556824bf8d1 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Thu, 30 May 2024 13:17:21 -0700 Subject: [PATCH] fix[venom]: fix list of volatile instructions (#4065) `create` and `create2` are volatile instructions that were missing. the other ones were already handled by the case for `output is None` in `removed_unused_variables`. misc: - rename `volatile` to `is_volatile` for uniformity - use `is_bb_terminator` instead of `in BB_TERMINATORS` --------- Co-authored-by: Harry Kalogirou --- vyper/venom/analysis/cfg.py | 6 ++--- vyper/venom/basicblock.py | 24 ++++++++++++++++--- vyper/venom/passes/dft.py | 6 ++--- vyper/venom/passes/remove_unused_variables.py | 2 +- vyper/venom/venom_to_assembly.py | 2 +- 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/vyper/venom/analysis/cfg.py b/vyper/venom/analysis/cfg.py index 6bd7e538e9..bd2ae34b68 100644 --- a/vyper/venom/analysis/cfg.py +++ b/vyper/venom/analysis/cfg.py @@ -1,6 +1,6 @@ from vyper.utils import OrderedSet from vyper.venom.analysis.analysis import IRAnalysis -from vyper.venom.basicblock import BB_TERMINATORS, CFG_ALTERING_INSTRUCTIONS +from vyper.venom.basicblock import CFG_ALTERING_INSTRUCTIONS class CFGAnalysis(IRAnalysis): @@ -18,9 +18,7 @@ def analyze(self) -> None: for bb in fn.get_basic_blocks(): assert len(bb.instructions) > 0, "Basic block should not be empty" last_inst = bb.instructions[-1] - assert ( - last_inst.opcode in BB_TERMINATORS - ), f"Last instruction should be a terminator {bb}" + assert last_inst.is_bb_terminator, f"Last instruction should be a terminator {bb}" for inst in bb.instructions: if inst.opcode in CFG_ALTERING_INSTRUCTIONS: diff --git a/vyper/venom/basicblock.py b/vyper/venom/basicblock.py index 19e8801663..d6fb9560cd 100644 --- a/vyper/venom/basicblock.py +++ b/vyper/venom/basicblock.py @@ -12,6 +12,8 @@ "call", "staticcall", "delegatecall", + "create", + "create2", "invoke", "sload", "sstore", @@ -34,6 +36,15 @@ "ret", "jmp", "jnz", + "djmp", + "log", + "selfdestruct", + "invalid", + "revert", + "assert", + "assert_unreachable", + "stop", + "exit", ] ) @@ -41,7 +52,6 @@ [ "mstore", "sstore", - "dstore", "istore", "tstore", "dloadbytes", @@ -67,6 +77,10 @@ ] ) +assert VOLATILE_INSTRUCTIONS.issuperset(NO_OUTPUT_INSTRUCTIONS), ( + NO_OUTPUT_INSTRUCTIONS - VOLATILE_INSTRUCTIONS +) + CFG_ALTERING_INSTRUCTIONS = frozenset(["jmp", "djmp", "jnz"]) if TYPE_CHECKING: @@ -221,9 +235,13 @@ def __init__( self.error_msg = None @property - def volatile(self) -> bool: + def is_volatile(self) -> bool: return self.opcode in VOLATILE_INSTRUCTIONS + @property + def is_bb_terminator(self) -> bool: + return self.opcode in BB_TERMINATORS + def get_label_operands(self) -> Iterator[IRLabel]: """ Get all labels in instruction. @@ -499,7 +517,7 @@ def is_terminated(self) -> bool: # if we can/need to append instructions to the basic block. if len(self.instructions) == 0: return False - return self.instructions[-1].opcode in BB_TERMINATORS + return self.instructions[-1].is_bb_terminator @property def is_terminal(self) -> bool: diff --git a/vyper/venom/passes/dft.py b/vyper/venom/passes/dft.py index 8429c19711..f45a60079c 100644 --- a/vyper/venom/passes/dft.py +++ b/vyper/venom/passes/dft.py @@ -1,6 +1,6 @@ from vyper.utils import OrderedSet from vyper.venom.analysis.dfg import DFGAnalysis -from vyper.venom.basicblock import BB_TERMINATORS, IRBasicBlock, IRInstruction, IRVariable +from vyper.venom.basicblock import IRBasicBlock, IRInstruction, IRVariable from vyper.venom.function import IRFunction from vyper.venom.passes.base_pass import IRPass @@ -30,7 +30,7 @@ def _process_instruction_r(self, bb: IRBasicBlock, inst: IRInstruction, offset: self.visited_instructions.add(inst) self.inst_order_num += 1 - if inst.opcode in BB_TERMINATORS: + if inst.is_bb_terminator: offset = len(bb.instructions) if inst.opcode == "phi": @@ -55,7 +55,7 @@ def _process_basic_block(self, bb: IRBasicBlock) -> None: for inst in bb.instructions: inst.fence_id = self.fence_id - if inst.volatile: + if inst.is_volatile: self.fence_id += 1 # We go throught the instructions and calculate the order in which they should be executed diff --git a/vyper/venom/passes/remove_unused_variables.py b/vyper/venom/passes/remove_unused_variables.py index 1ce5c141d9..be9c1ed535 100644 --- a/vyper/venom/passes/remove_unused_variables.py +++ b/vyper/venom/passes/remove_unused_variables.py @@ -31,7 +31,7 @@ def run_pass(self): def _process_instruction(self, inst): if inst.output is None: return - if inst.volatile: + if inst.is_volatile or inst.is_bb_terminator: return uses = self.dfg.get_uses(inst.output) if len(uses) > 0: diff --git a/vyper/venom/venom_to_assembly.py b/vyper/venom/venom_to_assembly.py index beb530a42c..51fac10134 100644 --- a/vyper/venom/venom_to_assembly.py +++ b/vyper/venom/venom_to_assembly.py @@ -305,7 +305,7 @@ def _clean_unused_params(self, asm: list, bb: IRBasicBlock, stack: StackModel) - for i, inst in enumerate(bb.instructions): if inst.opcode != "param": break - if inst.volatile and i + 1 < len(bb.instructions): + if inst.is_volatile and i + 1 < len(bb.instructions): liveness = bb.instructions[i + 1].liveness if inst.output is not None and inst.output not in liveness: depth = stack.get_depth(inst.output)