Fuzz bug: interpretation of `return_call` is not correct. about binaryen HOT 16 CLOSED

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.

from binaryen.

Sorry, looks like the field is called throws_.

from binaryen.

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?

from binaryen.

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 🐛

from binaryen.

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

from binaryen.

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

from binaryen.

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 😱

from binaryen.

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.

from binaryen.

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?

from binaryen.

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?

from binaryen.

@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?

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.

from binaryen.

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.

from binaryen.

Do J2Wasm or other toolchains currently generate return_calls by default?

from binaryen.

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

from binaryen.

OCaml optimizes tail calls when it does not change the semantics. So, my compiler changes all call + return into return_call except inside try blocks.

I don't know of any language which allows tail-call in the scope of an exception handler. And I don't see any useful optimization which would move a tail-call within the scope of an exception handler. So, I think it's going to be exceedingly rare to find a return_call within a try block.

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

[...] 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.

Oh, indeed, it is not correct to turn a return_call inside a try block into a call + br. I did not thought about that. But if there is no return_call inside a try block anywhere in the initial code, I believe the current implementation of inlining is correct. Maybe you could just disable inlining when it might not be correct? That is, you do not inline a return_call within a try block and you bails out if you encounter one in the body of the inlined function.

@aheejin

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?

Well, the semantics of a return_call is to pop all the exception handlers and then to perform the call. But popping the exception handlers is also what branching outside a try block does.

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?

Inlining should not change the semantics of the code, so one should not need to recompute the effect.

To fix effects.h, would it be enough to set parent.throws_ not only when parent.tryDepth == 0 but also when parent.tryDepth > 0 && curr->isReturn?

from binaryen.

Fixed by #6451

from binaryen.