[tlively]

When IRBuilder::makeStackSwitch constructed a StackSwitch expression via Builder::makeStackSwitch, it did not pass the continuation type or initialize ret->type prior to calling ret->finalize(). When the continuation operand had a bottom reference type, StackSwitch::finalize() called getMaximallyUninhabitable(type) while type was still default-constructed Type::none, leaving the StackSwitch with Type::none instead of the uninhabitable refinement of the target continuation's return continuation parameters. This incorrect typing would cause parse failures on later instructions.

Although the problem primarily appears when parsing WAT we would never print (specifically switch with a null parameter), the fuzzer can also observe it when round-tripping a binary after Stack IR optimizations elide a local that was holding the null value.

Fix the problem by passing the continuation type to Builder::makeStackSwitch and initializing ret->type from the last parameter's continuation signature before calling ret->finalize(). Add validation in IRBuilder::makeStackSwitch that the last parameter of the switch continuation is a continuation type to avoid assertion failures or UB.

[gemini-code-assist]

Code Review

This pull request updates the stack switching implementation to compute the result type of a StackSwitch from its continuation type immediate, preventing issues when the continuation is null. It also adds validation to ensure the last parameter of a switch continuation is a continuation type, along with corresponding tests. The review feedback highlights potential assertion failures or undefined behavior if basic continuation types (such as nocont) are passed to getContinuation(). It is recommended to check if the heap types are basic and handle them safely by either setting the type to Type::unreachable or returning an error.

[gemini-code-assist]

If contType or lastParam is a basic continuation type (such as nocont), calling getContinuation() on it will result in an assertion failure or undefined behavior because basic heap types do not have a continuation signature. We should check if the heap type is basic and handle it safely by setting the type to Type::unreachable.

    if (contType.isBasic()) {
      ret->type = Type::unreachable;
    } else {
      auto params = contType.getContinuation().type.getSignature().params;
      assert(params.size() > 0);
      auto lastParam = params[params.size() - 1];
      assert(lastParam.isContinuation());
      if (lastParam.getHeapType().isBasic()) {
        ret->type = Type::unreachable;
      } else {
        ret->type =
          lastParam.getHeapType().getContinuation().type.getSignature().params;
      }
    }

[gemini-code-assist]

If ct is a basic continuation type (such as nocont), calling getContinuation() on it will cause an assertion failure or undefined behavior. We should validate that ct is not a basic type before retrieving its continuation signature.

  if (ct.isBasic()) {
    return Err{"expected non-basic continuation type annotation on switch"};
  }
  auto params = ct.getContinuation().type.getSignature().params;
  auto nparams = params.size();

[kripken]

Do we do this kind of thing in other parts of wasm-builder? This looks a lot more complex than I am used to seeing in this file.

[kripken]

How about passing in the type of the last param, which I think the caller has?

[tlively]

Hmmm maybe. The current implementation is consistent with makeResume and makeResumeThrow in that it takes the type of the continuation being passed to the instruction. It's also consistent with all makeXYZNew instructions in that it takes the same type that appears as the type immediate on the corresponding Wasm instruction. The complexity here comes from the inherent complexity of figuring out switch's result type from its type immediates, so is not necessarily unexpected or a problem.

OTOH, makeStructGet and makeArrayGet just take their expected result types as parameters. They don't take parameters corresponding to their Wasm type immediates and they don't do any kind of lookup at all. We could update all the stack switching builder methods to do the same. If you prefer that, I could do it as a preliminary PR.

WDYT?

[kripken]

Yeah, I guess this complexity is reasonable, given the spec. sgtm as is.