Stream: git-wasmtime

Topic: wasmtime / issue #3136 Adding heap support in filetest in...

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

afonso360 opened issue #3136:

Hey!

I've been thinking about adding heap (and stack) support for the interpreter, but I think it would be a good idea to have a runtest test suite before doing that.

To that end, I've been thinking about how to integrate heaps with runtests, and this is what I've come up with, but would like to discuss and gather some feedback before proceeding with implementation.

Benefit

We get better test coverage both for the interpreter and all cranelift backends.

Implementation

My idea would be to allocate each heap in a separate Vec, and pass pointers to it in a vmctx struct. We would do this
in the trampoline whenever a function has heap annotations and a vmctx argument.

function %heap_load_store_test(i64 vmctx, i32, i32) -> i32 {
    gv0 = vmctx
    gv1 = load.i64 notrap aligned gv0+0
    gv2 = load.i64 notrap aligned gv0+8
    gv3 = load.i64 notrap aligned gv0+16
    heap0 = static gv1, min 0x1000, bound 0x1_0000_0000, offset_guard 0
    heap1 = dynamic gv2, bound gv3, offset_guard 0

block0(v0: i64, v1: i32, v2: i32):
    v3 = iconst.i64 0
    v4 = heap_addr.i64 heap0, v3, 4

    store.i32 v1, v4
    v6 = load.i32 v4

    v7 = iadd.i32 v6, v2
    return v7
}
; run: %heap_load_store_test(1, 2) == 3

In this example we have two heaps, so we would allocate a struct that holds info about the location of these heaps
and pass its pointer as the first argument to the function

The first heap (static) is preallocated with the minimum size of 4096 bytes. I don't think we have a way to resize
it, so we would always trap on accesses past the minimum size. This heap only requires the base pointer, so
we only allocate 8 bytes of the vmctx struct for the base pointer.

The second heap (dynamic) requires two pointers (base + bounds), so we reserve 16 bytes in the vmctx struct and pass
the base and bounds pointers. Here we can reserve a default size like 16KB. Again we don't have a way to resize it dynamically, so we just don't do that.

I think the syntax for what I described above is correct in the example function, but it might not be, so take it with
a grain of salt.

I don't like that with this design the runtest arguments no longer match what ends up in the function. But if this is
restricted to heap tests it shouldn't be too bad.

Another note is that although we don't have to always use the first argument for vmctx, it would be nicer if we
restricted (by design or by convention) it that way so that things would be less confusing.

Questions:

• What happens when we have heap annotations but no vmctx argument? My opinion here is that we just don't allocate anything.
• Is there a way for us to grow heaps in the middle of a test? How is this done in other virtual machines?

Alternatives

We can not do this and implement heap tests in the interpreter as regular rust tests. This would be a lot more verbose and would lose the benefit of testing other backends.

cc: @cfallin, @abrown

Wasmtime GitHub notifications bot (Aug 04 2021 at 15:23):

afonso360 commented on issue #3136:

Another alternative to this can also be to provide access to the heaps via a global symbol, which would potentially solve the mismatch between run arguments and function arguments for heap tests.

Wasmtime GitHub notifications bot (Aug 04 2021 at 16:11):

abrown commented on issue #3136:

I think I get the main point but I have questions about some of the details:

• would this "heap setup" code become a part of the test interpret infrastructure or some new test target?
• is the "heap setup" code supposed to read and understand the function prelude (the gv* and heap* definitions) and set things up appropriately? Are there restrictions to what the test writer can write in the function prelude beyond the current CLIF restrictions?

Wasmtime GitHub notifications bot (Aug 04 2021 at 16:12):

bjorn3 commented on issue #3136:

I think the heap setup code should read special directives similar to ; run:.

Wasmtime GitHub notifications bot (Aug 04 2021 at 18:19):

afonso360 commented on issue #3136:

would this "heap setup" code become a part of the test interpret infrastructure or some new test target?

The idea would be to keep the same targets that we have right now, and just extending their capabilities.

I thought that we could keep changes contained to the SingleFunctionCompiler, but we would also need to change the test interpret to create and pass the heap info when creating the interpreter.

is the "heap setup" code supposed to read and understand the function prelude (the gv* and heap* definitions) and set things up appropriately?

Yes, my idea would be to read the Heap structure from the Function that is passed into the SingleFunctionCompiler, I don't think we would need to understand gv* values, or do anything special with them.

