Stream: git-wasmtime

Topic: wasmtime / PR #2501 2499: First pass on TableOps fuzzer g...

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:30):

dhaynespls edited PR #2501 from dhaynespls/2499-tableops-to-wasm-encoder to main:

This PR migrates the TableOps fuzzer generator from the old string concatenation strategy to utilizing wasm_encoder to generate a wasm binary for us.

Tests updated accordingly.

Closes #2499

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:34):

dhaynespls updated PR #2501 from dhaynespls/2499-tableops-to-wasm-encoder to main:

This PR migrates the TableOps fuzzer generator from the old string concatenation strategy to utilizing wasm_encoder to generate a wasm binary for us.

Tests updated accordingly.

Closes #2499

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:53):

dhaynespls submitted PR Review.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:53):

dhaynespls created PR Review Comment:

cc @fitzgen I'm like .. 99% sure this new output is doing something different than your original expected output but I'm not exactly sure how to use the library to produce a concise copy.

(other than the order of sections which I followed https://webassembly.github.io/spec/core/binary/modules.html#binary-module for per the docs)

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen created PR Review Comment:

The 0th function is the gc function import (the function index space goes [import function 0, import function 1, import function 2, ..., defined function 0, defined function 1, defined function 2, ...]) so this index should be 1 here.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen created PR Review Comment:

These two instructions' order should be swapped. The result of the table.get is the thing we want to drop, so we want to do the table.get which pushes its result on top of the Wasm stack, and then do a drop, which pops the value off and drops it.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen created PR Review Comment:

You'll also need to define the type for this imported function in the type section (and make sure you aren't mixing up type indices for the gc and run functions' signatures):

types.function(vec![], vec![]);

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen created PR Review Comment:

Similarly, these need to be swapped around as well so that the table.get comes before the {local,table}.set.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen created PR Review Comment:

Here is the fully elaborated/desugared WAT that we should expect:

  (type (;0;) (func))
  (type (;1;) (func (param externref externref)))
  (import "" "gc" (func (;0;) (type 0)))
  (func (;1;) (type 1) (param externref externref)
    call 0
    table.get 0
    drop
    local.get 2
    table.set 1
    table.get 4
    table.set 3)
  (table (;0;) 10 externref)
  (export "run" (func 1)))

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen created PR Review Comment:

Heads up: we just published 0.2 recently, and may have 0.3 out in the next day or so.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:58):

fitzgen created PR Review Comment:

See my comments below :)

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:59):

dhaynespls submitted PR Review.

Wasmtime GitHub notifications bot (Dec 14 2020 at 18:59):

dhaynespls created PR Review Comment:

oh awesome you just reviewed I'll resolve this one and use your comments

Wasmtime GitHub notifications bot (Dec 14 2020 at 19:04):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Dec 14 2020 at 19:04):

fitzgen created PR Review Comment:

Also, the table.{get,set} should take the table index as a Wasm stack operand (created via i32.const) not as an immediate that is part of the instruction (which specifies which table is being used):

// Stack: []
func.instruction(Instruction::I32Const(*y);
// Stack: [y]
func.instruction(Instruction::I32Const(*x));
// Stack: [y, x]
func.instruction(Instruction::TableGet { table: 0 });
// Stack: [y, ref0]
func.instruction(Instruction::TableSet { table: 0 });
// Stack: []

Wasmtime GitHub notifications bot (Dec 14 2020 at 19:05):

fitzgen edited PR Review Comment.

Wasmtime GitHub notifications bot (Dec 14 2020 at 19:05):

fitzgen edited PR Review Comment.

Wasmtime GitHub notifications bot (Dec 14 2020 at 19:06):

fitzgen edited PR Review Comment.

Wasmtime GitHub notifications bot (Dec 14 2020 at 19:06):

fitzgen edited PR Review Comment.

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:22):

dhaynespls updated PR #2501 from dhaynespls/2499-tableops-to-wasm-encoder to main:

This PR migrates the TableOps fuzzer generator from the old string concatenation strategy to utilizing wasm_encoder to generate a wasm binary for us.

Tests updated accordingly.

Closes #2499

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:23):

dhaynespls submitted PR Review.

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:23):

dhaynespls created PR Review Comment:

Thanks for clarifying this, I was confused what table: was referring to (table index or value to put/get into a table).

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:23):

dhaynespls submitted PR Review.

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:23):

dhaynespls created PR Review Comment:

OH that makes total sense -- I wasn't thinking in terms of putting things onto a stack first before using them

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:25):

dhaynespls created PR Review Comment:

In the function body I needed to add the table index to your expected output ie. table.get 0

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:25):

dhaynespls submitted PR Review.

Wasmtime GitHub notifications bot (Dec 15 2020 at 18:26):

dhaynespls updated PR #2501 from dhaynespls/2499-tableops-to-wasm-encoder to main:

This PR migrates the TableOps fuzzer generator from the old string concatenation strategy to utilizing wasm_encoder to generate a wasm binary for us.

Tests updated accordingly.

Closes #2499

Wasmtime GitHub notifications bot (Dec 15 2020 at 21:02):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Dec 15 2020 at 21:02):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Dec 15 2020 at 21:02):

fitzgen created PR Review Comment:

Nitpick: there's a typo here ("a a") so we might as well wordsmith this a tiny bit while we're here:

    /// Serialize this module into a Wasm binary.

Wasmtime GitHub notifications bot (Dec 15 2020 at 21:02):

fitzgen created PR Review Comment:

This will let us avoid the wasm->wat disassembly (and thus have higher fuzzing throughput) unless logging is enabled:

        let wasm = ops.to_wasm_binary();
        log_wasm(&wasm);

Wasmtime GitHub notifications bot (Dec 15 2020 at 22:53):

dhaynespls updated PR #2501 from dhaynespls/2499-tableops-to-wasm-encoder to main:

This PR migrates the TableOps fuzzer generator from the old string concatenation strategy to utilizing wasm_encoder to generate a wasm binary for us.

Tests updated accordingly.

Closes #2499

Wasmtime GitHub notifications bot (Dec 16 2020 at 00:38):

dhaynespls updated PR #2501 from dhaynespls/2499-tableops-to-wasm-encoder to main:

This PR migrates the TableOps fuzzer generator from the old string concatenation strategy to utilizing wasm_encoder to generate a wasm binary for us.

Tests updated accordingly.

Closes #2499

Wasmtime GitHub notifications bot (Dec 17 2020 at 21:47):

alexcrichton merged PR #2501.

Last updated: Dec 23 2024 at 12:05 UTC