Stream: git-wasmtime

Topic: wasmtime / PR #5231 Cranelift: Make `heap_addr` return ca...

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:57):

fitzgen requested alexcrichton for a review on PR #5231.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:57):

fitzgen requested jameysharp for a review on PR #5231.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:57):

fitzgen opened PR #5231 from update-heap-addr to main:

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 https://github.com/bytecodealliance/wasmtime/pull/5190 lands (which is blocked on this commit) will also match the implemented behavior for dynamic memories.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Nov 09 2022 at 01:32):

jameysharp created PR review comment:

I thought I understood what you're trying to do, but this documentation has me puzzled. Should you have edited more of this comment, perhaps to say it returns p + Offset rather than just p?

Wasmtime GitHub notifications bot (Nov 09 2022 at 01:32):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 09 2022 at 17:52):

fitzgen updated PR #5231 from update-heap-addr to main.

Wasmtime GitHub notifications bot (Nov 09 2022 at 17:52):

fitzgen created PR review comment:

Ah yes, I overlooked that bit of documentation. Updated now.

Wasmtime GitHub notifications bot (Nov 09 2022 at 17:52):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 09 2022 at 17:56):

fitzgen updated PR #5231 from update-heap-addr to main.

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:20):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:20):

cfallin created PR review comment:

Tiny nit (and pre-existing) but worth trying to get right here: .. in Rust usually means inclusive start, exclusive end, and with those semantics we should write this p .. p + Offset + Size. Or alternately use ..=.

The plain meaning here might be clear enough to a "common-sense reader" but perhaps something like "offset range p .. p + Offset + Size (i.e., Offset + Size bytes starting at p)" would be even clearer?

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:20):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:20):

cfallin created PR review comment:

s/not greater than/less than or equal to/ here and below, for clarity?

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:20):

cfallin created PR review comment:

Comment here to note why this addition can't wrap (offset is u32, access_size is u8)?

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:20):

cfallin created PR review comment:

Seeing this a third time, now I think a helper might be best: offset_plus_size(offset, access_size), giving us a nice typesafe clearly-overflow-safe idiom.

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:20):

cfallin created PR review comment:

Perhaps factor this "offset + size" expression out of the if/else?

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:57):

fitzgen updated PR #5231 from update-heap-addr to main.

Wasmtime GitHub notifications bot (Nov 09 2022 at 18:57):

fitzgen has enabled auto merge for PR #5231.

Wasmtime GitHub notifications bot (Nov 09 2022 at 19:53):

fitzgen merged PR #5231.

Last updated: Dec 23 2024 at 13:07 UTC