Don't use MemoryStyle for heap base pointer relocations#9569
Merged
alexcrichton merged 2 commits intobytecodealliance:mainfrom Nov 6, 2024
Merged
Don't use MemoryStyle for heap base pointer relocations#9569alexcrichton merged 2 commits intobytecodealliance:mainfrom
MemoryStyle for heap base pointer relocations#9569alexcrichton merged 2 commits intobytecodealliance:mainfrom
Conversation
Instead add a helper method to `Memory` which indicates whether the base pointer of memory can move. Use this and plumb the result around to the various locations that require it. This improves the `readonly` application of the base pointer in Cranelift by having the optimization kick in in more scenarios. It additionally fixes combining shared linear memories with 64-bit addressing or a page size of 1 by adding a runtime check before relocation of a linear memory that it's allowed to do so. A few codegen tests are added to ensure that `readonly` is applied where it wasn't previously and additionally a new `*.wast` test was added with the cross product of various features of linear memories and some basic tests to ensure that the memories all work as expected. This refactoring fixes two preexisting issues about `shared` memories requiring a "static" memory style. The restriction is now based on whether the pointer can relocate or not and that's upheld via the new trait method added here. To account for these bug fixes the fuzzers have been updated to allow mixing the `threads` proposal with `memory64` or `custom-page-sizes`. Closes bytecodealliance#4267 Closes bytecodealliance#9523
Member
Author
|
I'll also notably point out that this is incompatible with |
github-actions
bot
added
fuzzing
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "fuzzing", "wasmtime:api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen
approved these changes
Nov 6, 2024
Member
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Really like this clean up, everything is much easier to follow now. Thanks!
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This code will be short-lived due to scheduled future refactorings but the idea is that when a "dynamic" memory is chosen the minimum size of the allocation needs to be at least `tunables.memory_reservation` to fit the constraints of the rest of the system, so be sure to factor that in.
alexcrichton
force-pushed
the
heap-base-pointer
branch
from
ecb7ede to
29e0875
Compare
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
2 participants
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.
Instead add a helper method to
Memorywhich indicates whether the base pointer of memory can move. Use this and plumb the result around to the various locations that require it. This improves thereadonlyapplication of the base pointer in Cranelift by having the optimization kick in in more scenarios. It additionally fixes combining shared linear memories with 64-bit addressing or a page size of 1 by adding a runtime check before relocation of a linear memory that it's allowed to do so.A few codegen tests are added to ensure that
readonlyis applied where it wasn't previously and additionally a new*.wasttest was added with the cross product of various features of linear memories and some basic tests to ensure that the memories all work as expected.This refactoring fixes two preexisting issues about
sharedmemories requiring a "static" memory style. The restriction is now based on whether the pointer can relocate or not and that's upheld via the new trait method added here.To account for these bug fixes the fuzzers have been updated to allow mixing the
threadsproposal withmemory64orcustom-page-sizes.Closes #4267
Closes #9523