Fuzz bug: interpretation of `return_call` is not correct.

[tlively]

This module behaves differently in our interpreter and in v8:

(module
 (import "fuzzing-support" "log-i32" (func $log (param i32)))
 (tag $t (param))

 (func $throw
  (throw $t)
 )

 (func $try-retcall
  (try $label$3
   (do
    (return_call $throw)
   )
   (catch $t)
  )
 )

 (func $test (export "test")
  (call $try-retcall)
  (call $log
   (i32.const 0)
  )
 )
)

Since we interpret return_call as call + return, the thrown exception is caught by the try and $test continues on to call $log. The correct behavior is that $try-retcall should return and then $throw should be called, so $test should propagate the exception and never call $log.

[kripken]

Wait, what? A return_call nested in a try-catch can throw without the try-catch catching it? Then I'm pretty sure we have several bugs around that, from effects.h to specific passes, because we assume that structured control flow implies what can be caught: the return_call is inside a try-catch, so we consider it catch-able.

The spec says Tail calls behave like a combination of return followed by a respective call which seems ambiguous at best, and it does not mention the interaction with try-catch anywhere that I can see.

cc @aheejin - did this EH/tail-call interaction come up on the EH side?

[tlively]

Yes! The return gets you out of the try and if the subsequent call throws, you're no longer in the try to catch it. Let's fix some bugs 🐛

[kripken]

Not that I doubt you 😄 but this is a huge gotcha in the spec - where is it documented?

[tlively]

Here's where the tail calling semantics are described: https://webassembly.github.io/tail-call/core/exec/instructions.html#tail-invocation-of-function-address-a

The important part is that it pops everything up to and including the current call frame. This will pop any handlers off the stack.

Here's an issue where this was explicitly discussed: WebAssembly/exception-handling#249

[kripken]

I see, thanks...

To me this is extremely surprising as it breaks the basic nesting property:

(func $i-do-not-throw
  (try
    ..body..
    (catch_all)
  )
)

That try is the entire function body, and the catch-all seems like it should capture any exceptions from ..body.., no matter what they are. That is how JS works:

function iDoNotThrow() {
  try {
    ..body..
  } catch (c) {}
}

But in wasm, a "clever" use of return_call in place of ..body.. can break that expectation. To me this is pretty shocking, personally, aside from the work it will take to fix in Binaryen (which I don't have an immediate estimate of).

By "clever" I mean that inlining code that contains a return_call becomes dangerous here, for example 😱

[vouillon]

By "clever" I mean that inlining code that contains a return_call becomes dangerous here, for example 😱

I think that's ok, since this return_call is going to be turned into a call. That's inlining a return_call within a try which is dangerous.

I suspect the sane thing to do is to maintain the invariant that there is no return_call within a try, with a pass at the start which would move any such occurrence outside of the try.

[aheejin]

Ah, right, this was indeed surprising when I first discovered it, and I somehow forgot to fix it in Binaryen... Do others currently generate tail calls by default? Emscripten doesn't, but do you know about others including J2Wasm and others?

I asked a question on changing the spec here (WebAssembly/exception-handling#249 (comment)) but I don't think people would have much appetite for changing the already-half-deprecated spec at this point.

@kripken @vouillon

By "clever" I mean that inlining code that contains a return_call becomes dangerous here, for example 😱

I think that's ok, since this return_call is going to be turned into a call. That's inlining a return_call within a try which is dangerous.

Do we turn a return_call into a call when inlining? (If so, why?)

I suspect the sane thing to do is to maintain the invariant that there is no return_call within a try, with a pass at the start which would move any such occurrence outside of the try.

How can we move a return_call outside of a try without changing the semantics?

[aheejin]

We can fix effects.h so that we scan the body of try-catch_all does not contain return_call before marking it as nothrow. But yeah if inlining happens within this function this effect has to be recomputed, which I don't think we would do. Should we just mark try-catch_all as mayThrow when tail-call is enabled?

[tlively]

A try or try_table with a catch_all (that does not itself throw, in the case of try) will never throw from the perspective of the surrounding code, but it may return if it contains a return or return_call (or any other tail call variant). So I think the only effect we need to add is branchesOut, which we already do for return_call and friends.

Do we turn a return_call into a call when inlining? (If so, why?)

If the caller contains call $callee and the callee contains a return_call $f, then before inlining, the return value of $f would have been returned from the call $callee instruction. When $callee is inlined, we still need that return value to be returned to the caller. If the caller contained return_call $f after inlining, it would incorrectly return the return value of $f instead of receiving it. To fix this, we currently replace return_call with call + br $inlined-block in the inlined code, but that's not correct in the presence of EH.

I can take a look at fixing inlining to handle this correctly.

[kripken]

@aheejin

Should we just mark try-catch_all as mayThrow when tail-call is enabled?

Maybe we can add to that, to only do it when calls has been set. That is, we can take advantage of the children that have already been scanned, to see if a call exists. (Though that won't be fully optimal in case of tested try's etc.)

@tlively

A try or try_table with a catch_all (that does not itself throw, in the case of try) will never throw from the perspective of the surrounding code, but it may return if it contains a return or return_call (or any other tail call variant).

Yes to the first part, but it may still throw from the perspective of other functions calling it. So perhaps we can get away with only adding mayThrow effect on entire functions, here?

binaryen/src/ir/effects.h

Line 66 in 1125750

// Walk an entire function body. This will ignore effects that are not

But I'm not sure. We should only have very few places that compute effects in a way observable by other functions, at least, and hopefully they all call that one method.

[tlively]

Ah, makes sense. Yes, hopefully we can make a local change in that function.

Can you explain the mayThrow stuff more? I don't see that term in effects.h.

[kripken]

Sorry, looks like the field is called throws_.

[aheejin]

Do J2Wasm or other toolchains currently generate return_calls by default?

[tlively]

I don't believe j2wasm emits return_call, but it looks like @vouillon's OCaml compiler does.