Stream: git-wasmtime

Topic: wasmtime / PR #3134 Change VMMemoryDefinition::current_le...

Wasmtime GitHub notifications bot (Jul 30 2021 at 18:11):

alexcrichton opened PR #3134 from current-length-usize to main:

This commit changes the definition of
VMMemoryDefinition::current_length to usize from its previous
definition of u32. This is a pretty impactful change because it also
changes the cranelift semantics of "dynamic" heaps where the bound
global value specifier must now match the pointer type for the platform
rather than the index type for the heap.

The motivation for this change is that the current_length field (or
bound for the heap) is intended to reflect the current size of the heap.
This is bound by usize on the host platform rather than u32 or u64. The previous choice of u32 couldn't represent a 4GB memory
because we couldn't put a number representing 4GB into the
current_length field. By using usize, which reflects the host's
memory allocation, this should better reflect the size of the heap and
allows Wasmtime to support a full 4GB heap for a wasm program (instead
of 4GB minus one page).

This commit also updates the legalization of the heap_addr clif
instruction to appropriately cast the address to the platform's pointer
type, handling bounds checks along the way. The practical impact for
today's targets is that a uextend is happening sooner than it happened
before, but otherwise there is no intended impact of this change. In the
future when 64-bit memories are supported there will likely need to be
fancier logic which handles offsets a bit differently (especially in the
case of a 64-bit memory on a 32-bit host).

The clif filetest changes should show the differences in codegen, and
the Wasmtime changes are largely removing casts here and there.

Closes #3022

<!--

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 (Jul 30 2021 at 19:48):

alexcrichton updated PR #3134 from current-length-usize to main.

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:08):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:19):

alexcrichton requested cfallin for a review on PR #3134.

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:51):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:51):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:51):

cfallin created PR review comment:

We should note that this also asserts on 32/32 and 64/64 cases; I don't think we need to do anything here in either case. I think this is fine for now -- better to assert out if we haven't thought carefully about it and if it's a theoretical need, especially in address computation logic -- but perhaps add a comment to this effect?

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:51):

cfallin created PR review comment:

Not strictly in-scope (sorry!) and I see you've just moved this code from below, but since we're looking at it now: we should have a method on the DFG for this -- maybe something like add_value_label_alias()? If not, at least a comment to the effect of "Add debug value-label alias so that debuginfo can name the extended value as the address".

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 16:59):

alexcrichton created PR review comment:

Oh hm wouldn't that be covered by the offset_ty == addr_ty check above?

I do want to make sure that unhandled cases are documented here for future readers, but I also figured that the matching pointer/offset cases were handled above.

Wasmtime GitHub notifications bot (Aug 02 2021 at 17:03):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 17:03):

cfallin created PR review comment:

Oh goodness -- I should, as a rule, discard any code review I do on Monday morning before the coffee is complete. Yes, I missed that, sorry!

Wasmtime GitHub notifications bot (Aug 02 2021 at 17:24):

alexcrichton updated PR #3134 from current-length-usize to main.

Wasmtime GitHub notifications bot (Aug 02 2021 at 18:09):

alexcrichton merged PR #3134.

Last updated: Oct 23 2024 at 20:03 UTC