binaryen icon indicating copy to clipboard operation
binaryen copied to clipboard

Inlining breaks asyncify

Open pmatos opened this issue 3 years ago • 2 comments

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.

pmatos avatar May 23 '22 09:05 pmatos

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 avatar May 23 '22 18:05 kripken

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.

kripken avatar May 23 '22 18:05 kripken