WebAssembly / binaryen

Optimizer and compiler/toolchain library for WebAssembly
Apache License 2.0
7.41k stars 733 forks source link

Inlining breaks asyncify #4681

Open pmatos opened 2 years ago

pmatos commented 2 years ago

While reading @kripken 's post on asyncify, I gave the test case shown a go:


  (memory 1 1)
  (import "spectest" "print" (func $print (param i32)))
  (import "asyncify" "start_unwind" (func $asyncify_start_unwind (param i32)))
  (import "asyncify" "stop_unwind" (func $asyncify_stop_unwind))
  (import "asyncify" "start_rewind" (func $asyncify_start_rewind (param i32)))
  (import "asyncify" "stop_rewind" (func $asyncify_stop_rewind))
  (global $sleeping (mut i32) (i32.const 0))
  (start $runtime)
  (func $main
    (call $print (i32.const 1))
    (call $sleep)
    (call $print (i32.const 3))
  )
  (func $sleep
    (if
      (i32.eqz (global.get $sleeping))
      (block
        ;; Start to sleep.
        (global.set $sleeping (i32.const 1))
        (i32.store (i32.const 16) (i32.const 24))
        (i32.store (i32.const 20) (i32.const 1024))
        (call $asyncify_start_unwind (i32.const 16))
      )
      (block
        ;; Resume after sleep.
        (call $asyncify_stop_rewind)
        (global.set $sleeping (i32.const 0))
      )
    )
  )
  (func $runtime
    ;; Call main the first time, let the stack unwind.
    (call $main)
    (call $asyncify_stop_unwind)
    ;; We could do anything we want around here while
    ;; the code is paused!
    (call $print (i32.const 2))
    ;; Set the rewind in motion.
    (call $asyncify_start_rewind (i32.const 16))
    (call $main)
  )
)

I was quite surprised that after:

$ bin/wasm-opt -o output.wat input.wat -O2 --asyncify -S
$ bin/wasm-shell output.wat                             
BUILDING MODULE [line: 1]
1 : i32
3 : i32
2 : i32
1 : i32
3 : i32

Bisecting shows that the first bad commit is https://github.com/WebAssembly/binaryen/commit/1a6efdb4233a077bc6e5e8a340baf5672bb5bced . The title of the issue is an assumption due to the description in the first bad commit.

kripken commented 2 years ago

Looks like that additional inlining does lead to issues, yes. The blogpost mentions the relevant reason:

One thing you may notice if you read the instrumented code is that runtime and sleep are not instrumented by Asyncify, as it assumes any function calling its API is “runtime” code - the code that controls when to unwind and rewind, and in particular, unwinding must stop when it reaches there!

We need some way to know when not to instrument a function, and use the presence of API calls for that - which is not great. In particular it can break due to inlining. Ideally we'd mark all the things the runtime calls as noinline, or we'd link in the runtime after optimizing, or something like that. However both of those techniques don't have good support, so in practice, I think disabling the inliner is necessary when using Asyncify with the runtime code linked in. This should probably be documented better...

kripken commented 2 years ago

We should also probably add a flag to disable inlining entirely. Atm, all max values need to be set to zero using something like -aimfs=0 -fimfs=0 -ocimfs=0.