Pop in catch emitted inside a block ("not enough arguments on the stack" from VM)

[kripken]

(module
 (tag $tag (param i32))

 (func $foo
  (local $temp i32)
  (try
   (do
    (nop)
   )
   (catch $tag
    (call $call
     (block (result i32)
      (local.set $temp
       (pop i32)
      )
      (call $nop)
      (local.get $temp)
     )
    )
   )
  )
 )

 (func $call (param i32))
 (func $nop)
)

In this IR the pop exists in a place that Binaryen accepts but wasm does not: we use a pop to get the catch value, but we are inside a block so the wasm does not validate, and a VM will error on something like this:

CompileError: WebAssembly.instantiate(): Compiling function #0 failed: not enough arguments on the stack for local.set (need 1, got 0) @+43)

That is with just translating that wat into a binary, bin/wasm-opt a.wat -all -o a.out.wasm. However, if we add a --roundtrip in that command, then the IR becomes this:

   (catch $tag$0
    (local.set $temp
     (pop i32)
    )
    (call $call
     (block $label$3 (result i32)
      (call $nop)
      (local.get $temp)
     )
    )
   )

Note how the pop was moved into a proper position. So it seems like we have some logic in the binary reading code that handles this somehow, and so using --roundtrip is a workaround, at least, so this is probably not urgent.

I noticed this in wasm GC that had this:

   (catch $tag$0
    (throw $tag$0
     (ref.cast
      (pop (ref null $type$16))
      (global.get $global$1)
     )
    )
   )

That cast can be statically removed, leading to:

   (catch $tag$0
    (throw $tag$0
     (block (result (ref null $type$16))
      (local.set $4
       (pop (ref null $type$16))
      )
      (drop
       (global.get $global$1)
      )
      (local.get $4)
     )
    )
   )

And that is how the problem can occur.

This does not seem specific to wasm GC, but perhaps we have more peephole opts that end up emitting small blocks, which increase the risk.

[tlively]

Inserting a new block around a pop will have this problem until we support block param types. Perhaps a short-term fix would be to not add such blocks when there is a danglingPop effect on the body?

[kripken]

I considered that, but it would mean anywhere in the codebase that creates a block would need to check that - that would be invasive, I'm afraid, in many passes.

I hope a "fixup" process that runs during binary emitting might be practical somehow - maybe similar to what happens during --roundtrip (but I am not sure how that works).

[aheejin]

Yeah this is tricky... I had to fix many points because of this (e.g. #2885). I checked why --roundtrip fixes this and it looks it's not a feature but a bug. In your code, when reading local.set, it runs popExpression, which does not check if it goes across the current block boundary. So in this case it happens to generate correct code but I don't think this will fix things in general.

I'm not sure if we can come up with a general "fixup" pass.. Maybe we can.. But we have to make sure that those inner blocks generated between catch and pop are not targeted by any branches. My previous approach was just to find out all possibly violating optimizations and fix them, but I agree that it's not very scalable.

[tlively]

Maybe we should prioritize supporting block parameters to fix this problem "properly," then. I think the best way to do that would be to allow dangling pops at the beginning of arbitrary blocks, not just in catches, along with a new Type params field on Block.

[aheejin]

@tlively That would be nice, but that mean EH works only when multivalue is enabled right? I don't think that will be a problem in practice, because all engines that implement EH have implemented multivalue already.. But multivalue support is flaky in many parts still, so it might entail more than we think.

[kripken]

Thinking more on this, I think a general fixup would not be that hard. Basically it could scan catch blocks and see if the pop is nested in control flow, and if so, add a local to pop it immediately instead.

Block parameters would be more efficient, but also more work I think?

[tlively]

The fixup would also have to introduce a new local to hold the result of the new pop and replace the old pop with a local.get of that local, but yeah, that should work.

[aheejin]

@kripken I think that will work. Should that be a new pass, or something like TypeUpdating::handleNonDefaultableLocals that we run at the end of relevant passes?

[kripken]

@aheejin I think the second option sounds better. That way people running a pass don't need to always remember to run a fixup pass after it. If we do that, plus add validation of this, then we should be pretty safe I think.