Remove recursion from AMode lowering rules #6968
Merged
Conversation
This file contains 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
This commit removes the recursion present in the x64 backend's `to_amode` and `to_amode_add` helpers. The recursion is currently unbounded and controlled by user input meaning it's not too hard to craft user input which triggers stack overflow in the host. By removing recursion there's no need to worry about this since the stack depth will never be hit. The main concern with removing recursion is that code quality may not be quite as good any more. The purpose of the recursion here is to "hunt for constants" and update the immediate `Offset32`, and now without recursion only at most one constant is found and folded instead of an arbitrary number of constants as before. This should continue to produce the same code as before so long as optimizations are enabled, but without optimizations this will produce worse code than before. Note with a hypothetical mid-end optimization that does this constant folding for us the rules here could be further simplified to purely consider the shape of the input `Value` to amode computation without considering constants at all.
Same as the prior commit for x64, but for aarch64 instead. Unlikely to be reachable from wasm due to this only being a part of addressing modes which are more strictly controlled in wasm (e.g. wasm addresses are always a zero-extended value added to a base, so it's not possible to have a long chain of constants at the top level in clif)
github-actions
bot
added
cranelift
|
Given our pending patch releases I'll go ahead and backport this to all the branches once this is approved. |
cfallin
approved these changes
Sep 11, 2023
This was referenced Sep 11, 2023
alexcrichton
added a commit
that referenced
this pull request
Sep 12, 2023
* x64: Remove recursion in `to_amode` helper This commit removes the recursion present in the x64 backend's `to_amode` and `to_amode_add` helpers. The recursion is currently unbounded and controlled by user input meaning it's not too hard to craft user input which triggers stack overflow in the host. By removing recursion there's no need to worry about this since the stack depth will never be hit. The main concern with removing recursion is that code quality may not be quite as good any more. The purpose of the recursion here is to "hunt for constants" and update the immediate `Offset32`, and now without recursion only at most one constant is found and folded instead of an arbitrary number of constants as before. This should continue to produce the same code as before so long as optimizations are enabled, but without optimizations this will produce worse code than before. Note with a hypothetical mid-end optimization that does this constant folding for us the rules here could be further simplified to purely consider the shape of the input `Value` to amode computation without considering constants at all. * aarch64: Remove recursion from amode lowering rules Same as the prior commit for x64, but for aarch64 instead. Unlikely to be reachable from wasm due to this only being a part of addressing modes which are more strictly controlled in wasm (e.g. wasm addresses are always a zero-extended value added to a base, so it's not possible to have a long chain of constants at the top level in clif)
eduardomourar
pushed a commit
to eduardomourar/wasmtime
that referenced
this pull request
Sep 13, 2023
* x64: Remove recursion in `to_amode` helper This commit removes the recursion present in the x64 backend's `to_amode` and `to_amode_add` helpers. The recursion is currently unbounded and controlled by user input meaning it's not too hard to craft user input which triggers stack overflow in the host. By removing recursion there's no need to worry about this since the stack depth will never be hit. The main concern with removing recursion is that code quality may not be quite as good any more. The purpose of the recursion here is to "hunt for constants" and update the immediate `Offset32`, and now without recursion only at most one constant is found and folded instead of an arbitrary number of constants as before. This should continue to produce the same code as before so long as optimizations are enabled, but without optimizations this will produce worse code than before. Note with a hypothetical mid-end optimization that does this constant folding for us the rules here could be further simplified to purely consider the shape of the input `Value` to amode computation without considering constants at all. * aarch64: Remove recursion from amode lowering rules Same as the prior commit for x64, but for aarch64 instead. Unlikely to be reachable from wasm due to this only being a part of addressing modes which are more strictly controlled in wasm (e.g. wasm addresses are always a zero-extended value added to a base, so it's not possible to have a long chain of constants at the top level in clif)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit removes recursion from AMode lowering rules for both the x64 and aarch64 backends. On the x64 backend this can lead to stack overflow on the host given wasm inputs during compilation due to the fact that
i32.addgoes through this path of lowering (e.g. trying to use theleainstruction). The recursion in these rules are rewritten to hunt for constants differently in a non-recursive fashion which relies on egraph optimizations to place constants in the right place to get good code output.This PR additionally updates the AArch64 backend to remove the recursion there as well. While not reachable from wasm this is still reachable from Cranelift and is a good bug to fix regardless.
Note that the stack overflow during compilation requires optimizations to be disabled, as otherwise egraphs will already fold constants together which means that recursion won't happen deeply.
Thanks to DFINITY for notifying the security@bytecodealliance.org mailing list for this issue! We concluded this in the end wasn't a CVE-worthy issue because it requires non-default settings to trigger (e.g. disabling optimizations). This is something I'll backport to the 13 release branch, however.