Skip to content

Commit

Permalink
Fix interpretation of return_call* (#6451)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tlively
tlively authored Apr 8, 2024
1 parent b434f76 commit dce7c35
Show file tree
Hide file tree
Showing 13 changed files with 2,310 additions and 198 deletions.
113 changes: 84 additions & 29 deletions src/ir/effects.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,23 @@ class EffectAnalyzer {
// noticeable from the perspective of the caller, that is, effects that are
// only noticeable during the call, but "vanish" when the call stack is
// unwound.
//
// Unlike walking just the body, walking the function will also
// include the effects of any return calls the function makes. For that
// reason, it is a bug if a user of this code calls walk(Expression*) and not
// walk(Function*) if their intention is to scan an entire function body.
// Putting it another way, a return_call is syntax sugar for a return and a
// call, where the call executes at the function scope, so there is a
// meaningful difference between scanning an expression and scanning
// the entire function body.
void walk(Function* func) {
walk(func->body);

// Effects of return-called functions will be visible to the caller.
if (hasReturnCallThrow) {
throws_ = true;
}

// We can ignore branching out of the function body - this can only be
// a return, and that is only noticeable in the function, not outside.
branchesOut = false;
Expand Down Expand Up @@ -143,6 +157,22 @@ class EffectAnalyzer {
// or a continuation that is never continued, are examples of that.
bool mayNotReturn = false;

// Since return calls return out of the body of the function before performing
// their call, they are indistinguishable from normal returns from the
// perspective of their surrounding code, and the return-callee's effects only
// become visible when considering the effects of the whole function
// containing the return call. To model this correctly, stash the callee's
// effects on the side and only merge them in after walking a full function
// body.
//
// We currently do this stashing only for the throw effect, but in principle
// we could do it for all effects if it made a difference. (Only throw is
// noticeable now because the only thing that can change between doing the
// call here and doing it outside at the function exit is the scoping of
// try-catch blocks. If future wasm scoping additions are added, we may need
// more here.)
bool hasReturnCallThrow = false;

// Helper functions to check for various effect types

bool accessesLocal() const {
Expand Down Expand Up @@ -466,43 +496,63 @@ class EffectAnalyzer {
return;
}

const EffectAnalyzer* targetEffects = nullptr;
if (parent.funcEffectsMap) {
auto iter = parent.funcEffectsMap->find(curr->target);
if (iter != parent.funcEffectsMap->end()) {
targetEffects = &iter->second;
}
}

if (curr->isReturn) {
parent.branchesOut = true;
// When EH is enabled, any call can throw.
if (parent.features.hasExceptionHandling() &&
(!targetEffects || targetEffects->throws())) {
parent.hasReturnCallThrow = true;
}
}

if (parent.funcEffectsMap) {
auto iter = parent.funcEffectsMap->find(curr->target);
if (iter != parent.funcEffectsMap->end()) {
// We have effect information for this call target, and can just use
// that. The one change we may want to make is to remove throws_, if
// the target function throws and we know that will be caught anyhow,
// the same as the code below for the general path.
const auto& targetEffects = iter->second;
if (targetEffects.throws_ && parent.tryDepth > 0) {
auto filteredEffects = targetEffects;
filteredEffects.throws_ = false;
parent.mergeIn(filteredEffects);
} else {
// Just merge in all the effects.
parent.mergeIn(targetEffects);
}
return;
if (targetEffects) {
// We have effect information for this call target, and can just use
// that. The one change we may want to make is to remove throws_, if the
// target function throws and we know that will be caught anyhow, the
// same as the code below for the general path. We can always filter out
// throws for return calls because they are already more precisely
// captured by `branchesOut`, which models the return, and
// `hasReturnCallThrow`, which models the throw that will happen after
// the return.
if (targetEffects->throws_ && (parent.tryDepth > 0 || curr->isReturn)) {
auto filteredEffects = *targetEffects;
filteredEffects.throws_ = false;
parent.mergeIn(filteredEffects);
} else {
// Just merge in all the effects.
parent.mergeIn(*targetEffects);
}
return;
}

parent.calls = true;
// When EH is enabled, any call can throw.
if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) {
// When EH is enabled, any call can throw. Skip this for return calls
// because the throw is already more precisely captured by the combination
// of `hasReturnCallThrow` and `branchesOut`.
if (parent.features.hasExceptionHandling() && parent.tryDepth == 0 &&
!curr->isReturn) {
parent.throws_ = true;
}
}
void visitCallIndirect(CallIndirect* curr) {
parent.calls = true;
if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) {
parent.throws_ = true;
}
if (curr->isReturn) {
parent.branchesOut = true;
if (parent.features.hasExceptionHandling()) {
parent.hasReturnCallThrow = true;
}
}
if (parent.features.hasExceptionHandling() &&
(parent.tryDepth == 0 && !curr->isReturn)) {
parent.throws_ = true;
}
}
void visitLocalGet(LocalGet* curr) {
Expand Down Expand Up @@ -745,21 +795,26 @@ class EffectAnalyzer {
}
}
void visitCallRef(CallRef* curr) {
if (curr->isReturn) {
parent.branchesOut = true;
if (parent.features.hasExceptionHandling()) {
parent.hasReturnCallThrow = true;
}
}
if (curr->target->type.isNull()) {
parent.trap = true;
return;
}
parent.calls = true;
if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) {
parent.throws_ = true;
}
if (curr->isReturn) {
parent.branchesOut = true;
}
// traps when the call target is null
if (curr->target->type.isNullable()) {
parent.implicitTrap = true;
}

parent.calls = true;
if (parent.features.hasExceptionHandling() &&
(parent.tryDepth == 0 && !curr->isReturn)) {
parent.throws_ = true;
}
}
void visitRefTest(RefTest* curr) {}
void visitRefCast(RefCast* curr) {
Expand Down
Loading

0 comments on commit dce7c35

Please # to comment.