Cranelift: Make heap_addr return calculated base + index + offset#5231
Merged
fitzgen merged 3 commits intobytecodealliance:mainfrom Nov 9, 2022
Merged
Cranelift: Make heap_addr return calculated base + index + offset#5231fitzgen merged 3 commits intobytecodealliance:mainfrom
heap_addr return calculated base + index + offset#5231fitzgen merged 3 commits intobytecodealliance:mainfrom
Conversation
jameysharp
reviewed
Nov 9, 2022
4542a8f to
adb2d6c
Compare
Rather than return just the `base + index`.
(Note: I've chosen to use the nomenclature "index" for the dynamic operand and
"offset" for the static immediate.)
This move the addition of the `offset` into `heap_addr`, instead of leaving it
for the subsequent memory operation, so that we can Spectre-guard the full
address, and not allow speculative execution to read the first 4GiB of memory.
Before this commit, we were effectively doing
load(spectre_guard(base + index) + offset)
Now we are effectively doing
load(spectre_guard(base + index + offset))
Finally, this also corrects `heap_addr`'s documented semantics to say that it
returns an address that will trap on access if `index + offset + access_size` is
out of bounds for the given heap, rather than saying that the `heap_addr` itself
will trap. This matches the implemented behavior for static memories, and after
bytecodealliance#5190 lands (which is blocked
on this commit) will also match the implemented behavior for dynamic memories.
adb2d6c to
9996cb1
Compare
cfallin
approved these changes
Nov 9, 2022
Member
cfallin
left a comment
There was a problem hiding this comment.
Looks generally really good to me -- thanks for doing this update!
I added some nitpicks on the documentation, some on pre-existing stuff, as I figure it's worth trying to get this really polished and clear; and a few little cleanups elsewhere; but nothing major.
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
3 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.
Rather than return just the
base + index.(Note: I've chosen to use the nomenclature "index" for the dynamic operand and "offset" for the static immediate.)
This move the addition of the
offsetintoheap_addr, instead of leaving it for the subsequent memory operation, so that we can Spectre-guard the full address, and not allow speculative execution to read the first 4GiB of memory.Before this commit, we were effectively doing
Now we are effectively doing
Finally, this also corrects
heap_addr's documented semantics to say that it returns an address that will trap on access ifindex + offset + access_sizeis out of bounds for the given heap, rather than saying that theheap_addritself will trap. This matches the implemented behavior for static memories, and after #5190 lands (which is blocked on this commit) will also match the implemented behavior for dynamic memories.