bugc: fix return-epilogue stack cleanup for multiple stale values#274
Open
gnidan wants to merge 1 commit into
Open
bugc: fix return-epilogue stack cleanup for multiple stale values#274gnidan wants to merge 1 commit into
gnidan wants to merge 1 commit into
Conversation
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A user function's return epilogue clears any leftover stack values before jumping back to the caller, keeping only the return value on top. The cleanup rotation was wrong whenever two or more stale values sat below the return value: each iteration swapped the deepest element to the top and popped it, which on the first swap sank the return value to the bottom and then, on the next iteration, brought it back up and popped it. It only produced the right result when exactly one stale value needed removing.
In practice this surfaced whenever a function returned from a branch.
loadValueduplicates an operand that is already on the stack, so a comparison operand and the branch condition are both left behind; areturnin the taken branch then ran the cleanup with two stale values and returned the branch condition instead of the intended value. For exampleif (x == 0) { return 9; } else { return 7; }returned 1 (the condition) rather than 9 from the true branch, while the false branch happened to work.This replaces the rotation with a repeated
SWAP1; POP: with the return value on top, swap the stale value just beneath it up to the top and pop it, once per stale value. The return value stays on top throughout, so any number of leftovers are removed correctly.A regression test covers returning a constant and a parameter from a branch, and returning a call result from a branch, at every optimization level.
The scope of this change is the return-epilogue stack cleanup, where a branch-return yielded the branch condition instead of the return value. It does not touch how the operand stack is tracked across blocks and calls, which has a separate, more serious problem: self-recursive functions are miscompiled because the pre-call stack cleanup undercounts untracked leftovers and corrupts the callee's arguments, so this fix does not yet make recursion correct. Both that active miscompilation and the latent multi-predecessor desync stem from evmgen threading its tracked stack linearly through block layout order rather than through the control-flow graph, and both are tracked in #275.