I thought about it this way mostly so we don't have to introduce any new parsing steps, we would use the parsed info that's already included in the Function. However, this also has some issues, where we have to infer heap size from the min size, and come up with a default for dynamic heaps, which isn't great. (see bjorn3's suggestion below)

Are there restrictions to what the test writer can write in the function prelude beyond the current CLIF restrictions?

I think you wouldn't be able to write static heaps with a huge min value, because this would cause a big allocation and potentially OOM. But otherwise I don't see anything that we wouldn't be able to implement correctly or ignore.

By ignore here I mean for example the offset_guard feature or the bound feature, which we can't really interact with in any meaningful way.

I think the heap setup code should read special directives similar to ; run:.

I think this also has some positives in that it is a bit more clear that the environment is providing something extra to the function under test. It is also a bit more clear where we won't have to infer heap size from the existing declarations.

It introduces an extra parsing step, but it looks like it wouldn't be too hard to integrate with the current infrastructure.

I think we can have a similar syntax to what we already have in the function prelude, but only include heap type and size:

; heap: static size=0x1000
; heap: dynamic size=16
````

What I also like about this is that it allows us to extend to other features. If we later for example want to test symbols we could also add a `; symbol:` annotation to expose a symbol to the function (but that's probably a future discussion).
~~~

Wasmtime GitHub notifications bot (Aug 06 2021 at 10:35):

afonso360 commented on issue #3136:

I have an initial implementation in #3154.

Are there restrictions to what the test writer can write in the function prelude beyond the current CLIF restrictions?

Implementing this revealed that while we do not have additional restrictions to function preludes, in the current form we essentially reserve the first argument being a vmctx for these tests.

Wasmtime GitHub notifications bot (Aug 24 2021 at 16:28):

abrown closed issue #3136:

Hey!

I've been thinking about adding heap (and stack) support for the interpreter, but I think it would be a good idea to have a runtest test suite before doing that.

To that end, I've been thinking about how to integrate heaps with runtests, and this is what I've come up with, but would like to discuss and gather some feedback before proceeding with implementation.

Benefit

We get better test coverage both for the interpreter and all cranelift backends.

Implementation

My idea would be to allocate each heap in a separate Vec, and pass pointers to it in a vmctx struct. We would do this
in the trampoline whenever a function has heap annotations and a vmctx argument.

function %heap_load_store_test(i64 vmctx, i32, i32) -> i32 {
    gv0 = vmctx
    gv1 = load.i64 notrap aligned gv0+0
    gv2 = load.i64 notrap aligned gv0+8
    gv3 = load.i64 notrap aligned gv0+16
    heap0 = static gv1, min 0x1000, bound 0x1_0000_0000, offset_guard 0
    heap1 = dynamic gv2, bound gv3, offset_guard 0

block0(v0: i64, v1: i32, v2: i32):
    v3 = iconst.i64 0
    v4 = heap_addr.i64 heap0, v3, 4

    store.i32 v1, v4
    v6 = load.i32 v4

    v7 = iadd.i32 v6, v2
    return v7
}
; run: %heap_load_store_test(1, 2) == 3

In this example we have two heaps, so we would allocate a struct that holds info about the location of these heaps
and pass its pointer as the first argument to the function

The first heap (static) is preallocated with the minimum size of 4096 bytes. I don't think we have a way to resize
it, so we would always trap on accesses past the minimum size. This heap only requires the base pointer, so
we only allocate 8 bytes of the vmctx struct for the base pointer.

The second heap (dynamic) requires two pointers (base + bounds), so we reserve 16 bytes in the vmctx struct and pass
the base and bounds pointers. Here we can reserve a default size like 16KB. Again we don't have a way to resize it dynamically, so we just don't do that.

I think the syntax for what I described above is correct in the example function, but it might not be, so take it with
a grain of salt.

I don't like that with this design the runtest arguments no longer match what ends up in the function. But if this is
restricted to heap tests it shouldn't be too bad.

Another note is that although we don't have to always use the first argument for vmctx, it would be nicer if we
restricted (by design or by convention) it that way so that things would be less confusing.

Questions:

• What happens when we have heap annotations but no vmctx argument? My opinion here is that we just don't allocate anything.
• Is there a way for us to grow heaps in the middle of a test? How is this done in other virtual machines?

Alternatives

We can not do this and implement heap tests in the interpreter as regular rust tests. This would be a lot more verbose and would lose the benefit of testing other backends.

cc: @cfallin, @abrown

Last updated: Nov 22 2024 at 17:03 UTC