Stream: git-wasmtime

Topic: wasmtime / PR #3302 cranelift: Add heap support to the in...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 04 2021 at 19:46):

afonso360 opened PR #3302 from interp-heap to main:

Hey!

This PR adds heap support to the interpreter and support for heap tests for test interpret.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 04 2021 at 19:54):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 04 2021 at 19:54):

afonso360 created PR review comment:

I'm not entirely sure this belongs here.

I had this separately in InterpreterState::get_heap_address, but I didn't want to duplicate the bounds checking done on this function.

Since bounds checking is only required as part of the runtime guarantees (for heap_address in addressable memory), I think it would make sense to put this back in get_heap_address, but also means we then have InterpreterState::get_heap_address and InterpreterState::heap_address which do something different.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 04 2021 at 20:03):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 04 2021 at 20:03):

afonso360 created PR review comment:

I'm not too happy with this interface as a way to register a VMContext struct.

We need to have a way to address this struct, and currently we only have Stack and Heap addresses. Adding a whole new region just for VMContext structs seems a bit much, but this is also not very clear.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 04 2021 at 20:05):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 04 2021 at 20:10):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2021 at 03:53):

afonso360 deleted PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2021 at 11:16):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2021 at 21:55):

afonso360 updated PR #3302 from interp-heap to main.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 19:14):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 19:14):

cfallin created PR review comment:

At a high level, I'm not sure I fully grok what's going on here: the heap base is itself an address with a heap and offset component? In other words, do heaps resolve into regions within other heaps? If so, could you add a comment here (or a toplevel one somewhere) describing how the resolution works?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 19:14):

cfallin created PR review comment:

Should this be != 0? (I haven't fully internalized the interpreter state data structures so I could be misunderstanding what frame_stack actually means.)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 19:14):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 19:14):

cfallin created PR review comment:

Is there a reason for the trailing underscore here? Otherwise static_heap_i64 seems fine to me.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2021 at 19:14):

cfallin created PR review comment:

Ah -- does this have to do with the maximum test identifier length? If so, just choosing shorter names (rather than truncating them) seems like a good solution.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 26 2022 at 17:45):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 26 2022 at 17:45):

afonso360 created PR review comment:

Oh wow, yes it should. And the branches in the if are also swapped. :sweat_smile:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2022 at 13:00):

afonso360 updated PR #3302 from interp-heap to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2022 at 13:02):

afonso360 updated PR #3302 from interp-heap to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2022 at 13:09):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2022 at 13:09):

afonso360 created PR review comment:

the heap base is itself an address with a heap and offset component?
Well, yes it could be. Resolving global values means that that address could be anywhere in memory, even in the stack.

In other words, do heaps resolve into regions within other heaps?
Yes, we can have multiple CLIF Heaps that resolve to the same heap in the interpreter.

This function was quite a bit confusing because we used the same Heap type to mean both a CLIF Heap and a Interpreter Heap. I've now separated those two into separate types, and the function now
only does CLIF Heap resolution.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2022 at 13:10):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2022 at 13:11):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 27 2022 at 13:11):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2022 at 16:36):

cfallin created PR review comment:

If I understand this right, it is building a vmctx out of a linear layout of (base, length) tuples, but don't the heap directives at least for the runtests allow us to put the base and length pointers at arbitrary vmctx offsets? Do we need to consider that here? (If not, a comment would help!)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2022 at 16:36):

cfallin created PR review comment:

Do we need to check here that the length of the backing vec is at least as large as the size that was claimed in the ; heap: ... directives, or somehow connect to that? Or is there an invariant that we will extend it if needed?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2022 at 16:36):

cfallin created PR review comment:

s/its/it's/

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2022 at 16:36):

cfallin created PR review comment:

s/essential/essentially/

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2022 at 16:36):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2022 at 16:36):

cfallin created PR review comment:

stray type annotation in comment -- remove?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2022 at 16:36):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2022 at 14:18):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2022 at 14:18):

afonso360 created PR review comment:

We had that design at the beginning but eventually settled on having the annotations in the ; heap directive but forcing them to a linear layout of tuples. This is enforced when parsing the heap directives.

I've added a comment clarifying this.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2022 at 14:36):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2022 at 14:36):

afonso360 created PR review comment:

This function here is somewhat independent of the testing infrastructure. And the interpreter doesn't really care about the size of the backing memory, since out of bounds writes/reads generate a trap.

The memory is allocated by the RuntestEnvironment struct which is the same struct that parses the heap directives. From there we just pass that memory directly into the interpreter, this happens in test_interpret.rs that's where we enforce that the heaps have the same size as the directives.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2022 at 14:37):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2022 at 14:39):

afonso360 updated PR #3302 from interp-heap to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2022 at 16:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2022 at 16:05):

cfallin merged PR #3302.

Last updated: Nov 22 2024 at 17:03 UTC