[tlively]

When entering a nested control flow scope whose input parameters are scratch local.gets from the enclosing scope's inputs, reuse the enclosing scope's scratch locals directly instead of allocating new scratch locals. Share the movement logic in pushScope with moving legacy EH pops into nested unnamed blocks.

[gemini-code-assist]

Code Review

This pull request optimizes input parameter handling in IRBuilder::pushScope by allowing pops to move into unnamed blocks and reusing scratch locals from enclosing scopes instead of allocating new ones. It also simplifies pop validation in IRBuilder::makePop and adds comprehensive tests for block tunneling and misplaced pop parse errors. The review feedback points out a critical correctness bug in pushScope where isInputGet fails to verify that the LocalGet index matches the parent local's index, which could trigger an assertion failure or cause incorrect optimization. Additionally, the reviewer suggests wrapping the parameter matching loop in an if (hasInput) block to prevent potential out-of-bounds indexing of Type::none when hasInput is false.

[gemini-code-assist]

There are two significant issues with the current implementation of the input parameter matching loop:

1. Critical Correctness Bug (Assertion Failure / Incorrect Optimization):
   The isInputGet check verifies that expr is a LocalGet of the correct type and that parentIndex < parentLocals.size(). However, it does not verify that the index of the LocalGet actually matches parentLocals[parentIndex].
   If a nested block takes an input that is a local.get of a different local variable (which is perfectly valid WebAssembly), isInputGet will evaluate to true. This will trigger a failed assertion (assert(expr->cast<LocalGet>()->index == parentLocals[parentIndex])) in debug builds, or cause incorrect optimization/reuse of the wrong local in release builds.
   To fix this, we must explicitly check that expr->cast<LocalGet>()->index == parentLocals[parentIndex] as part of isInputGet.

2. Out-of-Bounds Indexing & Redundant Loop:
   If hasInput is false, scope.inputType is Type::none (which has a size of 0). However, if inputs contains a Pop (size 1), the loop still runs. Inside the loop, scope.inputType[i] is evaluated. Although short-circuiting currently prevents a crash when expr is a Pop, relying on this is extremely fragile.
   Wrapping the entire block in if (hasInput) completely avoids any potential out-of-bounds indexing of Type::none and avoids running a redundant loop when hasInput is false.

    if (hasInput) {
      const auto& parentLocals = getScope().inputLocals;
      // The new scope may only receive a suffix of the outer scope's parameters,
      // so match inputs starting from the remaining size of the outer stack
      // rather than from the beginning.
      size_t baseIndex = getScope().exprStack.size();
      for (size_t i = 0; i < inputs.size(); ++i) {
        // Move an enclosing scope's input expression (a scratch local.get or a
        // catch pop) directly into this scope rather than allocating a new
        // scratch local.
        auto* expr = inputs[i];
        size_t parentIndex = baseIndex + i;
        bool isInputGet = expr->is<LocalGet>() &&
                          expr->type == scope.inputType[i] &&
                          parentIndex < parentLocals.size() &&
                          expr->cast<LocalGet>()->index == parentLocals[parentIndex];
        bool isPop = isUnnamedBlock && inputs.size() == 1 && expr->is<Pop>();
        if (isInputGet) {
          scope.inputLocals.push_back(parentLocals[parentIndex]);
        } else if (!isPop) {
          auto scratch = addScratchLocal(scope.inputType[i]);
          CHECK_ERR(scratch);
          scope.inputLocals.push_back(*scratch);
          pushSynthetic(builder.makeLocalSet(*scratch, expr));
          inputs[i] = builder.makeLocalGet(*scratch, scope.inputType[i]);
        }
      }
    }

[github-actions]

🤖 Google Antigravity Code Review

I have reviewed the changes in this Pull Request and left inline feedback.

[github-actions]

[CRITICAL | Confidence: 5/5 | BUG]

isInputGet assumes that any LocalGet on the stack whose position matches parentIndex < parentLocals.size() must be the synthetic scratch local.get representing the parent scope's input. It delegates verifying the local index to assert(expr->cast<LocalGet>()->index == parentLocals[parentIndex]) rather than checking it as part of the condition.

However, inputs can contain a user local.get (for example, if user code in the enclosing scope pushes local.get $x after consuming outer inputs, or passes a local variable into an inner block). In such cases, if parentIndex < parentLocals.size() holds:

1. In debug builds, the assertion fails and crashes the compiler.
2. In release builds (with assertions disabled), scope.inputLocals.push_back(parentLocals[parentIndex]) is executed and no scratch local is created for expr. If the inner scope reuses inputLocals (e.g. an if's synthesized else arm, an explicit else, or a loop branch trampoline), it reads parentLocals[parentIndex] containing the outer block's input rather than expr's value, causing silent data corruption.

The local index match must be part of isInputGet:

Suggested change

	assert(expr->cast<LocalGet>()->index == parentLocals[parentIndex]);
	bool isInputGet = parentIndex < parentLocals.size() &&
	expr->is<LocalGet>() &&
	expr->cast<LocalGet>()->index == parentLocals[parentIndex] &&
	expr->type == scope.inputType[i];

[github-actions]

[HIGH | Confidence: 5/5 | BUG]

When a Pop expression is moved into an unnamed block, the comment notes:

(Note that the block may receive a name later if we encounter a break to it; in that case, the fixups that run later will have work to do.)

However, the nested pop fixup pass (EHUtils::handleBlockNestedPops) in finishScope(Function*) is gated by:

if (scope.needsPopFixup()) {
  EHUtils::handleBlockNestedPops(func, wasm, EHUtils::FeaturePolicy::RunIfNoEH);
}

needsPopFixup() in wasm-ir-builder.h returns funcScope->hasSyntheticBlock && funcScope->hasPop. For an explicit AST block that was originally unnamed and later assigned a name due to a branch targeting it (br 0), hasSyntheticBlock is never set to true (it is only set in packageAsBlock).

As a consequence, needsPopFixup() returns false, handleBlockNestedPops is never invoked, and the function ends up with a Pop nested inside a named Block. This produces invalid IR and fails module validation with catch's body (...) 's pop's location is not valid.

To ensure the fixup runs, scopeStack[0].noteSyntheticBlock() (or an explicit flag indicating that a block containing a pop was created/named) must be set when moving a Pop into an unnamed block.

[aheejin]

It looks this also contains changes in #9336. Are you going to rebase?