Handle return calls in wasm-ctor-eval #6464
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
src/tools/wasm-ctor-eval.cpp
Outdated
| // the partially-evalled contents and make the export use that (as the | ||
| // function may be used in other places than the export, which we do not | ||
| // want to affect). | ||
| if ((localExprs.size() && func->getParams() != Type::none) || |
There was a problem hiding this comment.
Perhaps add a comment on this long if condition? Before, if was "successes > 0 but not the max" and a comment on the very next line clarified what that meant. Now, there are 3 lines with seemingly very different conditions in them and no obvious hints as to what they mean...
|
|
||
| // Success! Apply the results. | ||
| interface.applyToModule(); | ||
| return EvalCtorOutcome(results); |
There was a problem hiding this comment.
We now treat functions with block bodies and non-block bodies uniformly by wrapping non-block bodies in blocks. See line 1098: auto* block = builder.blockify(func->body);. This made putting the whole thing in a loop much simpler.
There was a problem hiding this comment.
I must be missing something here, sorry. This code here does a try-catch. I was expecting to see it replaced by another try-catch. That seems unrelated to the blockify issue you mention, unless there is a subtle connection I am missing.
There was a problem hiding this comment.
There is now only a single try-catch on lines 1109-1121 that covers all cases.
There was a problem hiding this comment.
I see, thanks! Sorry I missed that, maybe I did control-F without expanding the diff enough...
|
The fuzzer seems fairly happy with this. After 35k+ iterations, it has only found two unrelated issues. Once this looks good to you, I think we should go ahead and merge this stack of PRs. We can do it as a single commit if you still think that's a good idea - I'm more ambivalent about it. |
| (global.set $g2 | ||
| (i32.const 2) | ||
| ) | ||
| (call $import) |
There was a problem hiding this comment.
This seems to overlap wit the previous testcase, which also had partial eval of the second function. But this call was a return-call there.
There was a problem hiding this comment.
Right, in the previous test case the role of the import is to be return-called so that it actually ends up as the current func at the end of evaluation. In contrast, the import in this test is just there to abort the eval and leave the current func, $test2, partially evaluated.
|
Do you still think it's a good idea to merge these three PRs as a single commit? Or just the first two? |
|
I think we should land them all as a single commit. Yes, the downside is it's a big commit, but otherwise the internal lack of consistency risks breaking bisection in the future. Maybe I am overly influenced by knowing how important bisectability is in projects like Firefox and Chrome where you constantly bisect regressions. I suppose for us it's more rare, and we can consider each of these commits a separate "bugfix". So I don't feel strongly here, I guess. If we do want to land this as one then the |
|
Ok, I'll take a look at implementing the effects the way you sketched. |
tlively
force-pushed
the
interpret-return-call
branch
from
c8beca6
to
adb1c54
Compare
tlively
force-pushed
the
return-call-ctor-eval
branch
from
668f7ec
to
c6497ac
Compare
When an evaluated export ends in a return call, continue evaluating the return-called function. This requires propagating the parameters, handling the case that the return-called function might be an import, and fixing up local indices in case the final function has different parameters than the original function.
tlively
force-pushed
the
interpret-return-call
branch
from
adb1c54
to
f711fdc
Compare
tlively
force-pushed
the
return-call-ctor-eval
branch
from
c6497ac
to
36a6b21
Compare
As far as their surrounding code is concerned return calls are no different from normal returns. It's only from a caller's perspective that a function containing a return call also has the effects of the return-callee. To model this more precisely in EffectAnalyzer, stash the throw effect of return-callees on the side and only merge it in at the end when analyzing the effects of a full function body.
We previously interpreted return calls as calls followed by returns, but that is not correct both because it grows the size of the execution stack and because it runs the called functions in the wrong context, which can be observable in the case of exception handling. Update the interpreter to handle return calls correctly by adding a new `RETURN_CALL_FLOW` that behaves like a return, but carries the arguments and reference to the return-callee rather than normal return values. `callFunctionInternal` is updated to intercept this flow and call return-called functions in a loop until a function returns with some other kind of flow. Pull in the upstream spec tests return_call.wast, return_call_indirect.wast, and return_call_ref.wast with light editing so that we parse and validate them successfully. * Handle return calls in wasm-ctor-eval (#6464) When an evaluated export ends in a return call, continue evaluating the return-called function. This requires propagating the parameters, handling the case that the return-called function might be an import, and fixing up local indices in case the final function has different parameters than the original function. * Update effects.h to handle return calls correctly (#6470) As far as their surrounding code is concerned return calls are no different from normal returns. It's only from a caller's perspective that a function containing a return call also has the effects of the return-callee. To model this more precisely in EffectAnalyzer, stash the throw effect of return-callees on the side and only merge it in at the end when analyzing the effects of a full function body.
This is a combined commit covering multiple PRs fixing the handling of return calls in different areas. The PRs are all landed as a single commit to ensure internal consistency and avoid problems with bisection. Original PR descriptions follow: * Fix inlining of `return_call*` (#6448) Previously we transformed return calls in inlined function bodies into normal calls followed by branches out to the caller code. Similarly, when inlining a `return_call` callsite, we simply added a `return` after the body inlined at the callsite. These transformations would have been correct if the semantics of return calls were to call and then return, but they are not correct for the actual semantics of returning and then calling. The previous implementation is observably incorrect for return calls inside try blocks, where the previous implementation would run the inlined body within the try block, but the proper semantics would be to run the inlined body outside the try block. Fix the problem by transforming inlined return calls to branches followed by calls rather than as calls followed by branches. For the case of inlined return call callsites, insert branches out of the original body of the caller and inline the body of the callee as a sibling of the original caller body. For the other case of return calls appearing in inlined bodies, translate the return calls to branches out to calls inserted as siblings of the original inlined body. In both cases, it would have been convenient to use multivalue block return to send call parameters along the branches to the calls, but unfortunately in our IR that would have required tuple-typed scratch locals to unpack the tuple of operands at the call sites. It is simpler to just use locals to propagate the operands in the first place. * Fix interpretation of `return_call*` (#6451) We previously interpreted return calls as calls followed by returns, but that is not correct both because it grows the size of the execution stack and because it runs the called functions in the wrong context, which can be observable in the case of exception handling. Update the interpreter to handle return calls correctly by adding a new `RETURN_CALL_FLOW` that behaves like a return, but carries the arguments and reference to the return-callee rather than normal return values. `callFunctionInternal` is updated to intercept this flow and call return-called functions in a loop until a function returns with some other kind of flow. Pull in the upstream spec tests return_call.wast, return_call_indirect.wast, and return_call_ref.wast with light editing so that we parse and validate them successfully. * Handle return calls in wasm-ctor-eval (#6464) When an evaluated export ends in a return call, continue evaluating the return-called function. This requires propagating the parameters, handling the case that the return-called function might be an import, and fixing up local indices in case the final function has different parameters than the original function. * Update effects.h to handle return calls correctly (#6470) As far as their surrounding code is concerned return calls are no different from normal returns. It's only from a caller's perspective that a function containing a return call also has the effects of the return-callee. To model this more precisely in EffectAnalyzer, stash the throw effect of return-callees on the side and only merge it in at the end when analyzing the effects of a full function body.
Update the expected output of `test_eval_ctors_debug_output` to account for changed debug output due to WebAssembly/binaryen#6464. The test will now succeed before and after that upstream Binaryen change.
Update the expected output of `test_eval_ctors_debug_output` to account for changed debug output due to WebAssembly/binaryen#6464. The test will now succeed before and after that upstream Binaryen change.
Update the expected output of `test_eval_ctors_debug_output` to account for changed debug output due to WebAssembly/binaryen#6464. The test will now succeed before and after that upstream Binaryen change.
When an evaluated export ends in a return call, continue evaluating the
return-called function. This requires propagating the parameters, handling the
case that the return-called function might be an import, and fixing up local
indices in case the final function has different parameters than the original
function.