Conversation
When we initially implemented the "heap tracker", the HEA section was relatively unused, except for temporary native and argument storage space. The heap tracker was a *third* LIFO stack in the system, that tracked the sizes of dynamically sized allocations into HEA. We use it for dynamically-sized arrays and for an edge case in the ternary operator. Why did we make a new LIFO stack instead of inlining the sizes into HEA? At the time, I figured HEA was too easily corrupted, and it would be safer to track the allocations separately. In retrospect this does not make much sense. The third structure is very difficult to inline in the JIT, it was never properly validated or capped, and the HEA of course should only ever be internally corrupted so the argument never made sense. This patch removes the HeapTracker data structure, which predates SourceMod 1.0, and simply stores allocation sizes directly into HEA.
SourcePawn has always required a balanced STK and HEA before a RETN instruction. This is unnecessary, since the VM already knows the correct values, and indeed validates them anyway. If at any point an exception is thrown, validation is skipped and the known-good values are restored. It is much simpler to make RETN responsible for restoring the correct values. The compiler then doesn't have to generate STACK and HEAP instructions before every return, or be burdened with flowing all return paths through a common cleanup epilogue. This adds 1-2 extra instructions on the generated prologue and epilogue paths, however, a compiler that takes advantage of the new semantics can omit the final STACK/HEA balancing sequence. For compilers to rely on the new semantics, they need a compatible VM, so the pcode version is being bumped from 11 to 12.
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.
The way RETN works is pretty lame. The compiler is responsible for balancing the stack and heap, despite the fact that the VM can already trivially do that. In fact, the VM does do it. The correct sp/hp values are restored after an exception, since the RETN instruction would never be reached.
This balancing requirement makes it harder to improve the compiler, since we have to emit an epilogue at every return site, which places a useless bookkeeping restriction and generates unnecessary bytecode.
There's an argument that requiring a balanced stack at RETN helps catch compiler bugs, but I'd argue those bugs aren't really important. Codegen leaks will be caught if they matter - inside loops, or with recursion. And codegen leaks are extremely rare. We've only had one that I know of, and it only manifested in a loop [1].
As a bonus, changing RETN simplifies some things in the VM. We finally have a reason to remove the hideous HeapTracker data structure which has somehow survived since ~2006 - the sizes of HEA allocations can just be stored in HEA itself. Then in the PROC/RETN paths, we simply store/pop the HEA pointer in the frame (there was a free slot that is used upstream, but not in SourcePawn). The SP pointer was always there to begin with.
Old PROC:
Old RETN:
New PROC:
New RETN:
Since a compiler cannot rely on this new functionality without a compatible VM, the bytecode version has received its first ever actual compatibility bump to 12. For now, the compiler still generates version 10 bytecode. An -x12 argument has been added to opt-in to the new semantics.
The VM now has a few more instructions for PROC/RETN, but a v12 compiler should be generating more efficient code. (And possibly as a follow-up we could remove instructions if the function is known not to use HEA.)
[1] https://bugs.alliedmods.net/show_bug.cgi?id=6370