Stream: git-wasmtime

Topic: wasmtime / PR #3187 cranelift: Add stack support to the i...

Wasmtime GitHub notifications bot (Aug 15 2021 at 18:13):

afonso360 opened PR #3187 from interperter-stack-vaddr to main:

Hey,

This PR is #3164 with an additional Virtual Addressing scheme that was discussed as a better alternative to returning the real addresses.

The virtual addresses are split into 4 regions (stack, heap, tables and global values), and the address itself is composed of an entry field and an offset field. In general the entry field corresponds to the instance of the resource (e.g. table5 is entry 5) and the offset field is a byte offset inside that entry.

There is one exception to this which is the stack, where due to only having one stack, the whole address is an offset field.

The number of bits in entry vs offset fields is variable with respect to the region and the address size (32bits vs 64bits). This is done because with 32 bit addresses we would have to compromise on heap size, or have a small number of global values / tables. With 64 bit addresses we do not have to compromise on this, but we need to support 32 bit addresses.

cc: @abrown @cfallin

Wasmtime GitHub notifications bot (Aug 15 2021 at 18:14):

afonso360 edited PR #3187 from interperter-stack-vaddr to main:

Hey,

This PR is #3164 with an additional Virtual Addressing scheme that was discussed as a better alternative to returning the real addresses.

The virtual addresses are split into 4 regions (stack, heap, tables and global values), and the address itself is composed of an entry field and an offset field. In general the entry field corresponds to the instance of the resource (e.g. table5 is entry 5) and the offset field is a byte offset inside that entry.

There is one exception to this which is the stack, where due to only having one stack, the whole address is an offset field.

The number of bits in entry vs offset fields is variable with respect to the region and the address size (32bits vs 64bits). This is done because with 32 bit addresses we would have to compromise on heap size, or have a small number of global values / tables. With 64 bit addresses we do not have to compromise on this, but we need to support 32 bit addresses.

cc: @abrown @cfallin

Wasmtime GitHub notifications bot (Aug 15 2021 at 18:55):

afonso360 updated PR #3187 from interperter-stack-vaddr to main.

Wasmtime GitHub notifications bot (Aug 15 2021 at 19:22):

afonso360 updated PR #3187 from interperter-stack-vaddr to main.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:06):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:06):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:06):

abrown created PR review comment:

nit: I think it might be helpful here to just document all of the decisions made in this file in a table:

address size	address kind	region value (2 bits)	entry bits (#)	offset bits (#)
32	Stack	0b00	0	30
32	Heap	0b01	2	28
...	...	...	...	...
64	Stack	0b00	0	62
...	...	...	...	...

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:06):

abrown created PR review comment:

I should really create a helper functions for this common "just run the interpreter on this CLIF" case. Don't worry about it for this PR but eventually I'll do something like that.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:20):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:20):

cfallin created PR review comment:

Would it make sense to track the current sum (basically a "frame pointer") in the interpreter state, rather than compute this dynamically? I could see a worst-case quadratic cost here especially if the fuzzer gets too devious with its test-case generation.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:20):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:20):

cfallin created PR review comment:

Is there a particular reason that we need to pull these specific errors out of the more general "heap out-of-bounds" error below? It seems to me that at least the first two (OOB load, OOB store) are canonical OOB cases, and "invalid address" would make some sense if we had a more general notion of address-space maps in Wasm, but I think the complexity and confusion presented in the API here is not really worth it. @abrown thoughts?

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:20):

cfallin created PR review comment:

Converting to a raw pointer then accessing it with unsafe feels a little bit unnecessary to me -- not that it's hard to see the safety here, since the code is very simple, but it entrenches a pattern that could go wrong later if it gets much more complicated.

Instead perhaps could we:

• Get a slice &mut [u8] of raw memory bytes from whatever region/object we're accessing (so the underlying interpreter-state for the stack, any wasm memories, etc., is just a Vec<u8>)
• Slice that into the number of bytes we need at the offset we need
• Provide accessors on DataValue that read and write from &[u8] / &mut [u8]?

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:52):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:52):

abrown created PR review comment:

I guess that makes sense: InvalidAddress is also really just an OOB. Can we get away with StackOutOfBounds here (to match the other ...OutOfBounds below)?

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:55):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2021 at 17:55):

abrown created PR review comment:

I think @afonso360 was just going along with what existed previously in DataValue (those unsafe methods are necessary for interacting with trampolines). I agree that it would probably be better to just some safe versions of those functions to DataValue.

Wasmtime GitHub notifications bot (Aug 16 2021 at 18:03):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2021 at 18:03):

cfallin created PR review comment:

Indeed, it was a reasonable approach given the APIs that existed! The unsafe accessors on DataValue can certainly stay for the other use-cases (and perhaps a future refactor can centralize those into e.g. unsafe { std::slice::from_raw_parts(...) } in the trampoline-adjacent code and then use of safe APIs on DataValue, but that's a separate conversation!).

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

afonso360 submitted PR review.

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

afonso360 created PR review comment:

I didn't use HeapOutOfBounds because i thought it would be heap specific, but we can fold all other memory accesses / invalid address into that.

We can also specialize a StackOutOfBounds for stack_loads/stores.

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

afonso360 submitted PR review.

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

afonso360 created PR review comment:

Yeah, sounds like a good idea. I can probably fix that when I implement heap access, ill probably have to add a number of tests similar to these ones.

Wasmtime GitHub notifications bot (Aug 21 2021 at 15:55):

afonso360 updated PR #3187 from interperter-stack-vaddr to main.

Wasmtime GitHub notifications bot (Aug 23 2021 at 09:30):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 23 2021 at 09:30):

afonso360 created PR review comment:

It looks like read_value_from/write_value_to are only used in the interpreter, so I replaced them with write_to_slice and read_from_slice. It looks like we don't use them anywhere else, but if we need them we have a version that uses slice::from_raw_parts in 5c594c4

Wasmtime GitHub notifications bot (Aug 23 2021 at 09:31):

afonso360 updated PR #3187 from interperter-stack-vaddr to main.

Wasmtime GitHub notifications bot (Aug 23 2021 at 09:42):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 23 2021 at 09:42):

afonso360 created PR review comment:

Oh, it looks like we duplicate the write_value_to in function_runner.rs

https://github.com/bytecodealliance/wasmtime/blob/b2bcdd13ecb87e9c7cdae9cead1e490bcd47e4fe/cranelift/filetests/src/function_runner.rs#L235-L247

Wasmtime GitHub notifications bot (Aug 23 2021 at 10:12):

afonso360 updated PR #3187 from interperter-stack-vaddr to main.

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

afonso360 updated PR #3187 from interperter-stack-vaddr to main.

Wasmtime GitHub notifications bot (Aug 23 2021 at 10:16):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 23 2021 at 10:16):

afonso360 created PR review comment:

Since we had this duplicated, I thought it would be best to merge it all into DataValue right now before we have some hard to track down memory layout mismatch.

Wasmtime GitHub notifications bot (Aug 23 2021 at 10:17):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 24 2021 at 08:04):

afonso360 updated PR #3187 from interperter-stack-vaddr to main.

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

abrown merged PR #3187.

Last updated: Dec 23 2024 at 13:07 UTC