Wasmtime GitHub notifications bot (Aug 06 2021 at 10:32): Wasmtime GitHub notifications bot (Aug 06 2021 at 10:32):afonso360 opened PR #3154 from filetest-heaps to main:
Hey, this is an initial version of what was discussed in #3136. The design and whether or not its a good idea are still up for debate, but I thought having an initial implementation would help visualizing how this would turn out.
The interpreter doesn't support multiple heaps yet, so we will need to alter
test interpretonce it does.
Wasmtime GitHub notifications bot (Aug 06 2021 at 10:49):afonso360 edited PR #3154 from filetest-heaps to main:
Hey, this is an initial version of what was discussed in #3136. The design and whether or not its a good idea are still up for debate, but I thought having an initial implementation would help visualizing how this would turn out.
The interpreter doesn't support multiple heaps yet, so we will need to alter
test interpretonce it does.Closes #3136
Wasmtime GitHub notifications bot (Aug 06 2021 at 11:20):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Aug 06 2021 at 11:20):bjorn3 created PR review comment:
Maybe also add information about what is stored at which offset from the vmctx pointer instead of keeping it implicit? For example:
; heap: static, size=0x1000, ptr=vmctx+0 ; heap: dynamic, size=0x1000, ptr=vmctx+16, bound=vmctx+24or
; heap: static, size=0x1000, ptr=*(vmctx+0) ; heap: dynamic, size=0x1000, ptr=*(vmctx+16), bound=*(vmctx+24)
Wasmtime GitHub notifications bot (Aug 08 2021 at 21:53):afonso360 updated PR #3154 from filetest-heaps to main.
Wasmtime GitHub notifications bot (Aug 11 2021 at 17:17):afonso360 updated PR #3154 from filetest-heaps to main.
Wasmtime GitHub notifications bot (Aug 11 2021 at 17:19):afonso360 updated PR #3154 from filetest-heaps to main.
Wasmtime GitHub notifications bot (Aug 16 2021 at 22:29):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 16 2021 at 22:29):abrown created PR review comment:
- Should this be handling overflow?
- And what changes here if/when #3187 is merged?
- And are we going to reuse this struct when we fuzz CLIF + heaps?
Wasmtime GitHub notifications bot (Aug 16 2021 at 22:29):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 16 2021 at 22:29):abrown created PR review comment:
I think the heaps may be laid out sequentially in memory but I'm not sure about the pointers: if the user writes the following,
; heap: static, size=0x1000, ptr=vmctx+0, bound=vmctx+16 ; heap: dynamic, size=0x1000, ptr=vmctx+8, bound=vmctx+24Then do bad things happen?
Wasmtime GitHub notifications bot (Aug 16 2021 at 22:29):abrown created PR review comment:
It seems like here, or somewhere, we should be checking the
Functionsignature to see if its arguments begin with avmctxand, if not, emit a useful error like "you need to change your function signature to..."
Wasmtime GitHub notifications bot (Aug 16 2021 at 22:29):abrown created PR review comment:
Perhaps this can't be done... enforced by the parser? Maybe the docs should say something about "must be laid out sequentially..."
Wasmtime GitHub notifications bot (Aug 17 2021 at 09:53):afonso360 created PR review comment:
If the layout is different from what we expect, we throw an error.
I don't like that this interface gives the user the impression that they can change the offsets. But I think its better than being implicit.
Maybe the docs should say something about "must be laid out sequentially..."
Yeah, I'll update the docs.
Wasmtime GitHub notifications bot (Aug 17 2021 at 09:53):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 17 2021 at 10:18):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 17 2021 at 10:18):afonso360 created PR review comment:
Should this be handling overflow?
According to the rust docs, this should always be safe. But even in the event where this goes wrong I would expect the tests to fail with a SIGSEGV. It might be better to change to the
wrapping_addvariant of this function.And what changes here if/when cranelift: Add stack support to the interpreter with virtual addresses #3187 is merged?
We won't be able to do this the same way with the interpreter, it will probably need some refactoring to handle the separate address space. Off the top of my head, I think we can do something like the following:
- Allocate the heaps as usual
- Register each heap with the interpreter
- Request the heap pointers from the interpreter
- Build the
context_structwith those pointers.And are we going to reuse this struct when we fuzz CLIF + heaps?
I hadn't thought about that, but it actually is an easy way to pass all of this info while fuzzing to the interpreter and the actual compiled target.
For fuzzing I'd like to relax the requirement that
vmctxis the first argument, but that shouldn't be an issue, since we only check for that in the parser.We may also need to coordinate the heaps with the generated
global_valuesin each function. Otherwise I think the fuzzer may have issues generating the correct offsets to get valid heap accesses.I think we should be OK reusing this for fuzzing with the changes to generate the valid pointers for the interpreter.
Wasmtime GitHub notifications bot (Aug 17 2021 at 10:20):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Aug 23 2021 at 13:49):afonso360 updated PR #3154 from filetest-heaps to main.
Wasmtime GitHub notifications bot (Aug 23 2021 at 13:50):afonso360 requested abrown for a review on PR #3154.
Wasmtime GitHub notifications bot (Aug 23 2021 at 23:41):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 24 2021 at 08:05):afonso360 updated PR #3154 from filetest-heaps to main.
Wasmtime GitHub notifications bot (Aug 24 2021 at 16:28):abrown merged PR #3154.
Last updated: Dec 23 2024 at 12:05 UTC