fix[venom]: fuzz venom pass ordering - #5219
HodanPlodky wants to merge 28 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 770ee21f6f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if space is None: | ||
| # sanity check | ||
| assert eff == FMP, eff |
There was a problem hiding this comment.
When a dead balance, selfbalance, extcodesize, extcodehash, or returndatasize instruction reaches this pass, its read effect maps to no address space, but it is not FMP, so this assertion raises and aborts compilation. These instructions are non-volatile and may legitimately have unused outputs; non-memory effects should simply require no alias-analysis invalidation rather than being rejected.
Useful? React with 👍 / 👎.
| if not compiler_settings.experimental_codegen: | ||
| pytest.skip() |
There was a problem hiding this comment.
In the checked fuzzing job at .github/workflows/test.yml:281-290, pytest is invoked with -m "fuzzing" but without --experimental-codegen; that option is store_true and therefore the compiler_settings fixture leaves experimental codegen disabled. Consequently every instance of this newly added test takes this skip, so none of the pass-disable combinations run in CI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73616d08ee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for inst in bb.instructions: | ||
| if inst.is_param: | ||
| if inst.opcode == "retpc_param": | ||
| inst.make_nop() |
There was a problem hiding this comment.
When a valid callee forwards its retpc_param through an assign or phi before using that alias as the final ret operand, replacing the parameter with a nop leaves the intermediate instruction reading an undefined variable; the later ret rewrite removes only the terminator's operand. This can make code generation fail when RemoveUnusedVariablesPass is disabled, as the new fuzz test permits. Bind the return-PC parameter to the invoke target as before, or rewrite all of its aliases before removing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64a85d4228
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| run = [] | ||
|
|
||
| def temp(*args, **kwargs): | ||
| run.append(True) |
There was a problem hiding this comment.
When this test is invoked with experimental codegen and the default gas/O2 optimization level, generated entries such as AssertEliminationPass, OverflowEliminationPass, ReduceLiteralsCodesize, and TailMergePass are not present in PASSES_O2, so their patched methods are never called. Because the run tracking list is not checked, examples containing only those passes pass vacuously, while mixed examples disable only part of the generated subset; filter the strategy by the selected pipeline or assert that every selected pass actually ran.
Useful? React with 👍 / 👎.
charles-cooper
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4ba5ba797
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # host's own reclaim governs the inlined data. Whether the | ||
| # host publishes is determined solely by the host's own | ||
| # terminators (plain `ret` is callee-save). | ||
| assert retpc_op in inst.operands, retpc_op |
There was a problem hiding this comment.
When a valid raw Venom callee represents its return PC as a plain parameter (%retpc = param; ret %retpc) or as a literal label, no retpc_param is encountered and retpc_op remains None, so this assertion aborts compilation whenever the callee is selected for inlining. These forms are accepted by the calling-convention validator and the previous implementation successfully rewrote their ret terminators, so the assertion must account for return PCs discovered through FunctionCallLayout rather than requiring the dedicated opcode.
Useful? React with 👍 / 👎.
| @@ -14,9 +14,6 @@ def get_example_vy_filenames(): | |||
|
|
|||
| @pytest.mark.parametrize("vy_filename", get_example_vy_filenames()) | |||
| def test_compile(vy_filename): | |||
There was a problem hiding this comment.
In the regular test matrix where --experimental-codegen is absent, this test compiles through the legacy backend, so implementation_v_700.vy still reaches the documented StackTooDeep: Unsupported dup depth 17 failure. The compiler changes in this commit are confined to the Venom backend and cannot fix that legacy path; removing the filename-specific xfail therefore makes every non-experimental matrix job fail when this parameter is collected.
Useful? React with 👍 / 👎.
charles-cooper
changed the title
This branch has not been deployed
4 participants
What I did
Created the fuzz test that showed few error in the implementation. There error are fixed and discuses in the #5231
How I did it
How to verify it
pytest -m "fuzzing" --experimental-codegen tests/functional/examples/thirdparty/test_thirdparty_fuzz.py --optimize O3 -k test_compile_pass_fuzzCommit message
Description for the changelog
Cute Animal Picture