fix[venom]: invalid jump error#4214
Conversation
charles-cooper
left a comment
charles-cooper
left a comment
There was a problem hiding this comment.
this is kind of a weird fix. isn't the real fix to change line 540 of venom_to_assembly.py to read if inst.output not in next_liveness?
I was wondering about this and it should be fix but the problem was that I wanted to minimize the number of the Just as a possibility I was playing with idea to clear up the stack at the end of the function before return but I think that would introduce more instruction then clearing the stack right after. |
HodanPlodky
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4214 +/- ##
==========================================
- Coverage 91.35% 88.34% -3.01%
==========================================
Files 109 109
Lines 15637 15628 -9
Branches 3443 3437 -6
==========================================
- Hits 14285 13807 -478
- Misses 920 1295 +375
- Partials 432 526 +94 ☔ View full report in Codecov by Sentry. |
…mprehensions Co-authored-by: Charles Cooper <[email protected]>
vyper/venom/venom_to_assembly.py
Outdated
| for i, inst in enumerate(all_inst): | ||
| next_liveness = all_inst[i + 1].liveness if i + 1 < len(all_inst) else OrderedSet() |
There was a problem hiding this comment.
| for i, inst in enumerate(all_inst): | |
| next_liveness = all_inst[i + 1].liveness if i + 1 < len(all_inst) else OrderedSet() | |
| for i, inst in enumerate(all_insts): | |
| next_liveness = all_insts[i + 1].liveness if i + 1 < len(all_insts) else OrderedSet() |
vyper/venom/venom_to_assembly.py
Outdated
| if "call" in inst.opcode and inst.output not in next_liveness: | ||
| if cleanup_needed and inst.output not in next_liveness: | ||
| depth = stack.get_depth(inst.output) | ||
| if depth != 0: |
There was a problem hiding this comment.
can this branch ever be hit? 😱
vyper/venom/venom_to_assembly.py
Outdated
|
|
||
| self._generate_evm_for_basicblock_r(asm, fn.entry, StackModel(), cleanup_needed) | ||
| self._generate_evm_for_basicblock_r( | ||
| asm, fn.entry, StackModel(), fn.is_cleanup_needed() |
There was a problem hiding this comment.
has to re-iterate over all basic blocks for every basic block no?
There was a problem hiding this comment.
ah, never mind, it gets passed thru in the recursion
vyper/venom/venom_to_assembly.py
Outdated
|
|
||
| def _is_cleanup_needed(fn: IRFunction) -> bool: | ||
| for bb in fn.get_basic_blocks(): | ||
| if bb.is_terminated and bb.instructions[-1].opcode == "ret": |
There was a problem hiding this comment.
is_terminated and then checking for ret seems superfluous. Just a check if instructions not empty.
fix an issue where `ret` would generate a jump to an invalid location because the stack is not clean. the solution is to always pop instruction outputs which are not used. note this leads to a slight performance regression (roughly 0.07% bytecode size), since `POP`s are always generated, even in cases when the stack does not need to be cleaned (e.g. before `STOP`). --------- Co-authored-by: Charles Cooper <[email protected]> Co-authored-by: Harry Kalogirou <[email protected]>
3 participants
What I did
Fixed the issue that caused the invalid jump when that occurred when
remove_unused_variablescould not remove instruction with output value because it is volatile.How I did it
Allowed to remove mload instruction in
remove_unused_variablesand insert POP instruction if needed in asm generation.How to verify it
I used vyper tests and tests in snekmate which originally exhibited error.
Commit message
Description for the changelog
Cute Animal Picture